optimize JIT and web response paths - #44
Open
theMackabu wants to merge 40 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThe pull request updates runtime allocation, Web APIs, UTF handling, GC policy, Silver bytecode and JIT execution. It adds framework examples, benchmark workloads, harness support, regression tests, and benchmark documentation. ChangesRuntime, Web APIs, and encoding
Silver inline caches and JIT
GC telemetry and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/modules/response.c (1)
85-86: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider using
js_native_data_allocindata_dup()for consistency withdata_new().
data_new()usesjs_native_data_alloc()(arena-backed) whiledata_dup()usescalloc(). Both are correctly freed byjs_native_data_free(), but usingcallocbypasses the arena optimization for cloned responses. If arena allocation is intended for allresponse_data_t,data_dup()should usejs_native_data_allocas well.♻️ Proposed refactor
static response_data_t *data_dup(const response_data_t *src) { - response_data_t *d = calloc(1, sizeof(response_data_t)); + response_data_t *d = js_native_data_alloc(rt->js, sizeof(response_data_t)); if (!d) return NULL;🤖 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/modules/response.c` around lines 85 - 86, Update data_dup() to allocate response_data_t with js_native_data_alloc(), matching data_new(), instead of calloc(); preserve the existing initialization and js_native_data_free() cleanup behavior.tests/test_response_constructor_fast_path.cjs (1)
65-76: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding
Object.prototypecleanup with try/finally.If the
Responseconstructor throws between setting and deletingObject.prototype.status/statusText, the prototype remains polluted for subsequent tests. Atry/finallyblock would ensure cleanup always runs.🛡️ Proposed fix
Object.prototype.status = 206; Object.prototype.statusText = "Partial Content"; -const inheritedLiteralResponse = new Response(null, { - headers: { "content-type": "text/plain" }, -}); -delete Object.prototype.status; -delete Object.prototype.statusText; +let inheritedLiteralResponse; +try { + inheritedLiteralResponse = new Response(null, { + headers: { "content-type": "text/plain" }, + }); +} finally { + delete Object.prototype.status; + delete Object.prototype.statusText; +} assert(inheritedLiteralResponse.status === 206, "literal init inherited status"); assert( inheritedLiteralResponse.statusText === "Partial Content", "literal init inherited statusText", );🤖 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/test_response_constructor_fast_path.cjs` around lines 65 - 76, Guard the temporary Object.prototype.status and statusText mutations in the inherited response test with a try/finally block: create inheritedLiteralResponse and perform its assertions in the try section, and always delete both prototype properties in finally, even if the Response constructor or assertions throw.
🤖 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 `@examples/npm/elysia/logger.ts`:
- Around line 5-16: Update the logger chain to use Elysia v2 hook APIs: replace
`.request()` with `.onRequest()`, `.afterHandle('global', ...)` with
`.onAfterHandle({ as: 'global' }, ...)`, and `.error('global', ...)` with
`.onError({ as: 'global' }, ...)`. Preserve the existing method filtering,
timing logic, `ctx.path`, and custom `ctx.start` field.
In `@src/modules/headers.c`:
- Around line 900-910: Update headers_data_append_if_missing to perform a
case-insensitive existence check by replacing the strcmp comparison in its
iteration over data->head with the existing ascii_case_equal helper, preserving
the early return when the header names match regardless of input casing.
- Around line 167-174: Update is_valid_value_n to allow obs-text bytes 0x80–0xff
in header values: remove the c > 127 rejection while continuing to reject NUL,
carriage return, and newline characters.
In `@src/silver/ops/property.h`:
- Around line 553-584: sv_get_elem_ic currently interns every string key before
caching, causing unbounded intern-table growth for unique runtime keys. Remove
the unconditional intern_string usage from this IC path and use the raw key
representation for lookup/cache validation, or otherwise ensure only bounded,
eligible keys are interned; preserve the existing fallback behavior and IC
guards.
- Around line 767-781: In the inline-cache property-add path, call
js_obj_ensure_prop_capacity() before retaining and assigning
ic->guard.add.to_shape; if allocation fails, return the OOM error without
modifying ptr->shape. Only swap the shape and release old_shape after capacity
is successfully ensured, then continue with ant_object_prop_set_unchecked and
cache updates.
In `@src/silver/swarm.c`:
- Around line 1417-1444: Add GC write-barrier calls before every direct value
store in the JIT put-field paths, including the inlined store in the generated
code around the overflow handling and jit_helper_put_field_transition_inobj in
glue.c. Reuse the existing barrier API and pass the target object and val with
the same semantics as sv_put_field_ic, ensuring both in-object and
overflow-property writes update the remembered set before storing.
---
Nitpick comments:
In `@src/modules/response.c`:
- Around line 85-86: Update data_dup() to allocate response_data_t with
js_native_data_alloc(), matching data_new(), instead of calloc(); preserve the
existing initialization and js_native_data_free() cleanup behavior.
In `@tests/test_response_constructor_fast_path.cjs`:
- Around line 65-76: Guard the temporary Object.prototype.status and statusText
mutations in the inherited response test with a try/finally block: create
inheritedLiteralResponse and perform its assertions in the try section, and
always delete both prototype properties in finally, even if the Response
constructor or assertions throw.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2904c354-7ef3-4262-9af9-8bca9420f515
📒 Files selected for processing (55)
examples/npm/elysia/ant.lockbexamples/npm/elysia/bench-no-server.tsexamples/npm/elysia/bench-server.tsexamples/npm/elysia/bench-stages.tsexamples/npm/elysia/logger.tsexamples/npm/elysia/package.jsonexamples/npm/elysia1/ant.lockbexamples/npm/elysia1/bench-no-server.tsexamples/npm/elysia1/bench-server.tsexamples/npm/elysia1/index.tsexamples/npm/elysia1/logger.tsexamples/npm/elysia1/package.jsonexamples/npm/hono/bench-server.tsinclude/ant.hinclude/arena.hinclude/common.hinclude/internal.hinclude/modules/headers.hinclude/modules/response.hinclude/object.hinclude/silver/engine.hinclude/silver/glue.hinclude/silver/opcode.hmeson/pgo/profiles/ant-darwin-aarch64.profdatasrc/ant.csrc/gc/objects.csrc/modules/builtin.csrc/modules/headers.csrc/modules/request.csrc/modules/response.csrc/silver/compiler.csrc/silver/engine.csrc/silver/glue.csrc/silver/ops/calls.hsrc/silver/ops/coercion.hsrc/silver/ops/comparison.hsrc/silver/ops/globals.hsrc/silver/ops/property.hsrc/silver/swarm.ctests/bench_context_construction.cjstests/bench_jit_constructor_shape.cjstests/bench_jit_import_named.mjstests/bench_jit_import_named_source.mjstests/bench_jit_object_literal.cjstests/bench_jit_string_calls.cjstests/bench_response_construction.cjstests/test_ctor_prop_feedback.cjstests/test_instanceof_ic_prototype_guard.cjstests/test_jit_accessor_ic.cjstests/test_jit_global_ic.cjstests/test_jit_object_literal_shape.cjstests/test_jit_string_call_intrinsics.cjstests/test_jit_string_proto_lookup.cjstests/test_request_cached_accessors.cjstests/test_response_constructor_fast_path.cjs
…sia-parity # Conflicts: # include/object.h # meson/pgo/profiles/ant-darwin-aarch64.profdata # src/ant.c # src/silver/compiler.c # src/silver/engine.c # src/silver/ops/globals.h
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/npm/elysia2/logger.ts (1)
5-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the v2 hook names here.
.request(),.afterHandle('global', ...), and.error('global', ...)don't match Elysia v2.0.0-exp.38; use.onRequest(),.onAfterHandle({ as: 'global' }, ...), and.onError({ as: 'global' }, ...).ctx.pathis valid, andctx.startcan stay as a custom field once the request hook is fixed.♻️ Proposed fix
export const logger = ({ methods = ['GET', 'PUT', 'POST', 'DELETE'] } = {}) => new Elysia() - .request(ctx => { + .onRequest(ctx => { if (!methods.includes(ctx.request.method)) return; ctx.start = performance.now(); console.log('<--', ctx.request.method, ctx.path); }) - .afterHandle('global', ctx => { + .onAfterHandle({ as: 'global' }, ctx => { if (!methods.includes(ctx.request.method)) return; console.log('-->', ctx.request.method, ctx.path, ctx.set.status ?? 200, 'in', Number((performance.now() - ctx.start).toFixed(2)), 'ms'); }) - .error('global', ctx => { + .onError({ as: 'global' }, ctx => { if (!methods.includes(ctx.request.method)) return; console.log('-->', ctx.request.method, ctx.path, ctx.set.status, 'in', ctx.start ? Number((performance.now() - ctx.start).toFixed(2)) : Number.NaN, 'ms'); });🤖 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 `@examples/npm/elysia2/logger.ts` around lines 5 - 16, Update the Elysia hook registrations in the logger chain: replace request with onRequest, afterHandle('global', ...) with onAfterHandle({ as: 'global' }, ...), and error('global', ...) with onError({ as: 'global' }, ...). Preserve the existing method filtering, logging, ctx.path usage, and custom ctx.start timing behavior.
🤖 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/harness/harness.js`:
- Around line 39-52: Update installDependencies to subscribe to child.stdout and
child.stderr streams using their data events when collecting output, rather than
listening for stdout/stderr events on the child process. Apply the same
stream-based handling in the Child implementation below, preserving the existing
output accumulation behavior.
---
Duplicate comments:
In `@examples/npm/elysia2/logger.ts`:
- Around line 5-16: Update the Elysia hook registrations in the logger chain:
replace request with onRequest, afterHandle('global', ...) with onAfterHandle({
as: 'global' }, ...), and error('global', ...) with onError({ as: 'global' },
...). Preserve the existing method filtering, logging, ctx.path usage, and
custom ctx.start timing behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1648026f-80b8-4cbf-bb40-fefc66337d56
📒 Files selected for processing (30)
examples/bench-v8/score.jsonexamples/npm/elysia2/ant.lockbexamples/npm/elysia2/bench-no-server.tsexamples/npm/elysia2/bench-server.tsexamples/npm/elysia2/bench-stages.tsexamples/npm/elysia2/index.tsexamples/npm/elysia2/logger.tsexamples/npm/elysia2/package.jsoninclude/common.hinclude/internal.hinclude/object.hinclude/silver/engine.hinclude/silver/glue.hinclude/silver/opcode.hmeson/pgo/profiles/ant-darwin-aarch64.profdatasrc/ant.csrc/gc/objects.csrc/silver/compiler.csrc/silver/engine.csrc/silver/glue.csrc/silver/ops/calls.hsrc/silver/ops/coercion.hsrc/silver/ops/globals.hsrc/silver/ops/property.hsrc/silver/swarm.ctests/harness/harness.jstests/harness/manifest.jstests/harness/run.jstests/harness/snapshots/rolldown.txttests/test_jit_global_ic.cjs
🚧 Files skipped from review as they are similar to previous changes (12)
- include/common.h
- src/silver/ops/globals.h
- src/silver/engine.c
- include/silver/engine.h
- include/internal.h
- src/gc/objects.c
- include/silver/opcode.h
- include/silver/glue.h
- src/silver/glue.c
- src/silver/ops/property.h
- src/ant.c
- src/silver/swarm.c
Owner
Author
|
@macroscope-app review |
|
Manual reviews triggered for commit All prior checks · these links stay valid even if you push more commits. |
|
Review triggered and in progress. Results will be posted as check runs when complete. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| NEXT(3); | ||
| } | ||
|
|
||
| L_CALL_STRING_INDEXOF: { |
There was a problem hiding this comment.
🟠 High silver/engine.c:1483
The L_CALL_STRING_INDEXOF and L_CALL_STRING_SUBSTRING fast paths pass call_args (a pointer into vm->stack) to js_string_indexof_call/js_string_substring_call. When call_this is a non-string object, those helpers coerce it via to_string_val, which can invoke user JavaScript; that nested execution can realloc vm->stack, invalidating the call_args pointer. The helper then reads stale memory for the arguments, causing a use-after-free, wrong arguments, or a crash. Consider copying the arguments into a local array before calling the helper, or falling back to sv_vm_call for non-string receivers.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/silver/engine.c around line 1483:
The `L_CALL_STRING_INDEXOF` and `L_CALL_STRING_SUBSTRING` fast paths pass `call_args` (a pointer into `vm->stack`) to `js_string_indexof_call`/`js_string_substring_call`. When `call_this` is a non-string object, those helpers coerce it via `to_string_val`, which can invoke user JavaScript; that nested execution can `realloc` `vm->stack`, invalidating the `call_args` pointer. The helper then reads stale memory for the arguments, causing a use-after-free, wrong arguments, or a crash. Consider copying the arguments into a local array before calling the helper, or falling back to `sv_vm_call` for non-string receivers.
ApprovabilityVerdict: Needs human review 4 blocking correctness issues found. This PR introduces substantial JIT optimizations and web API performance improvements with significant complexity. Unresolved high-severity review comments identify potential memory safety issues (use-after-free in string call fast paths) and a flag collision that could break constructor behavior. Human review is needed to verify correctness of these low-level changes. You can customize Macroscope's approvability policy. Learn more. |
unregister_buffer scanned the global registry linearly per freed buffer, making GC sweeps of dead buffers quadratic in live-buffer population; an intrusive registry_slot (index+1, 0 = never registered, verified before removal) makes it O(1) and keeps foreign wasm/wasi/clone buffers safe. Buffer.write now honors Node's (string[, offset[, length]][, encoding]) signature via per-encoding bounded encoders; byteLength is encoding-aware; utf8 writes never split a multibyte sequence.
Four unbraced HASH_ITER cleanups freed one entry after the loop (on NULL) instead of each entry, leaking a json_key_entry_t per object key per parse. path.relative treated '.' as a literal segment; both arguments now resolve against the working directory first, matching Node's relative(resolve(from), resolve(to)).
AES-GCM (128/192/256) and HMAC (block-size default or explicit bit length) generation, raw export gated on a native extractable flag, and extractable/usages recorded on CryptoKey objects. Fixes vite dev crashing on @vitejs/plugin-rsc's startup key generation.
ANT_DEBUG=gc:stats moves to src/gc/stats.c behind thin note hooks with a policy snapshot exporter. GC_POOL_PRESSURE_FLOOR drops 8MiB->1MiB (the floor only governs sub-MiB live pools, where majors cost ~2ms; at 8MiB a 1.4MiB-live pool grew ~7x between majors and slowed the mutator). Nursery growth cap 8x->2x per measurement; main-stack conservative scan is gated on a running coroutine; pool-pressure majors are cause-tagged. Adds gc_get_string_epoch (major-only) for string-pointer-keyed caches.
match.index, lastIndex (both directions), d-flag indices, replacer offsets, search results and split boundaries were pcre2 byte offsets; they now convert at the pcre2 boundary with an ASCII fast path and ascending-order conversions for the forward-resuming scan cache. Interpreted matches skip re-validating subjects already validated this major-GC string epoch (pcre2_jit_match never validates and never arms the cache); empty-match loops advance whole characters, never handing pcre2 a mid-character offset. Symbol.split gains a guarded scan-and-slice fast path over the cached compiled pattern (~7000x on 50KB subjects), exec lookups walk the prototype so overridden exec dispatches per spec and test() reaches its truthy-only branch, and no-JIT sv_tfb_* stubs restore -Djit=false.
…sia-parity json.c takes master's HASH_ITER fix (same bug, fixed independently on both sides). String.indexOf composes the branch ascii fast path with master's cached str_utf16_len clamp and memchr scan. OP_GET_GLOBAL adopts master's dedicated global-read IC fastpath wholesale. The branch's 16-param get-field IC emitter learns imp_accessor == NULL (accessor hits bail to slow) so master's inline-body call site works without accessor plumbing; func->filename follows master's move to func->debug->filename.
UTF-8 validity is a property of immutable string content, so it now lives in the flat-string meta word (2 spare bits beside the ascii state, ascii accessor masked accordingly) with a strict RFC 3629 validator that classifies WTF-8 lone-surrogate strings as invalid. regex_subject_match_options collapses to a flag check; the TLS validated-subject cache, mark-after-interpreted-match choreography, and the major-GC string epoch it was keyed on are all deleted. Valid subjects skip pcre2 UTF re-validation permanently instead of per-epoch; invalid ones never do.
| // pcre2_jit_match never validates UTF; it must not arm the cache | ||
| rc = pcre2_jit_match(compiled.code, (PCRE2_SPTR)str_ptr, str_len, (PCRE2_SIZE)q, match_options, compiled.match_data, match_ctx); | ||
| } else rc = pcre2_match(compiled.code, (PCRE2_SPTR)str_ptr, str_len, (PCRE2_SIZE)q, match_options, compiled.match_data, match_ctx); | ||
| if (rc < 0) break; |
There was a problem hiding this comment.
🟡 Medium modules/regex.c:2289
regexp_split_fast calls pcre2_match/pcre2_jit_match directly but never calls update_regexp_statics, so an eligible fast-path split like 'a,b'.split(/(,)/) leaves RegExp.$1, RegExp.lastMatch, RegExp.input, and the other legacy statics stale instead of reflecting the separator match. The normal split loop updates these via regexp_exec_abstract, so the fast path diverges in user-visible behavior when those statics are read after split. Consider calling update_regexp_statics with the match data after each successful match in regexp_split_fast.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/modules/regex.c around line 2289:
`regexp_split_fast` calls `pcre2_match`/`pcre2_jit_match` directly but never calls `update_regexp_statics`, so an eligible fast-path split like `'a,b'.split(/(,)/)` leaves `RegExp.$1`, `RegExp.lastMatch`, `RegExp.input`, and the other legacy statics stale instead of reflecting the separator match. The normal split loop updates these via `regexp_exec_abstract`, so the fast path diverges in user-visible behavior when those statics are read after `split`. Consider calling `update_regexp_statics` with the match data after each successful match in `regexp_split_fast`.
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/ant.c (1)
12473-12476: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCoerce
String.prototype.indexOfsearch arguments before string comparison.
indexOf’s search argument is subject toToString, so"".indexOf()and"".indexOf(1)should search"undefined"and"1". The currentindexOffast path returns-1for omitted or non-string searches (undefined,null, numbers) instead of using the string-coerced search value.Proposed fix
- if (nargs == 0) return tov(-1); - - ant_value_t search = args[0]; - if (vtype(search) != T_STR) return tov(-1); + ant_value_t search = nargs == 0 + ? js_mkstr(js, "undefined", 9) + : js_tostring_val(js, args[0]); + if (is_err(search)) return search;🤖 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/ant.c` around lines 12473 - 12476, Update the String.prototype.indexOf implementation around the nargs and vtype(search) checks to apply ToString coercion to the first search argument, including omitted arguments as undefined, before performing string comparison. Preserve the existing indexOf behavior and return values after coercion, so null, numbers, and other values are searched using their string representations.
🧹 Nitpick comments (1)
meson/pgo/profiles/ant-darwin-aarch64.profdata (1)
1-3448: 🧹 Nitpick | 🔵 TrivialBinary PGO profile — nothing to line-review; flagging repo-hygiene considerations instead.
This file is a binary LLVM indexed-profile artifact (PGO data for
anton darwin-aarch64), not source code — the "lines" here are arbitrary byte-chunk boundaries injected by the diff tool, so there's no logic to inspect. A few process-level points worth confirming for this class of change:
- Regenerated PGO profiles are opaque, full-file binary diffs every time; consider documenting (or automating via CI) how/when this file is regenerated so reviewers/maintainers know it's reproducible from a known compiler + source revision rather than hand-edited.
- PGO profiles are sensitive to compiler/toolchain version skew — worth confirming the build fails loudly (rather than silently ignoring or degrading) if
ant-darwin-aarch64.profdatadoesn't match the Clang/LLVM version used to consume it inmeson/pgo.- As more platforms are supported, per-arch binary blobs like this will accumulate in the main tree; consider whether these belong in Git LFS or a release-asset/artifact store instead of the primary source repo.
🤖 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 `@meson/pgo/profiles/ant-darwin-aarch64.profdata` around lines 1 - 3448, Document how and when ant-darwin-aarch64.profdata is regenerated, including the expected compiler and source revision, and ensure the meson/pgo consumption path validates toolchain/profile compatibility and fails clearly on mismatch. Also establish the repository policy for storing growing per-architecture binary PGO profiles, including whether Git LFS or an artifact store is required.
🤖 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 `@docs/exec-plans/active/gc-and-server-benchmark-protocol.md`:
- Around line 151-158: Update both termination blocks in
docs/exec-plans/active/gc-and-server-benchmark-protocol.md (lines 151-158 and
195-203) so they terminate the Ant child or dedicated benchmark process group
rather than only the /usr/bin/time wrapper; capture the relevant child/group
identifier, signal it with the existing TERM/KILL sequence, then wait for the
wrapper without orphaning the runtime.
In `@src/modules/buffer.c`:
- Around line 3307-3308: Update js_buffer_write to validate or coerce args[0]
before calling js_getstr, ensuring buffer_encode_into never receives a NULL
string. Use js_tostring_val for the required string conversion, or reject
non-string values consistently with js_buffer_copy, while preserving normal
string writes.
In `@src/modules/crypto.c`:
- Around line 933-936: Update crypto_subtle_generate_key_impl() so both AES and
HMAC generation branches call OPENSSL_cleanse(buf, len) after
crypto_make_key_object() has consumed the random bytes and before returning.
Preserve the existing key-object result and error handling while ensuring each
generated buffer is cleared on the return path.
In `@tests/test_buffer_registry_slots.cjs`:
- Around line 68-73: Update the drift assertion in the external buffer
accounting test to require the absolute value of drift to remain below 4 * 1024
* 1024, rejecting both positive overcounting and negative underflow while
preserving the existing diagnostic message.
In `@tests/test_regex_utf16_positions.cjs`:
- Around line 55-64: Update the WTF-8 global regex assertions in the test block
to explicitly verify that the first g.exec(wtf) finds the expected X match
before asserting the subsequent exec returns null. Keep the existing sticky
y.test(wtf) assertion unchanged.
- Around line 66-72: Update the astral-character expectation in the “empty-match
match loop astral” test to preserve non-Unicode RegExp behavior: /x*/g must
advance by UTF-16 code units and include the mid-surrogate position, producing
three empty matches for “😀”. Keep the Unicode and split assertions unchanged.
In `@tests/test_webcrypto_generate_export.cjs`:
- Around line 32-37: Add a nonzero-byte assertion for the first exported key
buffer `raw`, alongside the existing checks for `raw2`, before comparing the two
generated keys. Preserve the current `raw2` and key-difference assertions.
---
Outside diff comments:
In `@src/ant.c`:
- Around line 12473-12476: Update the String.prototype.indexOf implementation
around the nargs and vtype(search) checks to apply ToString coercion to the
first search argument, including omitted arguments as undefined, before
performing string comparison. Preserve the existing indexOf behavior and return
values after coercion, so null, numbers, and other values are searched using
their string representations.
---
Nitpick comments:
In `@meson/pgo/profiles/ant-darwin-aarch64.profdata`:
- Around line 1-3448: Document how and when ant-darwin-aarch64.profdata is
regenerated, including the expected compiler and source revision, and ensure the
meson/pgo consumption path validates toolchain/profile compatibility and fails
clearly on mismatch. Also establish the repository policy for storing growing
per-architecture binary PGO profiles, including whether Git LFS or an artifact
store is required.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c2c8cad-f4bc-4a15-978e-5c91af0246ef
📒 Files selected for processing (41)
docs/exec-plans/active/README.mddocs/exec-plans/active/gc-and-server-benchmark-protocol.mddocs/exec-plans/completed/README.mddocs/exec-plans/completed/jit-put-field-gc-performance.mdinclude/gc.hinclude/gc/stats.hinclude/internal.hinclude/modules/buffer.hinclude/modules/regex.hinclude/silver/engine.hinclude/silver/glue.hinclude/silver/opcode.hinclude/utf8.hmeson/pgo/profiles/ant-darwin-aarch64.profdatasrc/ant.csrc/gc/gc.csrc/gc/objects.csrc/gc/stats.csrc/main.csrc/modules/buffer.csrc/modules/builtin.csrc/modules/crypto.csrc/modules/fetch.csrc/modules/path.csrc/modules/regex.csrc/modules/request.csrc/modules/response.csrc/pool.csrc/silver/compiler.csrc/silver/engine.csrc/silver/glue.csrc/silver/ops/coercion.hsrc/silver/ops/globals.hsrc/silver/ops/property.hsrc/silver/swarm.csrc/utf8.ctests/harness/manifest.jstests/test_buffer_registry_slots.cjstests/test_path_relative_resolve.cjstests/test_regex_utf16_positions.cjstests/test_webcrypto_generate_export.cjs
🚧 Files skipped from review as they are similar to previous changes (12)
- src/modules/fetch.c
- src/modules/builtin.c
- src/silver/ops/coercion.h
- src/silver/ops/globals.h
- include/silver/glue.h
- src/modules/request.c
- tests/harness/manifest.js
- include/silver/opcode.h
- src/silver/ops/property.h
- src/modules/response.c
- src/silver/glue.c
- src/silver/swarm.c
…st paths to_string_val on an object receiver (or a proxy length trap in the includes generic path) can run user JS that reallocates vm->stack, leaving the helpers' args pointer stale. Read the at-most-two used argument values into locals before any coercion; the values stay rooted by their stack slots. Also propagate a throwing toString instead of masking it as a non-string error.
the branch-target reset block cleared slot_type, known_func, and has_const but left obj_site, so a stale object-literal site pointer could survive a control-flow merge and let OP_DEFINE_FIELD update the wrong site's shared_shape. Clear it with the rest of the speculative per-slot state; bench_jit_object_literal measures at parity.
the interpreter's sv_put_field_ic calls regexp_note_property_write before every store, but the JIT put-field IC fastpath stored inline, so a hot 'obj.exec = fn' never invalidated the regex fast paths. Gate the fastpath at compile time on a shared watched-name predicate; the define-field path is unwatched in the interpreter too, so it stays ungated.
the scan-and-slice split loop called pcre2 directly and never updated RegExp.$1/lastMatch, diverging from the exec-based slow path. Update statics once per successful separator match, including empty matches.
write, Buffer.from, byteLength, and toString all treated latin1 and ascii as UTF-8 passthrough, so 'é' wrote c3 a9 instead of e9 and round-trips broke. Encode one byte per UTF-16 code unit (unit & 0xff), report byteLength as the unit count, and decode bytes as U+00xx with the ascii high-bit strip, matching Node on the full surface.
@@replace passed a custom exec's raw index value to the replacer while slicing with a separately clamped byte position, so callbacks could see 1.5, out-of-range, or uncoerced values. Apply the spec conversion: ToIntegerOrInfinity then clamp to the subject's utf16 length.
subtle.generateKey silently defaulted extractable and keyUsages when called with fewer than the three required arguments; reject with a TypeError like importKey does. crypto_make_key_object stored the caller's usages array by reference, so mutating it after creation changed the key's usage set; copy it at creation.
list_append_raw discarded list_append_parts' failure, so headers_data_copy could return a silently partial copy on OOM and response cloning would drop headers; return NULL instead (data_dup already checks). The set, copy-from, and headers-init paths propagate the failure too. Response.redirect's ensure-headers error exit now frees href and clears the parsed url like its sibling exits.
path_absolutize treated any nonzero root length as fully qualified, so path.win32.relative compared C:foo and \foo unresolved. Absolutize now mirrors resolve(): drive-relative inputs keep their drive and join the cwd, rooted inputs are already handled by the absolute check. Also match Node's quirk where a device-less root with no common component returns the resolved target instead of a dot-dot walk.
every read-path lookup (elem IC, lkp, lkp_val, getter/setter walks, lkp_proto, lookup_prop_meta, ctor-proto) interned the probed key, so reading unique runtime keys grew the never-evicted table without bound (200k unique probes leaked ~9.3MB). Add a find-only intern_string_existing and use it on all read paths: a key that can hit was interned when its property was defined, so absence from the table proves absence from every shape. Writes still intern at creation.
zero the stack copies of fresh AES/HMAC key material once the key object owns them; make the protocol doc's stress10 snippets kill the ant child they warn about; assert absolute drift in the buffer registry test; assert the WTF-8 first match explicitly instead of a vacuous null-chain; check the first generated key's material and pin the generateKey arity and usages-snapshot behavior.
buf.write and Buffer.from copied the runtime's WTF-8 string bytes verbatim, so a lone surrogate produced invalid UTF-8 (ED A0 80) where Node emits U+FFFD. Share the same-length in-place patch te_encode already used as utf8_replace_wtf8_surrogates, gated on the memoized validity flag; te_encode adopts the gate too, turning TextEncoder's byte-wise copy loop into a memcpy fast path for valid strings.
| test_name="${test_file##*/}" | ||
| stdout="$RESULTS/gc-$label-$test_name.out" | ||
| timing="$RESULTS/gc-$label-$test_name.time" | ||
| if [ "$test_file" = tests/test_gc_stress10.js ]; then |
There was a problem hiding this comment.
🟡 Medium active/gc-and-server-benchmark-protocol.md:151
The special case for tests/test_gc_stress10.js always sleeps 20 seconds and records EXPECTED_TIMEOUT regardless of whether the process was still running at that point. If the runtime crashes or exits early with a non-zero status, the suite still reports EXPECTED_TIMEOUT instead of the real failure, masking regressions. The loop should check whether the process is still alive before the timeout and inspect wait's exit status so crashes and early exits are recorded as failures.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @docs/exec-plans/active/gc-and-server-benchmark-protocol.md around line 151:
The special case for `tests/test_gc_stress10.js` always sleeps 20 seconds and records `EXPECTED_TIMEOUT` regardless of whether the process was still running at that point. If the runtime crashes or exits early with a non-zero status, the suite still reports `EXPECTED_TIMEOUT` instead of the real failure, masking regressions. The loop should check whether the process is still alive before the timeout and inspect `wait`'s exit status so crashes and early exits are recorded as failures.
| // fast path: unmodified builtin regexp with default species and a | ||
| // numeric (or absent) limit — bypasses splitter construction and the | ||
| // per-position sticky probing entirely | ||
| if (regexp_can_use_internal_fast_path(js, rx) && |
There was a problem hiding this comment.
🟡 Medium modules/regex.c:2393
The fast path in builtin_regexp_symbol_split calls regexp_split_fast using rx's internal compiled flags instead of the observed flags_str that the spec-mandated species splitter would use. When an ordinary RegExp has an own flags property (e.g. /a/ with flags overridden to "i"), regexp_can_use_internal_fast_path still returns true because it only inspects internal slots, so the fast path is taken. Splitting "A" with such a regexp returns ["A"] instead of [] because regexp_split_fast matches with the original case-sensitive flags. Invalid overridden flag strings also bypass the constructor error that the species path would raise. Consider guarding the fast path on flags_str matching rx's internal flags, or compiling with flags_str.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/modules/regex.c around line 2393:
The fast path in `builtin_regexp_symbol_split` calls `regexp_split_fast` using `rx`'s internal compiled flags instead of the observed `flags_str` that the spec-mandated species splitter would use. When an ordinary `RegExp` has an own `flags` property (e.g. `/a/` with `flags` overridden to `"i"`), `regexp_can_use_internal_fast_path` still returns true because it only inspects internal slots, so the fast path is taken. Splitting `"A"` with such a regexp returns `["A"]` instead of `[]` because `regexp_split_fast` matches with the original case-sensitive flags. Invalid overridden flag strings also bypass the constructor error that the species path would raise. Consider guarding the fast path on `flags_str` matching `rx`'s internal flags, or compiling with `flags_str`.
| ./build/ant examples/npm/elysia2/bench-server.ts >/tmp/elysia2-build.log 2>&1 & | ||
| server_pid=$! | ||
|
|
||
| until curl -fsS http://127.0.0.1:3000/ >/dev/null; do sleep 0.05; done |
There was a problem hiding this comment.
🟡 Medium active/gc-and-server-benchmark-protocol.md:261
The server readiness loop at line 261 never times out and never checks whether $server_pid is still alive, so if the server process exits during startup (e.g. the fixture fails to load or port 3000 cannot be bound) the benchmark hangs forever instead of recording a failed run. The RSC readiness loop at line 319 has the same defect. Consider adding a bounded timeout and checking kill -0 "$server_pid" each iteration so a crashed server is detected and reported.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @docs/exec-plans/active/gc-and-server-benchmark-protocol.md around line 261:
The server readiness loop at line 261 never times out and never checks whether `$server_pid` is still alive, so if the server process exits during startup (e.g. the fixture fails to load or port 3000 cannot be bound) the benchmark hangs forever instead of recording a failed run. The RSC readiness loop at line 319 has the same defect. Consider adding a bounded timeout and checking `kill -0 "$server_pid"` each iteration so a crashed server is detected and reported.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/harness/manifest.js (1)
47-53: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPreserve PID scrubbing for the rolldown snapshot.
Line 52 replaces the scrub list, so variable process IDs are no longer normalized. Add the cache rule alongside
scrubPidto avoid snapshot churn.Proposed fix
- scrub: [[/(?:\.\.\/)+\.ant\/pkg\/cache\/[0-9a-f]+/g, '<ant-cache>']] + scrub: [ + ...scrubPid, + [/(?:\.\.\/)+\.ant\/pkg\/cache\/[0-9a-f]+/g, '<ant-cache>'] + ]🤖 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/harness/manifest.js` around lines 47 - 53, Update the rolldown snapshot entry in the manifest so its scrub configuration preserves the existing scrubPid rule while also applying the ant cache-path normalization regex. Extend the scrub list rather than replacing the PID scrubbing behavior.
🤖 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 `@src/ant.c`:
- Around line 12490-12500: Restore standard coercion in the shared string fast
paths: at src/ant.c lines 12490-12525, update the indexOf path to apply ToString
to search and ToIntegerOrInfinity to pos_arg before clamping; at src/ant.c lines
12556-12578, apply ToIntegerOrInfinity to both substring bounds, clamp each with
min(..., D(str_utf16_len(js, str))), then swap them as needed. Use the existing
coercion helpers and preserve the argument snapshots before to_string_val.
In `@tests/test_jit_regexp_exec_override.cjs`:
- Around line 9-10: Update putReplace() and its related test setup to override
the object’s Symbol.replace method instead of the string-keyed replace property.
Make the custom implementation return an observable result, assert that result
through String.prototype.replace(), and restore originalReplace afterward.
---
Outside diff comments:
In `@tests/harness/manifest.js`:
- Around line 47-53: Update the rolldown snapshot entry in the manifest so its
scrub configuration preserves the existing scrubPid rule while also applying the
ant cache-path normalization regex. Extend the scrub list rather than replacing
the PID scrubbing behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7741a295-1c3d-4f99-8612-45007aea7340
📒 Files selected for processing (27)
docs/exec-plans/active/README.mddocs/exec-plans/active/gc-and-server-benchmark-protocol.mdexamples/spec/buffer.jsexamples/spec/regexp.jsinclude/internal.hinclude/modules/regex.hinclude/utf8.hsrc/ant.csrc/modules/buffer.csrc/modules/crypto.csrc/modules/headers.csrc/modules/path.csrc/modules/regex.csrc/modules/response.csrc/modules/textcodec.csrc/silver/ops/property.hsrc/silver/swarm.csrc/utf8.ctests/harness/manifest.jstests/test_buffer_registry_slots.cjstests/test_intern_table_bounded.cjstests/test_jit_define_field_join_sites.cjstests/test_jit_regexp_exec_override.cjstests/test_path_relative_resolve.cjstests/test_regex_utf16_positions.cjstests/test_string_call_stack_realloc.cjstests/test_webcrypto_generate_export.cjs
💤 Files with no reviewable changes (1)
- docs/exec-plans/active/README.md
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/test_path_relative_resolve.cjs
- tests/test_buffer_registry_slots.cjs
- tests/test_webcrypto_generate_export.cjs
- src/modules/crypto.c
- docs/exec-plans/active/gc-and-server-benchmark-protocol.md
- include/internal.h
- src/modules/response.c
- src/modules/regex.c
- src/silver/ops/property.h
- src/modules/headers.c
- src/silver/swarm.c
Comment on lines
+12490
to
12500
| // args may point into vm->stack; to_string_val can run user JS (object | ||
| // receiver with a toString) and realloc the stack, so snapshot first | ||
| ant_value_t search = nargs > 0 ? args[0] : js_mkundef(); | ||
| ant_value_t pos_arg = nargs > 1 ? args[1] : js_mkundef(); | ||
|
|
||
| ant_value_t str = to_string_val(js, this_val); | ||
| if (is_err(str)) return str; | ||
| if (vtype(str) != T_STR) return js_mkerr(js, "indexOf called on non-string"); | ||
| if (nargs == 0) return tov(-1); | ||
|
|
||
| ant_value_t search = args[0]; | ||
| if (vtype(search) != T_STR) return tov(-1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
console.log('undefined'.indexOf()); // 0
console.log('aa'.indexOf('a', '1')); // 1
console.log('abc'.substring('1', '2')); // b
NODERepository: theMackabu/ant
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '12460,12610p' src/ant.c
echo
echo "== search for fast-path helpers / to_integer helpers =="
rg -n "ToIntegerOrInfinity|to_integer|indexOf|substring|substring_call|string_indexof" src/ant.c src -g '*.c' -g '*.h' | head -200Repository: theMackabu/ant
Length of output: 10153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect glue/engine handling around fast paths =="
sed -n '630,680p' src/silver/glue.c
sed -n '1480,1535p' src/silver/engine.c
sed -n '3275,3305p' src/silver/compiler.c
echo
echo "== inspect integer coercion implementations and string constructor helpers =="
rg -n "ant_value_t js_(|to_integer|to_number|to_string|to_bigint|to_boolean)|ToInteger|ToIntegerOrInfinity|js_str_\\{|defvalue|defnumber|defstring|defundefined|js_mknum|js_mkstr" src/ant.c src -g '*.c' | head -300
sed -n '1,260p' src/ant.c
sed -n '260,520p' src/ant.cRepository: theMackabu/ant
Length of output: 43885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== coercion and string methods in ant.c =="
sed -n '2560,2665p' src/ant.c
sed -n '13660,13910p' src/ant.c
echo
echo "== test harness/build files =="
git ls-files | rg '(^|/)(Makefile|CMake|meson|test|package.json|compile|run)' | head -200
echo
echo "== small behavior probe via Node for JS spec semantics =="
node - <<'NODE'
const cases = [
[`'undefined'.indexOf()`, `'undefined'.indexOf()`],
[`'abc'.indexOf('b', '1')`, `'abc'.indexOf('b', '1')`],
[`'abc'.substring('1', '2')`, `'abc'.substring('1', '2')`],
[`'abc'.substring('2', '1')`, `'abc'.substring('2', '1')`],
[`'abc'.substring(NaN, Infinity)`, `'abc'.substring(NaN, Infinity)`],
[`'abc'.substring(-Infinity, -Infinity)`, `'abc'.substring(-Infinity, -Infinity)`],
];
for (const [label, expr] of cases) {
try {
console.log(expr, '=>', eval(expr));
} catch (e) {
console.log(expr, 'THROWS', e && e.name + ': ' + e.message);
}
}
NODERepository: theMackabu/ant
Length of output: 17339
Restore standard argument coercion in the shared string fast paths.
The fast paths correctly snapshot stack arguments before to_string_val(), but the subsequent branches skip standard coercion: indexOf rejects string/object search and non-numeric pos, while substring rejects string/bignum/boolean bounds and treats omitted/NaN as non-bounds instead of 0.
src/ant.c#L12490-L12525: coercesearchwithToStringand coercepos_argwithToIntegerOrInfinitybefore clamping.src/ant.c#L12557-L12581: coerce both substring bounds withToIntegerOrInfinity, then clamp withmin(..., D(str_utf16_len(js, str)))before swapping.
📍 Affects 1 file
src/ant.c#L12490-L12500(this comment)src/ant.c#L12556-L12578
🤖 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/ant.c` around lines 12490 - 12500, Restore standard coercion in the
shared string fast paths: at src/ant.c lines 12490-12525, update the indexOf
path to apply ToString to search and ToIntegerOrInfinity to pos_arg before
clamping; at src/ant.c lines 12556-12578, apply ToIntegerOrInfinity to both
substring bounds, clamp each with min(..., D(str_utf16_len(js, str))), then swap
them as needed. Use the existing coercion helpers and preserve the argument
snapshots before to_string_val.
Comment on lines
+9
to
+10
| function putExec(o, f) { o.exec = f; } | ||
| function putReplace(o, f) { o.replace = f; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const re = /-/;
RegExp.prototype.replace = undefined;
console.log('string-key:', 'a-b'.replace(re, '+')); // still a+b
const saved = RegExp.prototype[Symbol.replace];
RegExp.prototype[Symbol.replace] = () => 'overridden';
console.log('symbol-key:', 'a-b'.replace(re, '+')); // overridden
RegExp.prototype[Symbol.replace] = saved;
NODERepository: theMackabu/ant
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced test file around the relevant helpers and assertion.
if [ -f tests/test_jit_regexp_exec_override.cjs ]; then
nl -ba tests/test_jit_regexp_exec_override.cjs | sed -n '1,80p'
else
echo "tests/test_jit_regexp_exec_override.cjs not found"
fd -a 'test_jit_regexp_exec_override.cjs' .
fiRepository: theMackabu/ant
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD=$(pwd)"
echo "files:"
git ls-files | grep 'test_jit_regexp_exec_override\.cjs$' || true
if [ -f tests/test_jit_regexp_exec_override.cjs ]; then
awk '{ printf "%6d\t%s\n", NR, $0 }' tests/test_jit_regexp_exec_override.cjs | sed -n '1,90p'
else
echo "tests/test_jit_regexp_exec_override.cjs not found"
fi
node - <<'NODE'
const re = /-/;
RegExp.prototype.replace = undefined;
console.log('string-key:', 'a-b'.replace(re, '+'));
const saved = RegExp.prototype[Symbol.replace];
RegExp.prototype[Symbol.replace] = () => 'overridden';
console.log('symbol-key:', 'a-b'.replace(re, '+'));
RegExp.prototype[Symbol.replace] = saved;
NODERepository: theMackabu/ant
Length of output: 2194
Exercise Symbol.replace, not the unrelated "replace" property.
putReplace() writes RegExp.prototype.replace, but String.prototype.replace() dispatches via RegExp.prototype[Symbol.replace], so the assertion cannot observe the override path under test. Store through Symbol.replace, assert an observable custom result, and restore originalReplace.
Also applies to: 36-39
🤖 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/test_jit_regexp_exec_override.cjs` around lines 9 - 10, Update
putReplace() and its related test setup to override the object’s Symbol.replace
method instead of the string-keyed replace property. Make the custom
implementation return an observable result, assert that result through
String.prototype.replace(), and restore originalReplace afterward.
Merged
…sia-parity # Conflicts: # docs/exec-plans/active/README.md # docs/exec-plans/completed/README.md # examples/bench-v8/score.json # include/internal.h # include/modules/headers.h # include/utf8.h # meson/pgo/profiles/ant-darwin-aarch64.profdata # src/ant.c # src/gc/gc.c # src/gc/objects.c # src/modules/builtin.c # src/modules/headers.c # src/modules/request.c # src/modules/response.c # src/utf8.c # tests/harness/harness.js # tests/harness/manifest.js
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/regex.c (1)
1277-1286: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse invalid-subject-safe PCRE2 matching for JavaScript strings.
All PCRE2 compilations use
PCRE2_UTF, and interpreted matches validate the subject unlessPCRE2_NO_UTF_CHECKis set; lone-UTF-16-surrogate subjects cannot get that option. AddPCRE2_MATCH_INVALID_UTFto the compile options so valid ECMAScript strings such as"\uD800"do not fail before matching through eitherpcre2_matchorpcre2_jit_match.🤖 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/modules/regex.c` around lines 1277 - 1286, Update the PCRE2 compilation options in the regex compilation path to include PCRE2_MATCH_INVALID_UTF alongside PCRE2_UTF, ensuring JavaScript strings containing lone UTF-16 surrogates can be matched without validation failure through both pcre2_match and pcre2_jit_match. Locate the compile-options setup used to create compiled.code; leave the matching branch logic unchanged.
🤖 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 `@src/modules/response.c`:
- Around line 1142-1151: Update js_response_clone to capture and validate the
bool results from headers_copy_from and the related header-copy operations,
including headers_create_empty, headers_set_immutable, and readable_stream_tee.
On any false result, free the duplicated clone data and return the corresponding
error value instead of constructing a partial Response.
In `@src/modules/server.c`:
- Around line 920-923: The server_finish_websocket_upgrade path in
src/modules/server.c lines 920-923 must check whether response_get_headers
returns an error before enumerating headers and stop the upgrade on failure; the
regular response completion path at src/modules/server.c lines 1026-1029 must
perform the same check before capturing or serializing headers and send the
internal-error response when it fails.
In `@src/utf8.c`:
- Around line 396-400: Update utf8_replace_wtf8_surrogates to validate bytes[i +
2] as a UTF-8 continuation byte before replacing the three-byte sequence, while
preserving the existing ED A0-BF prefix check and replacement behavior for valid
surrogate sequences.
In `@tests/harness/harness.js`:
- Around line 2-5: Update the Child process handling to listen for both spawn
exit and error events, settling this.exited exactly once regardless of which
occurs first. In the error handler, append the launch error message to output so
callers can record the failed executable launch, while preserving the existing
exit handling behavior.
---
Outside diff comments:
In `@src/modules/regex.c`:
- Around line 1277-1286: Update the PCRE2 compilation options in the regex
compilation path to include PCRE2_MATCH_INVALID_UTF alongside PCRE2_UTF,
ensuring JavaScript strings containing lone UTF-16 surrogates can be matched
without validation failure through both pcre2_match and pcre2_jit_match. Locate
the compile-options setup used to create compiled.code; leave the matching
branch logic unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d22a73d-ac9f-436a-9616-e78761e97660
📒 Files selected for processing (38)
docs/exec-plans/active/README.mddocs/exec-plans/completed/README.mdinclude/ant.hinclude/arena.hinclude/gc.hinclude/internal.hinclude/modules/buffer.hinclude/modules/headers.hinclude/modules/regex.hinclude/modules/response.hinclude/object.hinclude/silver/engine.hinclude/utf8.hsrc/ant.csrc/gc/gc.csrc/gc/objects.csrc/main.csrc/modules/buffer.csrc/modules/builtin.csrc/modules/crypto.csrc/modules/fetch.csrc/modules/headers.csrc/modules/path.csrc/modules/regex.csrc/modules/request.csrc/modules/response.csrc/modules/server.csrc/modules/textcodec.csrc/silver/compiler.csrc/silver/engine.csrc/silver/glue.csrc/silver/ops/coercion.hsrc/silver/ops/comparison.hsrc/silver/ops/globals.hsrc/silver/ops/property.hsrc/utf8.ctests/harness/harness.jstests/harness/manifest.js
💤 Files with no reviewable changes (2)
- include/utf8.h
- src/modules/path.c
🚧 Files skipped from review as they are similar to previous changes (26)
- src/modules/builtin.c
- src/main.c
- include/gc.h
- include/modules/buffer.h
- src/modules/fetch.c
- docs/exec-plans/active/README.md
- docs/exec-plans/completed/README.md
- include/modules/headers.h
- src/silver/ops/coercion.h
- src/silver/ops/globals.h
- src/silver/ops/comparison.h
- include/silver/engine.h
- include/object.h
- src/modules/request.c
- include/arena.h
- src/gc/objects.c
- src/modules/textcodec.c
- include/ant.h
- src/modules/crypto.c
- src/gc/gc.c
- src/silver/compiler.c
- src/modules/buffer.c
- include/modules/regex.h
- src/silver/glue.c
- src/silver/ops/property.h
- src/ant.c
Comment on lines
1142
to
1151
| src_headers = js_get_slot(this, SLOT_RESPONSE_HEADERS); | ||
| new_headers = headers_create_empty(js); | ||
| if (is_err(new_headers)) { | ||
| data_free(nd); | ||
| return new_headers; | ||
| if (headers_is_headers(src_headers)) { | ||
| new_headers = headers_create_empty(js); | ||
| if (is_err(new_headers)) { | ||
| data_free(js, nd); | ||
| return new_headers; | ||
| } | ||
| headers_copy_from(js, new_headers, src_headers); | ||
| headers_set_immutable(new_headers, headers_is_immutable(src_headers)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '\bheaders_copy_from\s*\(' include src
rg -n -C 14 \
'js_response_clone|headers_copy_from|headers_data_copy' \
src/modules/response.c src/modules/headers.cRepository: theMackabu/ant
Length of output: 16357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1080,1185p' src/modules/response.c
sed -n '240,288p' src/modules/headers.c
sed -n '9,30p' include/modules/headers.h
rg -n 'headers_finalize|headers_data_destroy|headers_free|js_value.*free|is_err|headers_is_headers|headers_is_immutable|headers_set_immutable|headers_create_from_data|headers_create_empty' src/modules/headers.c src/modules/response.c include/modules/headers.h src/modules/request.c | sed -n '1,180p'Repository: theMackabu/ant
Length of output: 16295
Check header-copy results from Response.clone.
headers_copy_from returns false when allocation fails during headers_create_empty, headers_copy_from, headers_set_immutable, or readable_stream_tee; each path in js_response_clone currently ignores that result and may return a Response object with only data copied from data_dup. Store and test the bool ok result before proceeding, free the clone data on failure, and return the error value.
🤖 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/modules/response.c` around lines 1142 - 1151, Update js_response_clone to
capture and validate the bool results from headers_copy_from and the related
header-copy operations, including headers_create_empty, headers_set_immutable,
and readable_stream_tee. On any false result, free the duplicated clone data and
return the corresponding error value instead of constructing a partial Response.
Comment on lines
920
to
923
| static bool server_finish_websocket_upgrade(server_request_t *req, ant_value_t response_obj, ant_value_t websocket_obj) { | ||
| response_data_t *resp = response_get_data(response_obj); | ||
| ant_value_t headers = response_get_headers(response_obj); | ||
| ant_value_t headers = response_get_headers(req->server->js, response_obj); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle errors from response_get_headers.
response_get_headers lazily materializes a Headers object and can return an error value. Both completion paths treat that value as Headers data.
src/modules/server.c#L920-L923: Check for an error before enumerating WebSocket upgrade headers. Stop the upgrade on failure.src/modules/server.c#L1026-L1029: Check for an error before capturing or serializing regular response headers. Send the internal-error response on failure.
📍 Affects 1 file
src/modules/server.c#L920-L923(this comment)src/modules/server.c#L1026-L1029
🤖 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/modules/server.c` around lines 920 - 923, The
server_finish_websocket_upgrade path in src/modules/server.c lines 920-923 must
check whether response_get_headers returns an error before enumerating headers
and stop the upgrade on failure; the regular response completion path at
src/modules/server.c lines 1026-1029 must perform the same check before
capturing or serializing headers and send the internal-error response when it
fails.
Comment on lines
+396
to
+400
| void utf8_replace_wtf8_surrogates(uint8_t *bytes, size_t len) { | ||
| for (size_t i = 0; i + 2 < len; i++) { | ||
| if (bytes[i] != 0xed || (bytes[i + 1] & 0xe0) != 0xa0) continue; | ||
| memcpy(bytes + i, "\xef\xbf\xbd", 3); | ||
| i += 2; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the third byte before replacing a surrogate.
Line 398 accepts ED A0-BF with any third byte. ED A0 41 is not a WTF-8 surrogate, but the function replaces it and loses byte 0x41. Require bytes[i + 2] to be a continuation byte.
Proposed fix
- if (bytes[i] != 0xed || (bytes[i + 1] & 0xe0) != 0xa0) continue;
+ if (bytes[i] != 0xed ||
+ (bytes[i + 1] & 0xe0) != 0xa0 ||
+ (bytes[i + 2] & 0xc0) != 0x80) continue;📝 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
| void utf8_replace_wtf8_surrogates(uint8_t *bytes, size_t len) { | |
| for (size_t i = 0; i + 2 < len; i++) { | |
| if (bytes[i] != 0xed || (bytes[i + 1] & 0xe0) != 0xa0) continue; | |
| memcpy(bytes + i, "\xef\xbf\xbd", 3); | |
| i += 2; | |
| void utf8_replace_wtf8_surrogates(uint8_t *bytes, size_t len) { | |
| for (size_t i = 0; i + 2 < len; i++) { | |
| if (bytes[i] != 0xed || | |
| (bytes[i + 1] & 0xe0) != 0xa0 || | |
| (bytes[i + 2] & 0xc0) != 0x80) continue; | |
| memcpy(bytes + i, "\xef\xbf\xbd", 3); | |
| i += 2; |
🤖 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/utf8.c` around lines 396 - 400, Update utf8_replace_wtf8_surrogates to
validate bytes[i + 2] as a UTF-8 continuation byte before replacing the
three-byte sequence, while preserving the existing ED A0-BF prefix check and
replacement behavior for valid surrogate sequences.
Comment on lines
+2
to
+5
| import { existsSync, readFileSync, writeFileSync, mkdirSync } from 'node:fs'; | ||
| import { resolve } from 'node:path'; | ||
|
|
||
| const ANT = process.env.ANT_TEST_BIN || process.execPath; | ||
| const ANT = resolve(process.env.ANT_TEST_BIN || process.execPath); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to current Node.js documentation, does child_process.spawn() report a missing or non-executable command through the ChildProcess errorevent, and does an EventEmitter without anerror listener throw?
💡 Result:
Yes, child_process.spawn reports a missing command (and other spawn-related failures, such as a non-executable command that cannot be executed) by emitting an 'error' event on the ChildProcess instance [1][2][3]. Regarding the behavior of an EventEmitter when an 'error' event is emitted: according to the Node.js documentation, if an EventEmitter does not have at least one listener registered for the 'error' event, the error is thrown, a stack trace is printed, and the Node.js process exits [4][5][6]. Therefore, if you do not attach an 'error' listener to the child process object returned by spawn, any failure to spawn the process will result in an unhandled exception that terminates the entire Node.js process [7][8][4].
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://nodejs.org/dist/latest/docs/api/child_process.html
- 3: https://nodejs.org/docs/latest-v26.x/api/child_process.html
- 4: https://nodejs.org/api/events.html
- 5: https://nodejs.org/dist/latest/docs/api/events.html
- 6: https://nodejs.org/download/release/v23.6.1/docs/api/events.html
- 7: http://stackoverflow.com/questions/27688804/how-do-i-debug-error-spawn-enoent-on-node-js
- 8: child_process.spawn() uncatchable error nodejs/node#3523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)harness\.js$' || true
echo
echo "harness.js outline:"
ast-grep outline tests/harness/harness.js --view condensed || true
echo
echo "Relevant Child lines:"
nl -ba tests/harness/harness.js | sed -n '1,280p' | rg -n "class Child|constructor\\(|this\\.proc|\\.on\\(|\\.wait\\(|resolve\\(|exitCode|exited" -A 2 -B 2
echo
echo "All Child usages:"
rg -n "new Child|\\.wait\\(|child\\.wait" tests/harness/harness.js
echo
echo "Node event probe:"
node - <<'JS'
const { spawn } = require('node:child_process');
const path = require('node:path');
const fs = require('node:fs');
const absent = path.join(process.cwd(), 'this-file-definitely-does-not-exist-ant-test-bin');
const proc = spawn(absent);
let exitEmitted = false;
let errorEmitted = false;
let thrown = false;
let code = null;
try {
proc.on('exit', (code) => {
exitEmitted = true;
code = code;
});
proc.on('close', () => {});
proc.on('spawn', () => {});
} catch (e) {
thrown = true;
}
const timeout = setTimeout(() => {
console.log(JSON.stringify({
hasErrorListener: proc.listenerCount('error') >= 1,
exitEmitted,
errorEmitted,
thrown,
exitCode: code
}, null, 2));
process.exit(0);
}, 200);
timeout.unref();
JSRepository: theMackabu/ant
Length of output: 447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant Child section:"
awk '{printf "%5d\t%s\n", NR, $0}' tests/harness/harness.js | sed -n '120,280p'
echo
echo "All Child usages:"
rg -n "new Child|\\.wait\\(|child\\.wait" tests/harness/harness.js
echo
echo "Node event probe:"
node - <<'JS'
const { spawn } = require('node:child_process');
const path = require('node:path');
const absent = path.join(process.cwd(), 'this-file-definitely-does-not-exist-ant-test-bin');
const proc = spawn(absent);
let exitEmitted = false;
let errorEmitted = false;
try {
proc.on('exit', (code) => {
exitEmitted = true;
console.log({exitEmitted,errorEmitted,code});
});
} catch (e) {
console.log({exitEmitted,errorEmitted,thrown:true,message:e.message});
}
setTimeout(() => {
console.log(JSON.stringify({exitEmitted,errorEmitted}, null, 2));
process.exit(0);
}, 200);
JSRepository: theMackabu/ant
Length of output: 8058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Top of tests/harness/harness.js:"
awk '{printf "%5d\t%s\n", NR, $0}' tests/harness/harness.js | sed -n '1,120p'
echo
echo "Node event probe with error handler:"
node - <<'JS'
const { spawn } = require('node:child_process');
const path = require('node:path');
const absent = path.join(process.cwd(), 'this-file-definitely-does-not-exist-ant-test-bin');
const proc = spawn(absent);
proc.on('exit', (code) => {
console.log(JSON.stringify({exitEmitted:true,errorEmitted:true,code}));
process.exit(0);
});
proc.on('error', (err) => {
console.log(err.code);
});
setTimeout(() => {
console.log(JSON.stringify({exitEmitted:false,errorEmitted:false}));
}, 200);
JSRepository: theMackabu/ant
Length of output: 4183
Handle failed executable launch in Child.
ANT_TEST_BIN can name a missing or non-executable file. In that case, spawn() emits error. Child has no error listener, so the unhandled error event exits the harness before test callers can record a failure. Settle this.exited once from both exit and error, and append the launch error message to output.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
🤖 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/harness/harness.js` around lines 2 - 5, Update the Child process
handling to listen for both spawn exit and error events, settling this.exited
exactly once regardless of which occurs first. In the error handler, append the
launch error message to output so callers can record the failed executable
launch, while preserving the existing exit handling behavior.
- restore O(1) ascii fast path for Buffer.byteLength/Buffer.from with ascii/latin1/ucs2 encodings, matching the memoized ucs2 branch - guard ms() harness helper against swapped (min, max) arguments from upstream merges - cross-reference JS_NATIVE_CTOR with the SV_CALL_* call_flags bits - allocate response data_dup clones from the native-data arena - export ANT_NATIVE_DATA_SLOT_SIZE and use it in the slot-fit asserts - derive the substring ascii flag from the memoized utf16 length - drop stale g_ prefixes on file statics; tidy decl order and blank lines left by the merge - re-run bench-v8 on the merged tree
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 by CodeRabbit
lastIndexbehavior.