fix(node:test): pass TestContext to hook callbacks#26610
Closed
ddmoney420 wants to merge 4 commits into
Closed
Conversation
…ion parsing (phase 1)
- Add captureStdout and captureStderr fields to WorkerOptions struct
- Parse stdout/stderr options in JSWorker.cpp without throwing errors
- Pass flags through Worker.cpp call chain to web_worker.zig
- Extend WebWorker struct to store capture configuration
- Options now flow correctly from JavaScript through C++ to Zig layer
Completed acceptance criteria:
✓ new Worker(file, { stdout: true }) does not throw
✓ new Worker(file, { stderr: true }) does not throw
✓ resourceLimits option accepted
Remaining work (Phase 2-4):
- Create pipes in worker initialization
- Implement FD redirection to pipes
- Wire event loop integration for PipeReader
- Implement stream dispatch functions
- Create ReadableStream objects in TypeScript layer
This establishes the foundation for full stdout/stderr capture support,
following the pattern used in subprocess.zig for reference.
Implements the Node.js worker_threads `stdout` and `stderr` options that allow capturing worker output as ReadableStreams. This unblocks Jest, Vitest, AVA, BullMQ, and other tools that depend on this feature. Changes: - Create pipes in WebWorker.create() when capture flags are set - Redirect worker stdout/stderr to pipe write ends using dup2() - Add JSWorker.cpp getters that create ReadableStreams from pipe FDs - Add Worker.h impl() getter for Zig WebWorker access - Add comprehensive tests for stdout/stderr capture Closes oven-sh#10768 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixes oven-sh#26170 The `t.after()`, `t.before()`, `t.beforeEach()`, and `t.afterEach()` callbacks were not receiving the TestContext as their first argument, which is required by the Node.js API. This change: - Updates `createHook` to accept an optional context parameter - Passes `this` (the TestContext) from the hook methods - Updates the HookFn type to accept optional TestContext Now code like this works correctly: ```javascript test("my test", (t) => { t.after((ctx) => { ctx.diagnostic("after hook called"); }); }); ``` Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
WalkthroughAdds stdout/stderr stream capture support for Node.js Worker threads across TypeScript, C++, and Zig layers. Introduces infrastructure for Chant task automation. Updates node:test hooks to receive TestContext parameter. Implements comprehensive tests for worker stream capture and hook context passing. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In @.chant/prompts/documentation.md:
- Around line 10-14: Replace the emphasized spec title line
("**{{spec.title}}**") with a proper Markdown heading to satisfy markdownlint
MD036; locate the template in .chant/prompts/documentation.md where the block
shows "## Your Spec" and change the bold title line to a heading (for example
"### {{spec.title}}" or another appropriate heading level) so the title renders
as a heading instead of emphasis.
- Around line 56-100: Template fenced code blocks lack surrounding blank lines
causing markdownlint MD031 failures; update the templates (e.g., the "API
Documentation" section in .chant/prompts/documentation.md and all other example
blocks in Architecture/README and README templates) by ensuring there is an
empty line immediately before each opening ``` and immediately after each
closing ```; apply this consistently to all fenced blocks (examples, code
snippets, and markdown examples) so every fenced block has a blank line above
and below.
In @.chant/prompts/split.md:
- Around line 6-104: The markdown is failing markdownlint rules MD022/MD031
because there are missing blank lines around headings and fenced code blocks;
update .chant/prompts/split.md to ensure there is an empty line before and after
each ATX heading (e.g., "## Output Format", "## Member 1:", "## Member 2:") and
add blank lines immediately before and after each fenced code block (``` ...
```), and also ensure spacing around the <details>/<summary> block so headings
and fences are separated by a blank line; make these whitespace-only edits so
content and examples like the "## Member 1:"/ "## Member 2:" blocks and the
triple-backtick fenced examples remain unchanged but are surrounded by the
required blank lines.
In @.chant/prompts/standard.md:
- Around line 6-49: The markdown violates MD022/MD031 because there are missing
blank lines around headings and the fenced code block in the list item; edit the
section containing the heading "If no existing spec found, create one:" and the
following fenced block (the triple-backtick bash snippet starting with just
chant add) to ensure there is a blank line before the heading and a blank line
both before and after the fenced block (and ensure list-item indentation is
preserved), so add the required empty lines around those elements to satisfy the
linter.
In @.chant/prompts/verify.md:
- Around line 10-38: Change the emphasized title block that renders
"{{spec.title}}" into a proper Markdown heading (e.g., replace any surrounding
emphasis markers around {{spec.title}} with a leading # or ##) and add a
language identifier to the fenced code block under "Output format" so the
triple-backtick fence becomes something like ```markdown to satisfy MD036/MD040;
update the template lines that include "{{spec.title}}" and the ``` fence near
"## Verification Summary" accordingly.
In @.chant/specs/2026-01-29-001-wh8.md:
- Around line 30-37: The fenced JavaScript code block under the "**Current
behavior:**" heading is not surrounded by blank lines (MD031); add a blank line
after the heading and a blank line after the closing ``` fence so the block is
separated from surrounding text—locate the "**Current behavior:**" section and
adjust spacing around the ```javascript ... ``` fenced block accordingly.
- Around line 165-213: Wrap the bare URLs (e.g., the GitHub Issue URL
"https://github.com/oven-sh/bun/issues/10768" and the Node.js docs URL
"https://nodejs.org/api/worker_threads.html") in Markdown link syntax or angle
brackets (e.g., [text](URL) or <URL>) to satisfy MD034/MD047, and ensure the
file ends with a single trailing newline (add an EOF newline) so the Markdown
linter stops flagging a missing newline.
- Around line 124-158: The Markdown headings (e.g., "Implementation Notes",
"Phase 1: Option Parsing (COMPLETED)", "Phase 2-4: Pipe Infrastructure (NOT YET
COMPLETED)", "Reference Implementations") are missing required blank lines; add
a single blank line before and after each heading and repeat the same for all
adjacent headings in this section so each heading is separated from surrounding
content per MD022.
In @.gitattributes:
- Around line 60-61: The .gitattributes entry references a custom merge driver
"chant-spec" but that driver is not defined; add a Git merge driver
configuration for "chant-spec" (e.g. a [merge "chant-spec"] section in
repository .git/config or the user's ~/.gitconfig) that defines a name and a
driver command and appropriate options (driver = <command> and optionally
trustExitCode = true) so Git can invoke the custom merge tool for the
.chant/specs/*.md pattern declared in .gitattributes; ensure the driver command
is installed or committed into the repo and documented so merges on
.chant/specs/*.md succeed.
In `@src/bun.js/bindings/webcore/WorkerOptions.h`:
- Around line 37-40: The comment in WorkerOptions.h mentions stdout/stderr/stdin
but the struct only defines captureStdout and captureStderr; either add a bool
captureStdin member to WorkerOptions (e.g., bool captureStdin { false };) if
stdin capture is supported, or change the comment to remove "stdin" so it
accurately reflects the available fields (captureStdout and captureStderr).
Update any related documentation or usages that reference stdin to match the
chosen approach.
In `@src/bun.js/web_worker.zig`:
- Around line 733-746: In WebWorker__createStdoutStream and
WebWorker__createStderrStream, don't null out
worker.stdout_read_fd/worker.stderr_read_fd before calling
createReadableStreamFromFd; instead call createReadableStreamFromFd(fd) first
and on success set the worker's *_read_fd to null, and on failure ensure the
original fd is closed (use the platform close/OS FD close function available in
the module) before returning .null so the pipe FD is not leaked.
In `@src/js/node/worker_threads.ts`:
- Around line 287-300: The stdin/stdout/stderr getters in the WorkerThread class
currently always return null; update each getter (stdin, stdout, stderr) to
delegate to the underlying WebWorker instance's corresponding property (e.g.,
this.worker.stdin / this.worker.stdout / this.worker.stderr) and return that
value if present, otherwise fall back to null; for stdout/stderr also ensure you
respect the capture options (captureStdout/captureStderr) by returning the
underlying ReadableStream when available and when the option is enabled,
otherwise return null.
In `@test/regression/issue/26170.test.ts`:
- Around line 1-44: The spawned process captures stderr but does not assert it;
update the test "t.after() callback receives TestContext" to assert that the
captured stderr is empty (e.g., add an assertion on the variable stderr after
awaiting proc.stdout.text(), proc.stderr.text(), and proc.exited) to surface
hook errors immediately, and ensure you also assert the process exit code
variable exitCode (the existing expect(exitCode).toBe(0) is fine); apply the
same stderr-empty assertion pattern to the other three spawn blocks that perform
similar hook checks so all spawned proc uses (proc, stdout, stderr, exitCode)
validate stderr is empty.
Comment on lines
+10
to
+14
| ## Your Spec | ||
|
|
||
| **{{spec.title}}** | ||
|
|
||
| {{spec.description}} |
Contributor
There was a problem hiding this comment.
Use a heading for the spec title instead of bold text.
markdownlint (MD036) flags emphasis-as-heading; promoting to a heading keeps lint clean.
✏️ Suggested change
-**{{spec.title}}**
+### {{spec.title}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Your Spec | |
| **{{spec.title}}** | |
| {{spec.description}} | |
| ## Your Spec | |
| ### {{spec.title}} | |
| {{spec.description}} |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 12-12: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In @.chant/prompts/documentation.md around lines 10 - 14, Replace the emphasized
spec title line ("**{{spec.title}}**") with a proper Markdown heading to satisfy
markdownlint MD036; locate the template in .chant/prompts/documentation.md where
the block shows "## Your Spec" and change the bold title line to a heading (for
example "### {{spec.title}}" or another appropriate heading level) so the title
renders as a heading instead of emphasis.
Comment on lines
+56
to
+100
| #### API Documentation | ||
|
|
||
| For documenting public APIs and interfaces: | ||
|
|
||
| ```markdown | ||
| # Module Name | ||
|
|
||
| Brief description of what this module does. | ||
|
|
||
| ## Overview | ||
|
|
||
| Explain the purpose and when to use this module. | ||
|
|
||
| ## Types | ||
|
|
||
| ### `TypeName` | ||
|
|
||
| Description of the type. | ||
|
|
||
| **Fields:** | ||
| - `field_name: Type` — Description of field | ||
|
|
||
| **Example:** | ||
| \```rust | ||
| let example = TypeName { field: value }; | ||
| \``` | ||
|
|
||
| ## Functions | ||
|
|
||
| ### `function_name(param: Type) -> ReturnType` | ||
|
|
||
| Description of what the function does. | ||
|
|
||
| **Parameters:** | ||
| - `param` — Description of parameter | ||
|
|
||
| **Returns:** Description of return value | ||
|
|
||
| **Example:** | ||
| \```rust | ||
| let result = function_name(argument); | ||
| \``` | ||
|
|
||
| **Errors:** When and why it returns errors | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add blank lines around fenced code blocks in the templates.
markdownlint (MD031) expects blank lines before/after fenced blocks; apply to all example blocks (API/Architecture/README).
✏️ Example fix (apply similarly to other blocks)
-For documenting public APIs and interfaces:
-
-```markdown
+For documenting public APIs and interfaces:
+
+```markdown
...
-```
+```
+🤖 Prompt for AI Agents
In @.chant/prompts/documentation.md around lines 56 - 100, Template fenced code
blocks lack surrounding blank lines causing markdownlint MD031 failures; update
the templates (e.g., the "API Documentation" section in
.chant/prompts/documentation.md and all other example blocks in
Architecture/README and README templates) by ensuring there is an empty line
immediately before each opening ``` and immediately after each closing ```;
apply this consistently to all fenced blocks (examples, code snippets, and
markdown examples) so every fenced block has a blank line above and below.
Comment on lines
+6
to
+104
| # Split Driver Specification into Member Specs | ||
|
|
||
| You are analyzing a driver specification for the {{project.name}} project and proposing how to split it into smaller, ordered member specs. | ||
|
|
||
| **IMPORTANT: This is an analysis task. Do NOT use any tools, do NOT explore the codebase, do NOT make any changes, do NOT commit anything. ONLY output text in the exact format specified below.** | ||
|
|
||
| ## Driver Specification to Split | ||
|
|
||
| **ID:** {{spec.id}} | ||
| **Title:** {{spec.title}} | ||
|
|
||
| {{spec.description}} | ||
|
|
||
| ## Your Task | ||
|
|
||
| 1. Analyze the specification and its acceptance criteria | ||
| 2. Propose a sequence of member specs where: | ||
| - Each member leaves code in a compilable state | ||
| - Each member is independently testable and valuable | ||
| - Dependencies are minimized (parallelize where possible) | ||
| - Common patterns are respected (add new alongside old → update callers → remove old) | ||
| 3. For each member, provide: | ||
| - A clear, concise title | ||
| - Description of what should be implemented | ||
| - Explicit acceptance criteria with checkboxes for verification | ||
| - Edge cases that should be considered | ||
| - Example test cases where applicable | ||
| - List of affected files (if identifiable from the spec) | ||
| - Clear "done" conditions that can be verified | ||
|
|
||
| ## Complexity Thresholds (Linting-Aware) | ||
|
|
||
| Each resulting member spec should meet these thresholds to pass linting: | ||
| - **Acceptance Criteria:** ≤ 5 items (allows haiku to verify completion) | ||
| - **Target Files:** ≤ 5 files (keeps scope focused, minimal coupling) | ||
| - **Description Length:** ≤ 200 words (haiku-friendly, clear intent) | ||
|
|
||
| These thresholds ensure the split produces specs that are: | ||
| - **Independently executable** by Claude Haiku | ||
| - **Verifiable** with clear, specific acceptance criteria | ||
| - **Self-contained** without cross-references | ||
|
|
||
| ## Why Thorough Acceptance Criteria? | ||
|
|
||
| These member specs will be executed by Claude Haiku, a capable but smaller model. A strong model (Opus/Sonnet) doing the split should think through edge cases and requirements thoroughly. Each member must have: | ||
|
|
||
| - **Specific checkboxes** for each piece of work (not just "implement it") | ||
| - **Edge case callouts** to prevent oversights | ||
| - **Test scenarios** to clarify expected behavior | ||
| - **Clear success metrics** so Haiku knows when it's done | ||
| - **Within complexity thresholds** so the spec stays manageable for haiku | ||
|
|
||
| This way, Haiku has a detailed specification to follow and won't miss important aspects. | ||
|
|
||
| ## Preventing Cross-References | ||
|
|
||
| Resulting member specs must be independent and not reference each other: | ||
| - **No spec ID cross-references** in member descriptions (no mentions of `.1`, `.2`, etc.) | ||
| - **Separate target_files** whenever possible (avoid coupling through shared files) | ||
| - **Each spec self-contained** with clear acceptance criteria (no implicit dependencies beyond the dependency chain) | ||
|
|
||
| This ensures members can be executed in parallel where dependencies allow. | ||
|
|
||
| ## Output Format | ||
|
|
||
| **CRITICAL: Output ONLY the member specs in EXACTLY this format. No preamble, no summary, no tool use.** | ||
|
|
||
| Start your output directly with `## Member 1:` and continue with each member. | ||
|
|
||
| ``` | ||
| ## Member 1: <title> | ||
|
|
||
| <description of what this member accomplishes> | ||
|
|
||
| ### Acceptance Criteria | ||
|
|
||
| - [ ] Specific criterion 1 | ||
| - [ ] Specific criterion 2 | ||
| - [ ] Specific criterion 3 | ||
|
|
||
| ### Edge Cases | ||
|
|
||
| - Edge case 1: Describe what should happen and how to test it | ||
| - Edge case 2: Describe what should happen and how to test it | ||
|
|
||
| ### Example Test Cases | ||
|
|
||
| For this feature, verify: | ||
| - Case 1: Input X should produce Y | ||
| - Case 2: Input A should produce B | ||
|
|
||
| **Affected Files:** | ||
| - file1.rs | ||
| - file2.rs | ||
|
|
||
| ## Member 2: <title> | ||
|
|
||
| ... (continue with same format) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix markdownlint spacing around headings and fenced blocks.
markdownlint flags missing blank lines around headings and fences; add empty lines to satisfy MD022/MD031.
✍️ Suggested formatting adjustments
-# Split Driver Specification into Member Specs
-
-You are analyzing a driver specification for the {{project.name}} project and proposing how to split it into smaller, ordered member specs.
+# Split Driver Specification into Member Specs
+
+You are analyzing a driver specification for the {{project.name}} project and proposing how to split it into smaller, ordered member specs.
+
@@
-## Output Format
-
-**CRITICAL: Output ONLY the member specs in EXACTLY this format. No preamble, no summary, no tool use.**
-
-Start your output directly with `## Member 1:` and continue with each member.
-
-```
+## Output Format
+
+**CRITICAL: Output ONLY the member specs in EXACTLY this format. No preamble, no summary, no tool use.**
+
+Start your output directly with `## Member 1:` and continue with each member.
+
+```
## Member 1: <title>
@@
## Member 2: <title>
@@-If no files are identified, you can omit the Affected Files section.
+
+If no files are identified, you can omit the Affected Files section.
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.20.0)</summary>
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
[warning] 31-31: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
[warning] 36-36: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
[warning] 42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
[warning] 49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @.chant/prompts/split.md around lines 6 - 104, The markdown is failing
markdownlint rules MD022/MD031 because there are missing blank lines around
headings and fenced code blocks; update .chant/prompts/split.md to ensure there
is an empty line before and after each ATX heading (e.g., "## Output Format",
"## Member 1:", "## Member 2:") and add blank lines immediately before and after
each fenced code block (...), and also ensure spacing around the
/
block so headings and fences are separated by a blank line; make these whitespace-only edits so content and examples like the "## Member 1:"/ "## Member 2:" blocks and the triple-backtick fenced examples remain unchanged but are surrounded by the required blank lines. ````
Comment on lines
+6
to
+49
| # Execute Spec | ||
|
|
||
| You are implementing a spec for {{project.name}}. | ||
|
|
||
| ## Your Spec | ||
|
|
||
| **{{spec.title}}** | ||
|
|
||
| {{spec.description}} | ||
|
|
||
| ## Instructions | ||
|
|
||
| 1. **Read** the relevant code first | ||
| 2. **Plan** your approach before coding | ||
| 3. **Implement** the changes | ||
| 4. **Run `cargo fmt`** to format the code | ||
| 5. **Run `cargo clippy`** to fix any lint errors and warnings | ||
| 6. **Run tests** with `just test` and fix any failures | ||
| 7. **Verify** the implementation works and all acceptance criteria are met | ||
| 8. **Check off** each acceptance criterion in `{{spec.path}}` by changing `- [ ]` to `- [x]` | ||
| 9. **Commit** with message: `chant({{spec.id}}): <description>` | ||
| 10. **Verify git status is clean** - ensure no uncommitted changes remain | ||
|
|
||
| ## When You Notice Issues Outside Your Scope | ||
|
|
||
| If you encounter a problem that is NOT part of your current spec: | ||
|
|
||
| 1. **Check if a spec already exists** by running: `just chant list` | ||
| - Look for specs with similar titles or keywords related to the issue | ||
| - Check the `.chant/archive/` directory for completed specs | ||
|
|
||
| 2. **If no existing spec found, create one:** | ||
| ```bash | ||
| just chant add "Brief description of the issue" | ||
| ``` | ||
| - This creates a new spec documenting the problem for future resolution | ||
|
|
||
| 3. **Note in your output** that you've created a new spec with its ID | ||
|
|
||
| 4. **Continue with your original assignment** - do NOT fix the out-of-scope issue | ||
| - Focus on completing the current spec's acceptance criteria | ||
| - The new spec can be tackled separately | ||
|
|
||
| **Examples:** |
Contributor
There was a problem hiding this comment.
Fix markdownlint spacing around headings and fenced block.
MD022/MD031 will keep failing until blank lines are added around headings and the fenced block in the list item.
✏️ Example fix
---
name: standard
purpose: Default execution prompt
---
# Execute Spec-2. **If no existing spec found, create one:**
- ```bash
+2. **If no existing spec found, create one:**
+
+ ```bash
just chant add "Brief description of the issue"
```🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 31-31: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 36-36: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In @.chant/prompts/standard.md around lines 6 - 49, The markdown violates
MD022/MD031 because there are missing blank lines around headings and the fenced
code block in the list item; edit the section containing the heading "If no
existing spec found, create one:" and the following fenced block (the
triple-backtick bash snippet starting with just chant add) to ensure there is a
blank line before the heading and a blank line both before and after the fenced
block (and ensure list-item indentation is preserved), so add the required empty
lines around those elements to satisfy the linter.
Comment on lines
+10
to
+38
| ## Your Spec | ||
|
|
||
| **{{spec.title}}** | ||
|
|
||
| {{spec.description}} | ||
|
|
||
| ## Your Task | ||
|
|
||
| Review each acceptance criterion and determine if it is met. Use the three-status system below: | ||
|
|
||
| - **PASS**: The criterion is demonstrably satisfied (you verified code, tests, output, etc.) | ||
| - **FAIL**: The criterion is not satisfied or there is evidence it's broken | ||
| - **SKIP**: The criterion's status cannot be determined (ambiguous, no source to check, or requires manual review) | ||
|
|
||
| ## Reporting Format | ||
|
|
||
| For each criterion, provide exactly one status. When you SKIP a criterion, explain why briefly. | ||
|
|
||
| Output format: | ||
|
|
||
| ``` | ||
| ## Verification Summary | ||
|
|
||
| - [x] Criterion 1: PASS | ||
| - [ ] Criterion 2: FAIL | ||
| - [x] Criterion 3: SKIP — Unable to verify without running the full system | ||
|
|
||
| Overall status: PASS/FAIL/MIXED | ||
| ``` |
Contributor
There was a problem hiding this comment.
Address markdownlint: heading and fenced code language.
Convert the emphasized title to a heading and add a fence language to satisfy MD036/MD040.
✍️ Suggested formatting adjustments
-**{{spec.title}}**
+### {{spec.title}}
@@
-```
+```markdown
## Verification Summary
@@</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.20.0)</summary>
[warning] 12-12: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
---
[warning] 30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @.chant/prompts/verify.md around lines 10 - 38, Change the emphasized title
block that renders "{{spec.title}}" into a proper Markdown heading (e.g.,
replace any surrounding emphasis markers around {{spec.title}} with a leading #
or ##) and add a language identifier to the fenced code block under "Output
format" so the triple-backtick fence becomes something like markdown to satisfy MD036/MD040; update the template lines that include "{{spec.title}}" and the fence near "## Verification Summary" accordingly.
</details>
<!-- fingerprinting:phantom:triton:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+60
to
+61
| # Chant spec files use a custom merge driver for intelligent conflict resolution | ||
| .chant/specs/*.md merge=chant-spec |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the chant-spec merge driver is defined in git config
echo "=== Checking for chant-spec merge driver in git config ==="
git config --get merge.chant-spec.driver || echo "Warning: merge driver 'chant-spec' not found in git config"
echo ""
# Check the structure of .chant/specs/ directory
echo "=== Checking .chant/specs/ directory structure ==="
if [ -d ".chant/specs" ]; then
tree -L 3 .chant/specs/ 2>/dev/null || find .chant/specs/ -type f -name "*.md" | head -20
else
echo ".chant/specs/ directory does not exist yet"
fi
echo ""
# Also check .gitattributes content to see the context
echo "=== Current .gitattributes content around lines 60-61 ==="
sed -n '58,63p' .gitattributesRepository: oven-sh/bun
Length of output: 533
The custom merge driver chant-spec is not defined in Git configuration.
The .gitattributes file references merge=chant-spec, but the chant-spec merge driver is not configured in .git/config or .gitconfig. Git will fail during merge operations on .chant/specs/*.md files if this driver is not defined. Add the merge driver configuration before this gitattributes rule takes effect.
The pattern .chant/specs/*.md is appropriate for the current directory structure, which contains only root-level spec files.
🤖 Prompt for AI Agents
In @.gitattributes around lines 60 - 61, The .gitattributes entry references a
custom merge driver "chant-spec" but that driver is not defined; add a Git merge
driver configuration for "chant-spec" (e.g. a [merge "chant-spec"] section in
repository .git/config or the user's ~/.gitconfig) that defines a name and a
driver command and appropriate options (driver = <command> and optionally
trustExitCode = true) so Git can invoke the custom merge tool for the
.chant/specs/*.md pattern declared in .gitattributes; ensure the driver command
is installed or committed into the repo and documented so merges on
.chant/specs/*.md succeed.
Comment on lines
+37
to
+40
| // stdout/stderr/stdin options for node:worker_threads | ||
| bool captureStdout { false }; | ||
| bool captureStderr { false }; | ||
| // resourceLimits would go here, but may not be applicable to JSC |
Contributor
There was a problem hiding this comment.
Comment mentions stdin but no field exists.
The comment says stdout/stderr/stdin, yet only captureStdout/captureStderr are defined. Either add a captureStdin flag or adjust the comment to avoid implying unsupported stdin capture.
📝 Suggested fix
- // stdout/stderr/stdin options for node:worker_threads
+ // stdout/stderr options for node:worker_threads📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // stdout/stderr/stdin options for node:worker_threads | |
| bool captureStdout { false }; | |
| bool captureStderr { false }; | |
| // resourceLimits would go here, but may not be applicable to JSC | |
| // stdout/stderr options for node:worker_threads | |
| bool captureStdout { false }; | |
| bool captureStderr { false }; | |
| // resourceLimits would go here, but may not be applicable to JSC |
🤖 Prompt for AI Agents
In `@src/bun.js/bindings/webcore/WorkerOptions.h` around lines 37 - 40, The
comment in WorkerOptions.h mentions stdout/stderr/stdin but the struct only
defines captureStdout and captureStderr; either add a bool captureStdin member
to WorkerOptions (e.g., bool captureStdin { false };) if stdin capture is
supported, or change the comment to remove "stdin" so it accurately reflects the
available fields (captureStdout and captureStderr). Update any related
documentation or usages that reference stdin to match the chosen approach.
Comment on lines
+733
to
+746
| export fn WebWorker__createStdoutStream(worker: *WebWorker, globalObject: *jsc.JSGlobalObject) JSValue { | ||
| const fd = worker.stdout_read_fd orelse return .null; | ||
| worker.stdout_read_fd = null; // Transfer ownership to the stream | ||
| return createReadableStreamFromFd(globalObject, fd) catch return .null; | ||
| } | ||
|
|
||
| /// Create a ReadableStream from the stderr pipe FD. | ||
| /// Takes ownership of the FD - it will be closed when the stream is finalized. | ||
| /// Returns null if stderr is not being captured. | ||
| export fn WebWorker__createStderrStream(worker: *WebWorker, globalObject: *jsc.JSGlobalObject) JSValue { | ||
| const fd = worker.stderr_read_fd orelse return .null; | ||
| worker.stderr_read_fd = null; // Transfer ownership to the stream | ||
| return createReadableStreamFromFd(globalObject, fd) catch return .null; | ||
| } |
Contributor
There was a problem hiding this comment.
Close pipe FDs when stream creation fails to avoid leaks.
stdout_read_fd/stderr_read_fd are cleared before stream creation; on error the FD is lost and never closed. Close on error and only clear after success for both stdout and stderr.
🔧 Suggested fix
export fn WebWorker__createStdoutStream(worker: *WebWorker, globalObject: *jsc.JSGlobalObject) JSValue {
const fd = worker.stdout_read_fd orelse return .null;
- worker.stdout_read_fd = null; // Transfer ownership to the stream
- return createReadableStreamFromFd(globalObject, fd) catch return .null;
+ const stream = createReadableStreamFromFd(globalObject, fd) catch {
+ fd.close();
+ return .null;
+ };
+ worker.stdout_read_fd = null; // Transfer ownership to the stream
+ return stream;
}
export fn WebWorker__createStderrStream(worker: *WebWorker, globalObject: *jsc.JSGlobalObject) JSValue {
const fd = worker.stderr_read_fd orelse return .null;
- worker.stderr_read_fd = null; // Transfer ownership to the stream
- return createReadableStreamFromFd(globalObject, fd) catch return .null;
+ const stream = createReadableStreamFromFd(globalObject, fd) catch {
+ fd.close();
+ return .null;
+ };
+ worker.stderr_read_fd = null; // Transfer ownership to the stream
+ return stream;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export fn WebWorker__createStdoutStream(worker: *WebWorker, globalObject: *jsc.JSGlobalObject) JSValue { | |
| const fd = worker.stdout_read_fd orelse return .null; | |
| worker.stdout_read_fd = null; // Transfer ownership to the stream | |
| return createReadableStreamFromFd(globalObject, fd) catch return .null; | |
| } | |
| /// Create a ReadableStream from the stderr pipe FD. | |
| /// Takes ownership of the FD - it will be closed when the stream is finalized. | |
| /// Returns null if stderr is not being captured. | |
| export fn WebWorker__createStderrStream(worker: *WebWorker, globalObject: *jsc.JSGlobalObject) JSValue { | |
| const fd = worker.stderr_read_fd orelse return .null; | |
| worker.stderr_read_fd = null; // Transfer ownership to the stream | |
| return createReadableStreamFromFd(globalObject, fd) catch return .null; | |
| } | |
| export fn WebWorker__createStdoutStream(worker: *WebWorker, globalObject: *jsc.JSGlobalObject) JSValue { | |
| const fd = worker.stdout_read_fd orelse return .null; | |
| const stream = createReadableStreamFromFd(globalObject, fd) catch { | |
| fd.close(); | |
| return .null; | |
| }; | |
| worker.stdout_read_fd = null; // Transfer ownership to the stream | |
| return stream; | |
| } | |
| /// Create a ReadableStream from the stderr pipe FD. | |
| /// Takes ownership of the FD - it will be closed when the stream is finalized. | |
| /// Returns null if stderr is not being captured. | |
| export fn WebWorker__createStderrStream(worker: *WebWorker, globalObject: *jsc.JSGlobalObject) JSValue { | |
| const fd = worker.stderr_read_fd orelse return .null; | |
| const stream = createReadableStreamFromFd(globalObject, fd) catch { | |
| fd.close(); | |
| return .null; | |
| }; | |
| worker.stderr_read_fd = null; // Transfer ownership to the stream | |
| return stream; | |
| } |
🤖 Prompt for AI Agents
In `@src/bun.js/web_worker.zig` around lines 733 - 746, In
WebWorker__createStdoutStream and WebWorker__createStderrStream, don't null out
worker.stdout_read_fd/worker.stderr_read_fd before calling
createReadableStreamFromFd; instead call createReadableStreamFromFd(fd) first
and on success set the worker's *_read_fd to null, and on failure ensure the
original fd is closed (use the platform close/OS FD close function available in
the module) before returning .null so the pipe FD is not leaked.
Comment on lines
287
to
300
| get stdin() { | ||
| // TODO: | ||
| // TODO: implement stdin capture | ||
| return null; | ||
| } | ||
|
|
||
| get stdout() { | ||
| // TODO: | ||
| // TODO: implement stdout capture - return ReadableStream when captureStdout option is true | ||
| return null; | ||
| } | ||
|
|
||
| get stderr() { | ||
| // TODO: | ||
| // TODO: implement stderr capture - return ReadableStream when captureStderr option is true | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Expose worker stdio streams instead of always returning null.
If the underlying WebWorker now supports stdout/stderr capture, these getters mask that capability and keep node:worker_threads incompatible.
[suggested_fix]
🔧 Delegate to the underlying worker getters (fallback to null)
get stdin() {
- // TODO: implement stdin capture
- return null;
+ // TODO: implement stdin capture
+ return (this.#worker as any).stdin ?? null;
}
get stdout() {
- // TODO: implement stdout capture - return ReadableStream when captureStdout option is true
- return null;
+ // TODO: implement stdout capture - return ReadableStream when captureStdout option is true
+ return (this.#worker as any).stdout ?? null;
}
get stderr() {
- // TODO: implement stderr capture - return ReadableStream when captureStderr option is true
- return null;
+ // TODO: implement stderr capture - return ReadableStream when captureStderr option is true
+ return (this.#worker as any).stderr ?? null;
}🤖 Prompt for AI Agents
In `@src/js/node/worker_threads.ts` around lines 287 - 300, The
stdin/stdout/stderr getters in the WorkerThread class currently always return
null; update each getter (stdin, stdout, stderr) to delegate to the underlying
WebWorker instance's corresponding property (e.g., this.worker.stdin /
this.worker.stdout / this.worker.stderr) and return that value if present,
otherwise fall back to null; for stdout/stderr also ensure you respect the
capture options (captureStdout/captureStderr) by returning the underlying
ReadableStream when available and when the option is enabled, otherwise return
null.
Comment on lines
+1
to
+44
| // https://github.com/oven-sh/bun/issues/26170 | ||
| // t.after() callback should receive the test context | ||
| import { test, expect } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| test("t.after() callback receives TestContext", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const { test } = require("node:test"); | ||
|
|
||
| test("test with after hook", (t) => { | ||
| t.after((ctx) => { | ||
| if (ctx === undefined) { | ||
| console.log("FAIL: ctx is undefined"); | ||
| process.exit(1); | ||
| } | ||
| if (typeof ctx.diagnostic !== "function") { | ||
| console.log("FAIL: ctx.diagnostic is not a function, got:", typeof ctx.diagnostic); | ||
| process.exit(1); | ||
| } | ||
| // Call diagnostic to ensure it works | ||
| ctx.diagnostic("after hook called successfully"); | ||
| console.log("PASS: ctx received correctly"); | ||
| }); | ||
| }); | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([ | ||
| proc.stdout.text(), | ||
| proc.stderr.text(), | ||
| proc.exited, | ||
| ]); | ||
|
|
||
| expect(stdout).toContain("PASS: ctx received correctly"); | ||
| expect(stdout).not.toContain("FAIL:"); | ||
| expect(exitCode).toBe(0); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert stderr output for spawned hook checks.
stderr is captured but never asserted. Adding an assertion (and mirroring it in the other three spawn blocks) helps surface failing hook errors immediately.
As per coding guidelines, Always check exit codes and test error scenarios when spawning processes in tests.
✅ Suggested addition (apply similarly to other tests)
const [stdout, stderr, exitCode] = await Promise.all([
proc.stdout.text(),
proc.stderr.text(),
proc.exited,
]);
expect(stdout).toContain("PASS: ctx received correctly");
+ expect(stderr).toBe("");
expect(stdout).not.toContain("FAIL:");
expect(exitCode).toBe(0);🤖 Prompt for AI Agents
In `@test/regression/issue/26170.test.ts` around lines 1 - 44, The spawned process
captures stderr but does not assert it; update the test "t.after() callback
receives TestContext" to assert that the captured stderr is empty (e.g., add an
assertion on the variable stderr after awaiting proc.stdout.text(),
proc.stderr.text(), and proc.exited) to surface hook errors immediately, and
ensure you also assert the process exit code variable exitCode (the existing
expect(exitCode).toBe(0) is fine); apply the same stderr-empty assertion pattern
to the other three spawn blocks that perform similar hook checks so all spawned
proc uses (proc, stdout, stderr, exitCode) validate stderr is empty.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -0,0 +1,150 @@ | |||
| // https://github.com/oven-sh/bun/issues/26170 | |||
Contributor
There was a problem hiding this comment.
Code Review
No issues found. Checked for bugs and CLAUDE.md compliance.
| @@ -0,0 +1,150 @@ | |||
| // https://github.com/oven-sh/bun/issues/26170 | |||
Contributor
There was a problem hiding this comment.
Code Review
No issues found. Checked for bugs and CLAUDE.md compliance.
Contributor
Author
|
Closing this PR - it's too large and has many unaddressed review comments. I apologize for the noise; I opened too many PRs at once. Will be more thoughtful with smaller, focused contributions going forward. |
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
node:testargumenttoftestis undefined #26170t.after(),t.before(),t.beforeEach(), andt.afterEach()callbacks were not receiving the TestContext as their first argumentTest plan
test/regression/issue/26170.test.ts🤖 Generated with Claude Code