fix(js-exec): honor encoding arg in Buffer.from / toString / write / byteLength#1
fix(js-exec): honor encoding arg in Buffer.from / toString / write / byteLength#1Hazzng wants to merge 6 commits into
Conversation
…byteLength Add pure-JS encoders/decoders for base64, base64url, hex, latin1/binary, ascii, and utf16le inside BUFFER_MODULE_SOURCE. Dispatch on a normalized encoding string in Buffer.from, Buffer.prototype.toString, Buffer.prototype.write, and Buffer.byteLength so the QuickJS shim matches real Node.js Buffer semantics for every encoding Node supports. - _normEnc: normalizes encoding aliases (binary→latin1, ucs2→utf16le, etc.) - _hexEncode/_hexDecode: lenient hex (stops at first invalid char) - _latin1Encode/_latin1Decode, _asciiEncode/_asciiDecode - _utf16leEncode/_utf16leDecode - _b64Encode/_b64UrlEncode/_b64Decode: forgiving base64 (ignores whitespace, tolerates missing padding, supports base64url alphabet) Adds js-exec.buffer-encoding.test.ts with 29 cases covering encode-only, decode-only, round-trips, binary bytes (0xff/0xfe/0x00), alias names, and forgiving inputs. Adds a guardrail comment above _normEnc explaining why round-trip tests alone would mask a broken encoding implementation.
|
Warning Review limit reached
More reviews will be available in 48 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds multi-encoding support to the QuickJS Buffer shim: encoding normalization and helpers, encoding-aware Buffer.from, Buffer.byteLength, Buffer.prototype.toString, and Buffer.prototype.write, plus a comprehensive Vitest suite and a changeset note. ChangesMulti-encoding Buffer support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@greptile review |
Greptile SummaryThis PR fixes the
Confidence Score: 4/5The encoding logic is correct across all seven encodings; the only gap is the absence of formal comparison tests required by team guidelines. The implementation correctly handles all encoding types, including edge cases like base64 whitespace stripping, odd-length hex truncation, and surrogate-pair utf16le encoding. The _b64UrlEncode template-literal double-backslash escaping is intentional and correct. The one gap is that AGENTS.md requires comparison tests for major command functionality, and none are present. No files have correctness issues; js-exec.buffer-encoding.test.ts is the only file that could benefit from a companion comparison test. Important Files Changed
Reviews (1): Last reviewed commit: "fix(js-exec): honor encoding arg in Buff..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { describe, expect, it } from "vitest"; | ||
| import { Bash } from "../../Bash.js"; | ||
|
|
||
| describe("buffer encoding", () => { | ||
| // ─── Encode side ───────────────────────────────────────────────── | ||
|
|
||
| it("Buffer.from(str).toString('base64')", async () => { | ||
| const env = new Bash({ javascript: true }); | ||
| const result = await env.exec( | ||
| `js-exec -c "console.log(Buffer.from('hello').toString('base64'))"`, |
There was a problem hiding this comment.
AGENTS.md requires comparison tests for major command functionality alongside unit tests. Fixing all Buffer encoding methods (from, toString, byteLength, write) is a significant behavioral change that warrants formal comparison tests against Node.js rather than ad-hoc node -e spot-checks. Without a comparison-tests file here, future regressions won't be caught by the comparison suite, and the manual spot-checks (documented only in the PR description) won't persist as executable evidence of correctness.
Context Used: packages/just-bash/AGENTS.md (source)
Greptile SummaryThis PR fixes the
Confidence Score: 4/5Safe to merge; encoding logic is correct and well-tested with only minor deviations from Node.js edge-case behaviour. The codec implementations are correct end-to-end — binary-byte round-trips, alias resolution, lenient base64 padding/whitespace, and hex stop-on-invalid all match Node.js. The two noted issues are narrow: null passed as encoding silently defaults to UTF-8 instead of throwing, and Buffer.byteLength for base64 allocates an unnecessary array. Neither affects the fixed functionality or existing tests. The codec helpers in module-shims.ts — specifically _normEnc around the null branch and the Buffer.byteLength base64 path. Important Files Changed
Reviews (2): Last reviewed commit: "fix(js-exec): honor encoding arg in Buff..." | Re-trigger Greptile |
Replace _b64Decode(value).length with a length-based formula (strip non-alphabet + padding, then floor(len * 3 / 4)). Matches Node's Buffer.byteLength semantics and avoids allocating an array just to count bytes.
|
Triaged the bot reviews: Greptile Issue 1 — Node accepts Greptile Issue 2 — Greptile review #1 — missing comparison tests: Not applicable. Comparison tests in this repo diff just-bash command output against recorded real-bash fixtures. This change is internal JS CodeRabbit: No actionable comments. |
…m correctness - Replace _latin1Encode/_asciiEncode with shared _rawEncode (both truncate to low byte; encode semantics are identical) - Fix Buffer.from(ArrayBuffer, byteOffset, length) to respect offset and length args - Fix Buffer.byteLength to throw TypeError for non-string/non-Buffer input instead of returning 0 - Fix Buffer.byteLength base64 regex to correctly handle mid-string '=' as terminator - Fix Buffer.prototype.toString to clamp negative start to 0, matching Node behavior - Fix Buffer.prototype.write to throw RangeError for negative/out-of-range offset
…n, toString negative end
- Buffer.from(ArrayBuffer, offset, length) now shares the backing store as Node does
(previously the inner Uint8Array was copied through the Buffer constructor).
- Buffer.prototype.write throws RangeError for negative length or length > remaining,
matching Node's ERR_OUT_OF_RANGE rather than silently clamping.
- Buffer.prototype.toString clamps negative end to 0 (Node behavior), so
Buffer.from("abc").toString("utf8", 0, -1) returns "" instead of "ab".
Adds edge-case tests covering all three behaviors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.changeset/clever-pumas-drum.md:
- Line 5: Add a top-level H1 heading to the changeset file to satisfy
markdownlint MD041: open .changeset/clever-pumas-drum.md and insert a
single-line H1 (e.g., "# js-exec: fix Buffer shim correctness") at the very top
before the existing body so the file begins with an H1 heading rather than plain
text; ensure the heading accurately reflects the existing diff summary and
preserves the rest of the file content.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 329150a9-55ce-469d-9456-7588f7dc9cfd
📒 Files selected for processing (3)
.changeset/clever-pumas-drum.mdpackages/just-bash/src/commands/js-exec/js-exec.buffer-encoding.test.tspackages/just-bash/src/commands/js-exec/module-shims.ts
| "just-bash": patch | ||
| --- | ||
|
|
||
| js-exec: fix Buffer shim correctness — ascii encode now uses & 0xff (not & 0x7f), consolidate latin1/ascii into shared \_rawEncode, fix Buffer.from(ArrayBuffer, offset, length), throw on invalid byteLength input, clamp negative toString start, throw RangeError for out-of-range write offset |
There was a problem hiding this comment.
Add a top-level heading to satisfy markdownlint MD041.
This file currently starts body content without an H1, which triggers the reported lint warning.
Suggested patch
---
"just-bash": patch
---
+# js-exec Buffer shim correctness fixes
+
js-exec: fix Buffer shim correctness — ascii encode now uses & 0xff (not & 0x7f), consolidate latin1/ascii into shared \_rawEncode, fix Buffer.from(ArrayBuffer, offset, length), throw on invalid byteLength input, clamp negative toString start, throw RangeError for out-of-range write offset📝 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.
| js-exec: fix Buffer shim correctness — ascii encode now uses & 0xff (not & 0x7f), consolidate latin1/ascii into shared \_rawEncode, fix Buffer.from(ArrayBuffer, offset, length), throw on invalid byteLength input, clamp negative toString start, throw RangeError for out-of-range write offset | |
| --- | |
| "just-bash": patch | |
| --- | |
| # js-exec Buffer shim correctness fixes | |
| js-exec: fix Buffer shim correctness — ascii encode now uses & 0xff (not & 0x7f), consolidate latin1/ascii into shared \_rawEncode, fix Buffer.from(ArrayBuffer, offset, length), throw on invalid byteLength input, clamp negative toString start, throw RangeError for out-of-range write offset |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 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 @.changeset/clever-pumas-drum.md at line 5, Add a top-level H1 heading to the
changeset file to satisfy markdownlint MD041: open
.changeset/clever-pumas-drum.md and insert a single-line H1 (e.g., "# js-exec:
fix Buffer shim correctness") at the very top before the existing body so the
file begins with an H1 heading rather than plain text; ensure the heading
accurately reflects the existing diff summary and preserves the rest of the file
content.
…writes to remaining
Per vercel bot review: Node validates the `length` arg against the buffer's
total size, not the remaining space after `offset`, and clamps actual writes
to the remaining space. The previous fix validated against remaining, which
incorrectly threw for cases like `Buffer.alloc(5).write('abcde', 3, 5)` that
Node accepts (writing 2 bytes).
Adds a regression test for the offset+length clamping case.
1 participant
Problem
The
Buffershim inside the QuickJS-backedjs-execruntime ignored theencodingargument on every encoding-aware method. All calls silently fell through to UTF-8, sobase64,hex,latin1,ascii, andutf16lewere broken. Encode + decode tests masked this because both sides were no-ops.Smoking gun:
Buffer.from([0xff,0xfe,0x00]).toString('base64')returned""instead of"//4A".Failing cases (before fix)
Run inside
js-exec(QuickJS), compared against real Node:Buffer.from([0xff,0xfe,0x00]).toString('base64')"""//4A"Buffer.from([0xff,0xfe,0x00]).toString('hex')"""fffe00"Buffer.from('//4A', 'base64')<Buffer 2f 2f 34 41>(raw utf8 of the string)<Buffer ff fe 00>Buffer.from('68656c6c6f', 'hex').toString()"68656c6c6f""hello"Buffer.byteLength('aGVsbG8=', 'base64')8(utf8 length of input string)5Buffer.from('hello', 'latin1').toString('latin1')0x80–0xff)Root cause: every entry point built the byte array via
_utf8Encode(str)regardless of theencodingargument;toStringsymmetrically decoded as UTF-8.Solution
Added pure-JS encode/decode helpers inside
BUFFER_MODULE_SOURCE(the IIFE that runs in QuickJS whereTextEncoder/atob/btoaare unavailable), and wired them into the four entry points:Buffer.from(str, encoding)buf.toString(encoding)Buffer.byteLength(str, encoding)buf.write(str, offset, length, encoding)Encodings supported:
utf8,base64,base64url,hex,latin1/binary,ascii,utf16le/ucs2. Node's lenient parsing is matched (base64 ignores whitespace + missing padding; hex stops at first invalid char). Unknown encodings throwTypeError.After fix — verified against
node -eByte-for-byte match with
node -erunning the same snippet.Tests
New file
js-exec.buffer-encoding.test.ts— 29 cases:0xff,0xfe,0x00) — these were the broken pathbinary,ucs2,utf-8)Existing
js-exec.node-compat.test.ts(120 tests) stays green — no regressions.Verification
Manual spot-checks against
node -econfirmed byte-for-byte match foraGVsbG8=,hello,//4A,68656c6c6f,ff00ab, andbyteLength= 5.Summary by CodeRabbit
New Features
Bug Fixes
Tests