Repository navigation
Conversation
|
Status: ready for review. Reproduction
With this branch, 22 pass. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 seconds. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe ChangesShell output bytes API
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely identifies the main change: declaring bytes() on ShellPromise in bun-types. |
| Description check | |
The description explains the problem, fix, scope, limitations, and verification results. It does not use the template headings exactly, but it contains the required information for what the PR does an… |
Full details: Description check
-
Explanation
The description explains the problem, fix, scope, limitations, and verification results. It does not use the template headings exactly, but it contains the required information for what the PR does and how it was verified.
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
ee5e016 to
ab53c08
Compare
|
Updated 8:40 PM PT - Oct 16th, 2026
⏳ @robobun, your commit 54481b6 is still building in
|
ab53c08 to
0f2322c
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I also checked the new declaration against the runtime in src/js/builtins/shell.ts: bytes() wraps arrayBuffer() in new Uint8Array(x), so Promise<Uint8Array<ArrayBuffer>> matches, and the example byte counts (6 for hello\n, 13 for Hello World!\n) are correct. The fixture assertion sits below both pinned-diagnostic lines (196, 322), so it does not shift them.
Extended reasoning...
The change adds a bytes() declaration to ShellPromise in packages/bun-types/shell.d.ts, a docs section in docs/runtime/shell.mdx, and one type-level fixture assertion; no runtime or security-sensitive code is touched. The declaration mirrors the existing runtime method, and the ruled-out note records that verification. packages/bun-types/ is covered by a CODEOWNER entry, and the inline finding notes a stale as any cast the description claims was removed, so a maintainer look remains appropriate rather than an automated approval.
The runtime has had ShellPromise.prototype.bytes() since #11104. The types declared bytes() on ShellOutput and ShellError only, so `await $`cmd`.bytes()` was a TS2339 error. Declare it, document it beside .blob() in the shell docs, and add a type check to the bun-types fixture.
0f2322c to
54481b6
Compare
This branch has not been deployed
1 participant
Problem
await $`echo hello`.bytes()works at runtime. TypeScript rejects it:error TS2339: Property 'bytes' does not exist on type 'ShellPromise'.bytes()toShellPromise,ShellOutputandShellErrorinsrc/js/builtins/shell.ts.packages/bun-types/shell.d.tsdeclares it onShellOutputandShellErroronly.Fix
bytes(): Promise<Uint8Array<ArrayBuffer>>onShellPromise, betweenarrayBuffer()andblob()as in the runtime class..bytes()section todocs/runtime/shell.mdx. Each printed output, there and in the JSDoc, comes from a run of the snippet.test/integration/bun-types/bun-types.test.ts, 22 pass. With the new fixture line and theshell.d.tsof main, 10 of 22 fail with that TS2339. The fivetscchecks oflint.ymlpass.Background
$`cmd`returns aShellPromise. The command starts onawait. An output method such as.text()calls.quiet(), starts the command and resolves with stdout.bun-types.test.tstype-checkstest/integration/bun-types/fixture/. It pins two expected diagnostics toindex.ts:196andindex.ts:322, so the new check is below both.test/#44614 cast the one caller,test/js/bun/shell/bunshell-instance.test.ts:99, toany. After this PR the cast is not necessary. This PR keeps it, so that test does not change.shell.d.tsdeclaresShellPromise.Downsides
$`cmd`.run()is still a type error. Notes lists it with six other mismatches that this PR does not change.bun-typespackage grows by 711 bytes: 412 inshell.d.ts, 299 in its copy of the docs page. The bun binary does not change.Notes
Not in this PR
ShellPromise.prototype.run()exists at runtime and has no declaration. It starts the command and returns the promise. Afterrun(),text(),json(),lines(),arrayBuffer(),bytes()andblob()reject withShell is already running. The docs and the types have never shown it (first shell PR: feat: Bun shell #7748). The repo has one caller,test/js/bun/shell/commands/rm.test.ts:273, with a cast. A maintainer must decide ifrun()is public API, so this PR does not declare it.get stdin(): WritableStreamis declared onShellPromise. The runtime has nostdin.new $.Shell()has the type$. The instance has nobraces,escape,Shell,ShellPromiseorShellError.ShellPromiseandShellErrorhave the constructor types ofPromiseandError. The runtime constructors take other arguments.ShellError#infoandShellPromise#cwd()with no argument work at runtime and have no declaration.$(Bun Shell) do not match the runtime #44724 for this list. A maintainer closed it, because GitHub issues are for problems that a user ran into. This list is now only here.The cast from #44614
test/#44614 merged with($`echo hello` as any).bytes()atbunshell-instance.test.ts:99. It listed "$.ShellPromisehas nobytes()" under the gaps it left, and it chose casts so that a fix of the types breaks nothing.bun build --no-bundle --target=bungives the same 4,776 bytes for the test with and without it. It can go in a later cleanup.tsc7.0.2 intest/reportsjs/bun/shell/bunshell-instance.test.ts(99,30): error TS2339: Property 'bytes' does not exist on type 'ShellPromise'.with theshell.d.tsof main, and no error in that file with this branch.test/also reads the copy of bun-types thatbun install --cwd testputs intest/node_modules. For the run with theshell.d.tsof main, that file was in both places.Docs
.arrayBuffer()is also not on the page. This PR leaves it off.<Buffer 48 65 6c 6c 6f 21 0a>, line 71 prints<Buffer >, and line 455 printsBlob (13 bytes). docs, bun-types: correct 20 examples that do not run as printed #44480 corrects examples of this kind and does not touch this page.Fixture line
index.ts:322but not at the end of the file. The branchrobobun/4462621c/shell-inherit-stdioappends its checks at the end, andgit merge-treereports a conflict for a second append there. This position merges cleanly with that branch.Probes (bun 1.4.3-canary 367d939, release build.
src/js/builtins/shell.tshas the same JavaScript on main.)ShellPromiseprototype:arrayBuffer, blob, bytes, constructor, cwd, env, json, lines, nothrow, quiet, run, text, then, throws.bytes()resolves to a plainUint8Array. Itsbufferis anArrayBuffer. It equalsstdoutfor nine command shapes, which include a builtin, an external command, a pipeline, empty output, and 108 KB of output.Also run
tsc --noEmit -pforscripts/build,scripts,src/js,srcandsrc/runtime/bake: no output.prettier --checkonshell.d.tsandshell.mdx.Self-review
test/#44614 note, a list of what is not in this PR, no change here to the three wrong outputs, and a statement about.arrayBuffer().no test proof · iteration 0 · no src or test change; test-proof not applicable