fix(formal): synchronize bytecode format version - #2
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (74)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds UTF-16 ECMAScript strings, a verified multi-module bytecode envelope, module-aware compiler and runtime execution, expanded lint analysis and CLI support, new runtime builtins, updated JIT/AOT ABIs, and differential corpus verification across JIT and AOT modes. ChangesWhole-program bytecode and compiler
Execution and CLI
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
|
| Filename | Overview |
|---|---|
| crates/bamts-runtime/src/builtins/mod.rs | define_descriptor still inserts descriptors after only an extensibility check; non-configurable/frozen property invariants remain unenforced. |
| crates/bamts-runtime/src/builtins/object.rs | Object.defineProperty path merges descriptors and delegates to define_descriptor without ECMAScript-compatible rejection of illegal redefinitions. |
| crates/bamts-bytecode/src/program.rs | Whole-program Program envelope and linkage for the format/version cutover; not implicated by the outstanding defineProperty defect. |
| crates/bamts-runtime/src/lib.rs | Large Program-based runtime cutover and execution paths; outstanding issue is in builtins property definition, not the main VM switch itself. |
Reviews (2): Last reviewed commit: "feat(runtime): resume timers and suspend..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 38
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/bamts-compiler/src/checker.rs (1)
1928-1945: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTwo function-body scopes bind statements without the new hoisting pre-pass.
resolve_functionestablishes the correct pairing at Line 1755 (bind_statementsthenbind_hoisted_statements); these two call sites create aScopeKind::Functionscope and skip the second call, so avarnested in a block is invisible to earlier references and reportsCANNOT_FIND_NAME.
crates/bamts-compiler/src/checker.rs#L1928-L1945: addself.bind_hoisted_statements(&block.data().statements, child);afterbind_statementsin the arrowFunctionBody::Blockarm.crates/bamts-compiler/src/checker.rs#L1873-L1878: add the same call forconstructor.body.data().statementsbeforeresolve_statements.🤖 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 `@crates/bamts-compiler/src/checker.rs` around lines 1928 - 1945, Function-body scopes for arrow functions and constructors omit the hoisting pre-pass, leaving nested var declarations unavailable to earlier references. In crates/bamts-compiler/src/checker.rs at lines 1928-1945, update the arrow FunctionBody::Block arm to call bind_hoisted_statements after bind_statements and before resolve_statements; apply the same change to the constructor body at lines 1873-1878 before resolve_statements, matching resolve_function’s ordering.crates/bamts-runtime/src/native.rs (1)
943-953: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
applydropspending_abi_erroron the floor.FatalTrap here drains
pending_errorandpending_fatal_kindbut notpending_abi_error. UnderBackend::Linked, a dynamic module runs throughrun_frame, and a nestedinvoke_linkedfailure sets exactly that cell before returningFatal. Result: the realAbiError::UnknownFunctionis reported asInvalidValue, and the latched error leaks into whatever unrelated failure surfaces next.invoke_linked_entrygets this ordering right — copy it.🐛 Drain the ABI error first
CompletionTag::FatalTrap => { + if let Some(error) = self.pending_abi_error.take() { + // No RuntimeError shape for linkage failures on this path: + // surface it as the fatal kind rather than losing it. + self.pending_abi_error.set(Some(error)); + } if let Some(error) = self.pending_error.take() { return Err(error); }Better: give the reference driver a way to propagate
AbiErrorverbatim instead of re-latching it.🤖 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 `@crates/bamts-runtime/src/native.rs` around lines 943 - 953, Update the CompletionTag::FatalTrap handling in apply to drain and return pending_abi_error before consuming pending_error or pending_fatal_kind, preserving the original AbiError such as UnknownFunction. Match the ordering and propagation behavior used by invoke_linked_entry, and ensure the ABI error is cleared so it cannot leak into a later failure.
🟡 Other comments (16)
crates/bamts-runtime/src/regexp.rs-644-657 (1)
644-657: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDead
_in_classparameter, and[\b]is broken because of it.
parse_escapetakes_in_class: bool, is called exactly once withfalse(line 538), and ignores it. That underscore is the author admitting the distinction was never implemented — and the consequence is right next door: inside a class,\bfalls throughparse_class_item's_arm intoescape_character, which has no0x62case, so it yields the literal'b'. ECMAScript requires[\b]to be U+0008 BACKSPACE.Either wire the flag up or delete it. Don't leave a parameter as a monument to an unfinished idea.
🐛 Handle the class-only escape and drop the dead parameter
let escaped = self.next().ok_or_else(|| self.error("trailing escape"))?; match escaped { 0x64 => Ok(ClassItem::Digit), 0x44 => Ok(ClassItem::NotDigit), 0x77 => Ok(ClassItem::Word), 0x57 => Ok(ClassItem::NotWord), 0x73 => Ok(ClassItem::Space), 0x53 => Ok(ClassItem::NotSpace), + // `\b` is BACKSPACE inside a class, not a word boundary. + 0x62 => Ok(ClassItem::Character(0x08)), _ => self.escape_character(escaped).map(ClassItem::Character), } } - fn parse_escape(&mut self, _in_class: bool) -> Result<Node, RegexError> { + fn parse_escape(&mut self) -> Result<Node, RegexError> {Update the single call site at line 538 accordingly.
🤖 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 `@crates/bamts-runtime/src/regexp.rs` around lines 644 - 657, Remove the unused _in_class parameter from parse_escape and update its sole call site to match. In parse_class_item, handle escape byte 0x62 as the backspace character U+0008 so [\b] produces ClassItem::Character('\u{0008}') rather than the literal 'b'; leave other escape handling unchanged.crates/bamts-cli/src/driver.rs-916-962 (1)
916-962: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTemp directories leak on failure, and the project root is whatever
/tmpgives you.Two problems. First,
remove_dir_allonly runs on the success path — the?and theassert_eq!both bail out leaving the directory behind, and since the name is keyed onprocess::id()you get a fresh turd every failing run. Second, the entrypoint lives outside CWD, sodiscover_project_rootwalks/tmpand/forbamts.toml/tsconfig.json; a stray marker file from any other tool makes/tmpthe project root and this test fails mysteriously. Drop atsconfig.jsonin the case directory to pin the root.🔧 Pin the root and clean up unconditionally
let entrypoint = directory.join("main.ts"); + std::fs::write(directory.join("tsconfig.json"), "{}")?; std::fs::write(&entrypoint, source)?; let args = parse_args(["run", entrypoint.to_str().expect("UTF-8 temp path")])?; - let outcome = super::execute(&args)?; - assert_eq!(outcome.stdout, expected_stdout, "{name}"); - assert_eq!(outcome.exit_code, 0, "{name}"); - std::fs::remove_dir_all(directory)?; + let result = super::execute(&args); + let _ = std::fs::remove_dir_all(&directory); + let outcome = result?; + assert_eq!(outcome.stdout, expected_stdout, "{name}"); + assert_eq!(outcome.exit_code, 0, "{name}");🤖 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 `@crates/bamts-cli/src/driver.rs` around lines 916 - 962, Update run_executes_node_vm_scripts_with_the_linked_backend to create a tsconfig.json marker in each case directory, ensuring project-root discovery is anchored there rather than walking /tmp or /. Replace the manual success-path cleanup with an unconditional cleanup guard so the temporary directory is removed when filesystem operations, execute, or assertions fail.crates/bamts-cli/src/diagnostics.rs-83-105 (1)
83-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTruncation is silent. Tell the user you hid their errors.
.take(error_limit)drops diagnostics with zero indication. The per-rule notes only cover rule-tagged lints —DiagnosticReport::newpushes diagnostics withrule == Nonestraight through without a summary entry (crates/bamts-compiler/src/diagnostic.rs:524-530). So a build capped at 50 with 300 hard errors prints 50 and nothing else. The user fixes 50 things, re-runs, and gets 50 more. That is exactly the kind of "helpful" throttling that wastes an afternoon.Emit a note when you truncate.
🔧 Report the suppressed remainder
- let diagnostics = report - .diagnostics() - .iter() - .take(error_limit) - .cloned() - .collect::<Vec<_>>(); + let available = report.diagnostics(); + let diagnostics = available + .iter() + .take(error_limit) + .cloned() + .collect::<Vec<_>>(); + let suppressed = available.len().saturating_sub(diagnostics.len()); let mut rendered = render(format, &diagnostics, sources); if matches!( format, DiagnosticsFormat::Text | DiagnosticsFormat::Pretty | DiagnosticsFormat::Compact ) { + if suppressed > 0 { + let _ = writeln!( + rendered, + "note: {suppressed} further diagnostic(s) not shown; raise `--error-limit`" + ); + } for summary in report.summaries() {🤖 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 `@crates/bamts-cli/src/diagnostics.rs` around lines 83 - 105, Update the diagnostic rendering flow around diagnostics collection and the existing text-format summary loop to detect when error_limit truncates report.diagnostics(). Emit a clear note reporting the number of suppressed diagnostics whenever the collected count is below the total count, including untagged diagnostics, while preserving the current per-rule notes and format behavior.crates/bamts-cli/src/main.rs-12-16 (1)
12-16: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
None => 2exits with a failure code and says nothing.Silent non-zero exit is the single most user-hostile thing a CLI can do. Yes,
parse_argscurrently guaranteesexplain_ruleisSomewhenever the mode isExplainand--help/-Vweren't given — which makes this branch dead code that exists solely to satisfy theOption. If it's truly unreachable, don't invent a silent exit path for it; report the error you already have a variant for.🔧 Say something before dying
- Ok(args) if args.is_explain() => match args.explain_rule.as_deref().map(explain_rule) { - Some(Ok(explanation)) => write_stdout(explanation.as_bytes()), - Some(Err(error)) => report_usage_error(&error), - None => 2, - }, + Ok(args) if args.is_explain() => { + match args + .explain_rule + .as_deref() + .ok_or(ArgsError::MissingExplainRule) + .and_then(explain_rule) + { + Ok(explanation) => write_stdout(explanation.as_bytes()), + Err(error) => report_usage_error(&error), + } + }🤖 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 `@crates/bamts-cli/src/main.rs` around lines 12 - 16, Update the Explain branch in main to handle a missing explain_rule by reporting the existing appropriate usage error variant instead of returning a silent non-zero status. Preserve the current success and explain_rule error handling, and reuse the existing error-reporting mechanism.crates/bamts-cli/src/args.rs-682-688 (1)
682-688: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
explainis order-dependent while every other subcommand isn't.You went out of your way two hunks below (Line 713) to make the entrypoint constraint "order-independent", and
run/checkaccepta.ts run— there's a whole test for that. Thenexplaingets bolted on with amode == Some(Mode::Explain)check that only looks at state already parsed. Sobamts explicit-any explainstuffs the rule intoentrypoint, silently drops it, and dies with "missing rule name for explain subcommand". Wrong diagnosis handed to the user.Resolve the rule after the scan instead of during it.
🔧 Resolve the explain rule after mode resolution
"explain" => set_mode(&mut mode, Mode::Explain)?, _ => { if mode == Some(Mode::Explain) { if explain_rule.replace(arg.clone()).is_some() { return Err(ArgsError::UnexpectedArgument { arg: arg.clone() }); } } else if entrypoint.is_some() {Then, after
effective_modeis computed (around Line 708):if effective_mode == Mode::Explain && explain_rule.is_none() { // A rule name scanned before the `explain` token landed in the entrypoint slot. explain_rule = entrypoint.take(); if let Some(extra) = extra_inputs.first() { return Err(ArgsError::UnexpectedArgument { arg: extra.clone() }); } }🤖 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 `@crates/bamts-cli/src/args.rs` around lines 682 - 688, Make explain rule parsing order-independent in the argument scan around the mode handling and effective_mode resolution. Do not rely on mode == Some(Mode::Explain) while scanning; after effective_mode is computed, move a rule captured in entrypoint into explain_rule when no rule was found, and reject any extra_inputs as UnexpectedArgument while preserving existing explain validation.crates/bamts-verification/tests/corpus_differential.rs-17-20 (1)
17-20: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDon't hardcode "20" next to an assertion that already knows the count.
The assertion compares against
PINNED_CASE_IDS; the prose will be a lie the first time someone pins case 21.🔧 Fix
assert_eq!( ids, PINNED_CASE_IDS, - "the differential harness must cover exactly the 20 pinned cases in manifest order" + "the differential harness must cover exactly the {} pinned cases in manifest order", + PINNED_CASE_IDS.len() );🤖 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 `@crates/bamts-verification/tests/corpus_differential.rs` around lines 17 - 20, Update the assertion message associated with the comparison of ids and PINNED_CASE_IDS to derive or reference the pinned collection’s actual count instead of hardcoding “20,” while preserving the requirement that all pinned cases appear in manifest order.crates/bamts-bytecode/src/string.rs-63-70 (1)
63-70: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDocument the panic on a public API.
slice_unitsindexesself.0[range]and will panic on an out-of-range or inverted range. Every other method on this type is total; if this one is not, say so in the doc comment (# Panics).🤖 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 `@crates/bamts-bytecode/src/string.rs` around lines 63 - 70, Update the public method String::slice_units documentation to add a # Panics section stating that it panics when the requested range is out of bounds or inverted; leave the slicing behavior unchanged.crates/bamts-compiler/src/checker.rs-868-880 (1)
868-880: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCanonicalize the merged diagnostics
check_with_lintsandcheck_programjust append the core, warning, and semantic diagnostics;check_core()hands itsVec<Diagnostic>back untouched. So the final order only stays sorted if every emitter happens to line up. Sort here once (and dedup if that’s part of the contract), or drop the sorted-output assertion inhard_warnings_merge_into_ordered_diagnostics.🤖 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 `@crates/bamts-compiler/src/checker.rs` around lines 868 - 880, Update check_with_lints and the corresponding check_program flow to canonicalize the merged diagnostics after combining core, warning, and semantic results, ensuring the returned diagnostics are sorted and deduplicated if required by the existing contract. Preserve the current diagnostic sources and update hard_warnings_merge_into_ordered_diagnostics only if the canonicalization contract is intentionally removed.crates/bamts-compiler/src/rules/mod.rs-1182-1198 (1)
1182-1198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
block_contains_value_returndoesn't look insidetry, loops, or labels — BAMTS-W065 will miss real cases.This only recurses through
Block/If/Switch. Areturn value;sitting inside atry,for,while,do-while,for-in/of, or a labeled statement is invisible to this function, sovisit_functionwill never flagBAMTS-W065for code like:function f(x: boolean): number { try { if (x) return 1; } catch {} }That function absolutely has an implicit-return-path bug and this check silently walks past it. Either wire this up properly or don't bother pretending it's exhaustive.
🔧 Proposed fix
fn block_contains_value_return(statements: &[Stmt]) -> bool { statements.iter().any(|statement| match statement.data() { Statement::Return(ret) => ret.argument.is_some(), Statement::Block(block) => block_contains_value_return(&block.data().statements), Statement::If(statement) => { block_contains_value_return(std::slice::from_ref(statement.consequent.as_ref())) || statement.alternate.as_ref().is_some_and(|alternate| { block_contains_value_return(std::slice::from_ref(alternate.as_ref())) }) } Statement::Switch(statement) => statement .cases .iter() .any(|case| block_contains_value_return(&case.data().consequent)), + Statement::Try(statement) => { + block_contains_value_return(&statement.block.data().statements) + || statement.handler.as_ref().is_some_and(|handler| { + block_contains_value_return(&handler.data().body.data().statements) + }) + || statement.finalizer.as_ref().is_some_and(|finalizer| { + block_contains_value_return(&finalizer.data().statements) + }) + } + Statement::For(statement) => { + block_contains_value_return(std::slice::from_ref(statement.body.as_ref())) + } + Statement::ForIn(statement) => { + block_contains_value_return(std::slice::from_ref(statement.body.as_ref())) + } + Statement::ForOf(statement) => { + block_contains_value_return(std::slice::from_ref(statement.body.as_ref())) + } + Statement::While(statement) => { + block_contains_value_return(std::slice::from_ref(statement.body.as_ref())) + } + Statement::DoWhile(statement) => { + block_contains_value_return(std::slice::from_ref(statement.body.as_ref())) + } + Statement::Labeled(statement) => { + block_contains_value_return(std::slice::from_ref(statement.body.as_ref())) + } _ => false, }) }🤖 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 `@crates/bamts-compiler/src/rules/mod.rs` around lines 1182 - 1198, Extend block_contains_value_return to recurse through Try, all loop forms (For, While, DoWhile, ForIn, and ForOf), and Labeled statements, traversing each contained statement list or body while preserving the existing Return, Block, If, and Switch handling so BAMTS-W065 detects value returns in every nested control-flow construct.crates/bamts-compiler/src/rules/mod.rs-127-168 (1)
127-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't fake a source location for compiler-option diagnostics
These
BAMTS-W082/083/084warnings still come out asline 1, col 1with a caret, which is a bogus source frame for a config-wide setting. Render zero-length boundary diagnostics without a code frame.🤖 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 `@crates/bamts-compiler/src/rules/mod.rs` around lines 127 - 168, Update analyze_compiler_options and the diagnostic rendering path so BAMTS-W082, BAMTS-W083, and BAMTS-W084 use zero-length boundary diagnostics without a source code frame or caret, rather than presenting TextRange::new(Utf16Pos::ZERO, Utf16Pos::ZERO) as line 1, column 1. Preserve their existing rule IDs, messages, severity handling, and sorting.crates/bamts-compiler/src/script.rs-119-123 (1)
119-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEvery
ProgramVerifyErroris mislabelled asCapacity.Linkage failures, entry-module mismatches and unresolved exports are not capacity exhaustion. Callers that branch on
ScriptCompileError(thenode:vmsurface, per the stack context) will report "a fixed compiler or bytecode capacity was exhausted" for a link bug. Map the variants, or add aLinkvariant and stop pretending.🐛 Distinguish linkage failures from capacity
pub enum ScriptCompileError { ... /// A fixed compiler or bytecode capacity was exhausted. Capacity { message: String }, + /// The assembled program failed bytecode-level verification or linking. + Link { message: String }, }fn map_program_error(error: ProgramVerifyError) -> ScriptCompileError { - ScriptCompileError::Capacity { - message: error.to_string(), - } + ScriptCompileError::Link { + message: error.to_string(), + } }Match on the concrete
ProgramVerifyErrorvariants if any of them genuinely are capacity limits.🤖 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 `@crates/bamts-compiler/src/script.rs` around lines 119 - 123, Update map_program_error to distinguish ProgramVerifyError variants instead of mapping every error to ScriptCompileError::Capacity. Preserve Capacity only for genuine capacity-limit variants, and map linkage, entry-module, and unresolved-export failures to the appropriate existing error variant or add a dedicated Link variant and update callers such as the node:vm surface to report it correctly.crates/bamts-native/src/native_bridge.rs-306-313 (1)
306-313: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winOne botched bulk doc insertion damaged two doc comments. The same "# Safety / The caller must provide a live, uniquely owned
frame..." block was spliced into the middle of two unrelated sentences, on items that take no pointers and have no safety contract. Rustdoc renders both as gibberish.
crates/bamts-native/src/native_bridge.rs#L306-L313: remove the injected block soNativeHelper::symbol's doc reads "…Byte-identical tobamts_codegen::Helper::symbol." again.crates/bamts-native/src/native_bridge.rs#L2164-L2171: remove the injected block so theCODEGEN_HELPERSdoc reads "…must stay byte-identical tobamts_codegen::Helper::{external_index, symbol}." again.🤖 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 `@crates/bamts-native/src/native_bridge.rs` around lines 306 - 313, Remove the incorrectly inserted Safety documentation block from NativeHelper::symbol in crates/bamts-native/src/native_bridge.rs:306-313, restoring the sentence ending with bamts_codegen::Helper::symbol. Remove the same block from the CODEGEN_HELPERS documentation at crates/bamts-native/src/native_bridge.rs:2164-2171, restoring the sentence ending with bamts_codegen::Helper::{external_index, symbol}; make no other changes.crates/bamts-verification/src/corpus.rs-838-846 (1)
838-846: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDrop the
mut.
outcomeis moved intowith_aot_compile_evidenceand never mutated here; clippy flags it, and this workspace almost certainly builds with warnings denied.🧹 Proposed fix
- let mut outcome = run_process( + let outcome = run_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 `@crates/bamts-verification/src/corpus.rs` around lines 838 - 846, Remove the unnecessary mutability from the outcome binding in the run_process flow before with_aot_compile_evidence; keep the existing value handling unchanged.Source: Linters/SAST tools
crates/bamts-runtime/src/vm.rs-155-183 (1)
155-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAllocate the constructed prototype after validating the arguments.
constructed_prototyperuns — and may allocate — beforesource_argumentshas coercedcodeor type-checkedoptions. Two consequences:new vm.runInThisContext(code, badOptions)leaves an allocated prototype behind on the throw path, and the observable side-effect order is inverted (ToString ofcodemust happen first). This file goes to considerable trouble elsewhere to prove failed installations retain nothing; don't undermine it here.♻️ Reorder
- let prototype = if constructing { - let function = machine - ... - } else { - None - }; - let (code, name) = source_arguments(machine, args)?; + let (code, name) = source_arguments(machine, args)?; + let prototype = if constructing { + let function = machine + .registry + .external + .get(&EcmaString::from_utf8("node:vm")) + .and_then(|module| { + module + .exports + .get(&EcmaString::from_utf8("runInThisContext")) + }) + .expect("node:vm installs runInThisContext") + .value; + Some( + machine + .constructed_prototype(function) + .map_err(EvalFailure::Runtime)?, + ) + } else { + None + };🤖 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 `@crates/bamts-runtime/src/vm.rs` around lines 155 - 183, In the runInThisContext flow, move the constructed-prototype lookup and constructed_prototype call to after source_arguments(machine, args) succeeds, so code coercion and options validation occur first. Preserve the existing constructing-dependent prototype behavior and pass the resulting prototype unchanged to call_entry.crates/bamts-runtime/src/builtins/regexp.rs-191-207 (1)
191-207: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
toStringreads the raw pattern, sonew RegExp("").toString()returns"//".The constructor deliberately stores
"(?:)"in thesourceproperty for an empty pattern (Line 65), thento_stringbypasses that and rebuilds fromregexp_parts, which hands back the raw emptypattern."//"is a line comment, which is the entire reason the spec mandates(?:). Use the same empty-pattern fallback here (or read thesourceproperty).🐛 Honour the empty-source form
let mut output = bamts_bytecode::EcmaStringBuilder::new(); output.push_unit(u16::from(b'/')); - for &unit in source.as_units() { + let source = if source.is_empty() { + EcmaString::from_utf8("(?:)") + } else { + source + }; + for &unit in source.as_units() { if unit == u16::from(b'/') { output.push_unit(u16::from(b'\\')); } output.push_unit(unit); }🤖 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 `@crates/bamts-runtime/src/builtins/regexp.rs` around lines 191 - 207, Update the to_string implementation to use the normalized source value, preserving the constructor’s "(?:)" fallback for an empty pattern instead of rebuilding from the raw regexp_parts pattern. Ensure new RegExp("").toString() produces "/(?:)/" while leaving escaping and flag handling unchanged.crates/bamts-runtime/src/builtins/string.rs-307-326 (1)
307-326: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix
lastIndexOf’sNaNposition handling.integer()collapsesNaNto0, solastIndexOf("a", NaN)searches from the start and returns the wrong result.NaNshould act like+∞and search the full string.🤖 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 `@crates/bamts-runtime/src/builtins/string.rs` around lines 307 - 326, Update the position handling in last_index_of so an explicit NaN argument is treated as positive infinity, using the full haystack length as the search endpoint instead of relying on integer(), while preserving the existing clamping behavior for defined numeric positions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 9beb5ea7-54b4-4b5a-b73a-1ae8063764ff
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
corpus/bamts.tomlcrates/bamts-bytecode/src/lib.rscrates/bamts-bytecode/src/program.rscrates/bamts-bytecode/src/string.rscrates/bamts-cli/Cargo.tomlcrates/bamts-cli/build.rscrates/bamts-cli/src/args.rscrates/bamts-cli/src/diagnostics.rscrates/bamts-cli/src/driver.rscrates/bamts-cli/src/main.rscrates/bamts-cli/tests/cli.rscrates/bamts-codegen/src/aot.rscrates/bamts-codegen/src/jit.rscrates/bamts-codegen/src/lib.rscrates/bamts-compiler/RULES.mdcrates/bamts-compiler/src/bin/generate_rule_reference.rscrates/bamts-compiler/src/checker.rscrates/bamts-compiler/src/checker/intrinsic_environment.rscrates/bamts-compiler/src/emitter.rscrates/bamts-compiler/src/lib.rscrates/bamts-compiler/src/lint.rscrates/bamts-compiler/src/lower.rscrates/bamts-compiler/src/parser.rscrates/bamts-compiler/src/pipeline.rscrates/bamts-compiler/src/program.rscrates/bamts-compiler/src/rules/mod.rscrates/bamts-compiler/src/rules/semantic/coercions.rscrates/bamts-compiler/src/rules/semantic/control_flow.rscrates/bamts-compiler/src/rules/semantic/enums.rscrates/bamts-compiler/src/rules/semantic/flow_safety.rscrates/bamts-compiler/src/rules/semantic/functions.rscrates/bamts-compiler/src/rules/semantic/intrinsics.rscrates/bamts-compiler/src/rules/semantic/members.rscrates/bamts-compiler/src/rules/semantic/mod.rscrates/bamts-compiler/src/rules/semantic/modules.rscrates/bamts-compiler/src/rules/semantic/object_types.rscrates/bamts-compiler/src/scanner.rscrates/bamts-compiler/src/script.rscrates/bamts-compiler/src/source.rscrates/bamts-compiler/src/syntax.rscrates/bamts-compiler/tests/rules.rscrates/bamts-native/src/lib.rscrates/bamts-native/src/native_bridge.rscrates/bamts-node/src/lib.rscrates/bamts-runtime/src/builtins/array.rscrates/bamts-runtime/src/builtins/collections.rscrates/bamts-runtime/src/builtins/date.rscrates/bamts-runtime/src/builtins/json.rscrates/bamts-runtime/src/builtins/mod.rscrates/bamts-runtime/src/builtins/number.rscrates/bamts-runtime/src/builtins/object.rscrates/bamts-runtime/src/builtins/regexp.rscrates/bamts-runtime/src/builtins/string.rscrates/bamts-runtime/src/builtins/symbol.rscrates/bamts-runtime/src/external_modules.rscrates/bamts-runtime/src/host_objects.rscrates/bamts-runtime/src/intrinsics.rscrates/bamts-runtime/src/lib.rscrates/bamts-runtime/src/native.rscrates/bamts-runtime/src/regexp.rscrates/bamts-runtime/src/vm.rscrates/bamts-verification/src/corpus.rscrates/bamts-verification/src/main.rscrates/bamts-verification/src/workspace_guard.rscrates/bamts-verification/tests/corpus_differential.rscrates/bamts/Cargo.tomlcrates/bamts/src/lib.rsformal/lean/Bamti/Bytecode/Model.leanproof/lean-assumptions.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
🪛 Clippy (1.97.1)
crates/bamts-verification/src/corpus.rs
[warning] 838-838: variable does not need to be mutable
(warning)
🪛 LanguageTool
crates/bamts-compiler/RULES.md
[style] ~630-~630: The adverb ‘never’ is usually put before the verb ‘assert’.
Context: ...d alternative: Handle every variant and assert never in the default branch. - Silence: `-A e...
(ADVERB_WORD_ORDER)
🔍 Remote MCP DeepWiki, Tavily, Valyu
Additional context found
- The PR’s own summary says it is a broad migration from module-by-module execution to a canonical whole-program container, and that it bumps
FORMAT_VERSIONto2,PROGRAM_VERSIONto3, and the AOT ABI tov3; it also says previous formats are incompatible and persisted outputs should be regenerated. - The PR page shows 69 selected files, spanning bytecode, compiler, runtime, codegen, CLI, verification, native/node, and formal/proof areas.
- I couldn’t independently inspect the repo through repo-indexed tools here: DeepWiki reported the repository was not found, and Tavily searches hit a usage-limit error.
🔇 Additional comments (112)
crates/bamts-compiler/src/rules/semantic/object_types.rs (1)
5-38: LGTM!crates/bamts-compiler/src/rules/semantic/modules.rs (1)
5-54: LGTM!crates/bamts-runtime/src/host_objects.rs (1)
55-67: LGTM!Also applies to: 132-152, 155-157, 240-240, 251-251
crates/bamts-runtime/src/regexp.rs (1)
17-61: LGTM!Also applies to: 97-149, 285-302, 718-731, 798-809
crates/bamts-runtime/src/intrinsics.rs (2)
58-63: LGTM!Also applies to: 86-91, 140-192, 204-233, 287-287, 364-376, 481-481, 512-551
26-31: 🗄️ Data Integrity & IntegrationNo issue.
prototypeis already used in both construct paths viaallocate_constructed_receiver_with(prototype), so it affects the constructed object’s prototype.> Likely an incorrect or invalid review comment.crates/bamts-cli/Cargo.toml (1)
12-12: 📐 Maintainability & Code Quality
node-hostis real.crates/bamts/Cargo.tomldefines it, so this is valid. The directbamts-nodedependency is for the build script; it doesn’t make this edge redundant.> Likely an incorrect or invalid review comment.crates/bamts-compiler/src/source.rs (1)
27-51: 🗄️ Data Integrity & IntegrationThe invariant is enforced earlier The loader canonicalizes local paths before they reach
LoadState::visit, soSourceIdentity::newonly sees canonical paths. The doc comment matches the code.> Likely an incorrect or invalid review comment.corpus/bamts.toml (1)
1-1: LGTM!crates/bamts-cli/build.rs (1)
29-46: LGTM!Also applies to: 68-70, 114-116
crates/bamts-cli/src/args.rs (8)
15-28: LGTM!Also applies to: 37-37
167-201: LGTM!Also applies to: 217-221
258-265: LGTM!Also applies to: 321-329
347-347: LGTM!Also applies to: 374-381
392-408: LGTM!
455-459: LGTM!Also applies to: 632-669
708-716: LGTM!Also applies to: 754-754, 770-774
939-941: LGTM!Also applies to: 1213-1273
crates/bamts-cli/src/diagnostics.rs (2)
32-32: LGTM!Also applies to: 573-573
926-941: LGTM!crates/bamts-cli/src/driver.rs (7)
9-23: LGTM!Also applies to: 51-61, 119-123, 138-158
288-335: LGTM!
454-459: LGTM!Also applies to: 695-754, 764-779
781-811: LGTM!
830-836: LGTM!
241-250: 🩺 Stability & AvailabilityNo issue:
ArgsErrorimplementsError
crates/bamts-cli/src/args.rs:334already hasimpl std::error::Error for ArgsError {}, soSelf::Usage(error) => Some(error)is fine.> Likely an incorrect or invalid review comment.
755-761: 🎯 Functional Correctness
FrontendMode::Checkis correct here.compileandrunlower fromFrontendOutputdirectly; this mode only controls optional emit, so hardcodingCheckis intentional.> Likely an incorrect or invalid review comment.crates/bamts-cli/src/main.rs (1)
40-42: LGTM!proof/lean-assumptions.json (1)
309-309: 🗄️ Data Integrity & IntegrationNeed the hashing inputs behind
source_sha256. The manifest only makes sense with the declared file set and normalization rules used to compute it.crates/bamts-cli/tests/cli.rs (3)
7-17: LGTM!
141-222: LGTM!
62-70: 📐 Maintainability & Code QualityDrop the cache-dir warning for the JIT tests.
run --target jitnever touchesBAMTS_CACHE_DIR; the only cache lookup is in the AOT link path viacached_node_archive(). These two JIT tests are not leaking ambient cache state.> Likely an incorrect or invalid review comment.crates/bamts/src/lib.rs (4)
44-62: LGTM!Also applies to: 67-88, 94-116
383-544: LGTM!
229-235: 🎯 Functional CorrectnessDrop this complaint:
compile_source_fileis not the linting front door.bamts.tomlis loaded in the CLI check path (lint_config_path/levels), while the facade only usestsconfig.jsonto load and compile the program. That split is deliberate, not a bug.> Likely an incorrect or invalid review comment.
136-139: 🎯 Functional CorrectnessDrop this filename complaint.
compile_classic_scriptonly usesresource_nameto derive a module name; the error path surfaces just the message, and the stack builder hardcodes<bamts>.String::from_utf16_lossy(source.name)is ugly, but it does not corrupt the reportedSyntaxError/stack text here.> Likely an incorrect or invalid review comment.crates/bamts-verification/src/main.rs (1)
191-203: LGTM!crates/bamts-verification/src/workspace_guard.rs (1)
479-509: LGTM!Also applies to: 903-910
crates/bamts-verification/tests/corpus_differential.rs (1)
41-148: LGTM!formal/lean/Bamti/Bytecode/Model.lean (1)
193-193: 🗄️ Data Integrity & IntegrationDrop this complaint; it's the wrong layer.
formal/lean/Bamti/Bytecode/Model.leanmodels the module wire, not the whole-program envelope.formatVersion = 2andmagicBytes = [66, 77, 84, 66, 67, 0, 0, 1]matchcrates/bamts-bytecode/src/lib.rsexactly, and the trailing1is part of the magic bytes, not a version field. If you meant the program envelope, that lives incrates/bamts-bytecode/src/program.rs.> Likely an incorrect or invalid review comment.crates/bamts/Cargo.toml (1)
11-17: 📐 Maintainability & Code QualityCI wiring isn’t present here.
node-hostis feature-gated as intended, but this repo doesn’t define a workflow to flip it on, socargo test -p bamtsmissing those tests is not a CI bug in this tree.> Likely an incorrect or invalid review comment.crates/bamts-bytecode/src/lib.rs (1)
60-62: LGTM!Also applies to: 106-106, 172-184, 279-279, 985-992, 1021-1033, 1919-1920, 1935-1936, 2028-2028, 2182-2182, 2198-2209, 2569-2569, 2582-2588, 2864-2876, 3459-3464, 3491-3504, 3506-3575, 4697-4731
crates/bamts-compiler/src/checker.rs (1)
13-20: LGTM!Also applies to: 29-30, 86-94, 132-145, 147-173, 281-307, 470-474, 539-539, 555-629, 647-744, 754-765, 811-820, 952-956, 968-994, 996-1044, 1054-1054, 1129-1135, 1196-1282, 1404-1452, 1555-1579, 1693-1693, 1716-1751, 1763-1809, 1995-2000, 2101-2115, 2218-2247, 2249-2282, 2375-2430, 2432-2448, 2487-2501, 2606-2667, 2848-2940, 3061-3145, 3344-3362, 3440-3475
crates/bamts-compiler/src/lower.rs (1)
47-55: LGTM!Also applies to: 93-110, 135-136, 175-180, 198-198, 229-231, 256-258, 275-275, 312-352, 361-380, 431-433, 537-542, 553-568, 663-663, 708-718, 798-823, 873-895, 899-912, 984-1029, 1030-1069, 1582-1641, 1646-1740, 2004-2033, 2989-2989, 3030-3044, 3058-3070, 3074-3103, 3149-3162, 3179-3195, 3198-3228, 3234-3284, 3286-3310, 3312-3323, 3326-3340, 3342-3356, 3358-3463, 3465-3479, 3899-3988, 4177-4237, 4251-4278, 4343-4407, 5077-5102, 5106-5110, 5186-5211, 5352-5442, 5533-5533, 5629-5635, 5672-5672, 5687-5701
crates/bamts-compiler/src/parser.rs (2)
3000-3008: LGTM!
3009-3024: 🎯 Functional CorrectnessNo issue here: every
Expression::Asconsumer in the compiler just unwrapsexpressionand ignorestype_node, so making it optional doesn’t change emission or traversal. Drop this.> Likely an incorrect or invalid review comment.crates/bamts-compiler/src/pipeline.rs (1)
17-19: LGTM!Also applies to: 132-173, 195-220
crates/bamts-compiler/src/program.rs (1)
33-206: LGTM!Also applies to: 208-337, 339-644, 646-712, 714-783, 818-1237, 1239-1270, 1272-1330, 1343-1413, 1415-1526, 1528-1576, 1585-1692, 1694-1789, 1791-1886, 1888-2011, 2013-2091, 2093-2155, 2157-2174, 2188-2241, 2243-2305, 2307-3159
crates/bamts-compiler/src/checker/intrinsic_environment.rs (1)
35-115: 🎯 Functional CorrectnessDrop this complaint: the runtime surface is intentionally closed. The browser/Node globals you listed are not part of this target, so treating their absence as a bug here is noise.
> Likely an incorrect or invalid review comment.crates/bamts-bytecode/src/program.rs (1)
960-977: 📐 Maintainability & Code QualityThis isn’t a wire-format divergence.
ProgramDecoder::u32/write_u32andDecoder::leb128/write_u32implement the same canonical unsigned LEB128 rules; the only real difference is the local error type plumbing.> Likely an incorrect or invalid review comment.crates/bamts-compiler/RULES.md (1)
1-864: LGTM!crates/bamts-compiler/src/bin/generate_rule_reference.rs (1)
1-6: LGTM!crates/bamts-compiler/src/rules/mod.rs (1)
1-13: LGTM!Also applies to: 40-108, 870-924, 944-1180, 1200-1244
crates/bamts-compiler/src/rules/semantic/coercions.rs (1)
1-30: LGTM!crates/bamts-compiler/src/rules/semantic/control_flow.rs (1)
1-14: LGTM!crates/bamts-compiler/src/rules/semantic/enums.rs (1)
1-22: LGTM!crates/bamts-compiler/src/rules/semantic/flow_safety.rs (1)
1-30: LGTM!crates/bamts-compiler/src/scanner.rs (1)
763-780: Correct, spec-conformant fix for surrogate handling.Pushing the
charvalidity check out ofread_hex_code_pointand intoscan_identifier_escapeis the right call: strings/templates are raw UTF-16 code-unit sequences (lone surrogates legal), identifiers require actual Unicode scalar values satisfying ID_Start/ID_Continue. Good catch, good fix.Also applies to: 865-933
crates/bamts-compiler/src/lint.rs (1)
365-368: The vacuous"const safe: number = 1;"clean examples are already covered by the contract-consolidation comment oncrates/bamts-compiler/src/rules/semantic/mod.rslines 1034-1170.Also applies to: 1216-1216, 1300-1303
crates/bamts-compiler/src/rules/semantic/functions.rs (1)
5-38: LGTM!crates/bamts-compiler/src/rules/semantic/intrinsics.rs (1)
5-38: LGTM!crates/bamts-compiler/src/rules/semantic/members.rs (1)
5-54: LGTM!crates/bamts-compiler/src/lib.rs (1)
8-17: LGTM!crates/bamts-compiler/src/script.rs (1)
87-96: 🗄️ Data Integrity & IntegrationNo issue: the module name is intentionally slot 0.
assemble_classic_script_namedpasses it as the only linkage string, andassemble_with_linkage_stringsinterns linkage strings before any other constants.> Likely an incorrect or invalid review comment.crates/bamts-codegen/src/aot.rs (1)
202-236: LGTM!Also applies to: 336-386, 536-596
crates/bamts-codegen/src/jit.rs (1)
68-98: LGTM!Also applies to: 257-257, 496-513, 824-839
crates/bamts-codegen/src/lib.rs (1)
196-197: LGTM!Also applies to: 236-238, 1108-1117, 1667-1678, 3086-3117
crates/bamts-native/src/lib.rs (1)
401-418: LGTM!Also applies to: 426-438, 463-470, 668-672
crates/bamts-node/src/lib.rs (3)
22-22: LGTM!Also applies to: 42-42, 83-85, 143-145, 546-591
381-385: 📐 Maintainability & Code QualityNo issue:
decode_aot_programis already gated with#[cfg(feature = "aot-main")], so it does not affect non-aot-mainbuilds.> Likely an incorrect or invalid review comment.
461-475: 🩺 Stability & AvailabilityNo duplication bug here.
host.stdout()andoutcome.stdoutare separate buffers, and this path intentionally prints both.> Likely an incorrect or invalid review comment.crates/bamts-runtime/src/builtins/array.rs (1)
333-356: LGTM!Also applies to: 724-724, 844-884
crates/bamts-native/src/native_bridge.rs (1)
1781-1821: LGTM!Also applies to: 1851-1885, 1893-1911, 2674-2778
crates/bamts-runtime/src/lib.rs (1)
370-397: LGTM!Also applies to: 1189-1297, 1547-1629, 3403-3449, 4170-4221
crates/bamts-runtime/src/native.rs (1)
198-216: LGTM!Also applies to: 1152-1245, 1618-1629, 2015-2027
crates/bamts-runtime/src/builtins/number.rs (1)
14-20: LGTM!Also applies to: 59-60, 214-226, 274-277, 317-320
crates/bamts-runtime/src/builtins/symbol.rs (1)
14-24: LGTM!Also applies to: 48-93, 120-120, 132-145, 180-201
crates/bamts-runtime/src/builtins/collections.rs (3)
3-23: LGTM!Also applies to: 51-119, 139-154, 380-458, 746-806
171-236: LGTM!
602-716: LGTM!crates/bamts-runtime/src/builtins/date.rs (1)
3-14: LGTM!Also applies to: 26-26, 37-37, 52-68, 88-90, 99-115
crates/bamts-runtime/src/builtins/json.rs (4)
3-28: LGTM!Also applies to: 38-47, 61-100, 117-146
156-235: LGTM!Also applies to: 226-335, 337-377
443-455: LGTM!Also applies to: 457-515, 517-572, 611-643, 645-671
704-790: LGTM!crates/bamts-runtime/src/builtins/mod.rs (3)
3-3: LGTM!Also applies to: 23-27, 63-96, 116-116, 133-133, 166-190, 226-226, 257-258, 426-449
505-538: LGTM!Also applies to: 584-601, 621-633, 694-722, 767-795
811-933: LGTM!Also applies to: 947-1009
crates/bamts-runtime/src/builtins/object.rs (5)
3-33: LGTM!Also applies to: 58-75, 113-124, 147-170, 179-192
245-341: LGTM!Also applies to: 384-431, 446-460, 462-482
517-549: LGTM!Also applies to: 633-642, 647-655, 663-704, 732-742
799-872: LGTM!Also applies to: 882-908
943-1465: LGTM!Also applies to: 1509-1560, 1569-1798
crates/bamts-runtime/src/builtins/regexp.rs (2)
4-29: LGTM!Also applies to: 39-44, 59-65, 86-111, 126-126, 137-137
227-227: LGTM!Also applies to: 242-253, 264-366
crates/bamts-runtime/src/builtins/string.rs (6)
3-21: LGTM!Also applies to: 66-66, 76-76, 87-87, 98-108
153-226: LGTM!Also applies to: 240-282, 300-306, 350-379
392-482: LGTM!Also applies to: 495-527
545-601: LGTM!Also applies to: 602-649, 730-768
778-863: LGTM!Also applies to: 865-891, 904-907
943-943: LGTM!Also applies to: 969-971, 993-1011, 1031-1039, 1049-1079, 1109-1194
crates/bamts-runtime/src/external_modules.rs (4)
1-117: LGTM!Also applies to: 119-309
311-492: LGTM!Also applies to: 494-539, 541-658
660-793: LGTM!
834-898: LGTM!Also applies to: 900-1525
crates/bamts-verification/src/corpus.rs (4)
2-19: LGTM!Also applies to: 35-39, 459-471, 545-546
855-916: LGTM!Also applies to: 918-970, 1019-1086
1123-1131: LGTM!Also applies to: 1407-1435, 1925-1966, 1993-2029
52-73: 📐 Maintainability & Code QualityNo drift bug here.
crates/bamts-verification/tests/corpus_differential.rsalready checkscorpus.casesagainstPINNED_CASE_IDSin manifest order, so this constant is not quietly going stale the way you claim.> Likely an incorrect or invalid review comment.crates/bamts-runtime/src/vm.rs (3)
1-122: LGTM!Also applies to: 124-147
185-215: LGTM!Also applies to: 217-243, 245-277, 279-329
331-859: LGTM!
| fn lint_config_path(args: &CliArgs) -> Option<PathBuf> { | ||
| let start = args.entrypoint.as_deref().map_or_else( | ||
| || std::env::current_dir().ok(), | ||
| |entrypoint| { | ||
| let path = Path::new(entrypoint); | ||
| let parent = path.parent().unwrap_or_else(|| Path::new(".")); | ||
| if parent.is_absolute() { | ||
| Some(parent.to_path_buf()) | ||
| } else { | ||
| std::env::current_dir() | ||
| .ok() | ||
| .map(|directory| directory.join(parent)) | ||
| } | ||
| }, | ||
| )?; | ||
| start | ||
| .ancestors() | ||
| .map(|directory| directory.join("bamts.toml")) | ||
| .find(|path| path.is_file()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Two independent, disagreeing searches for the same bamts.toml.
lint_config_path does its own ancestor walk from the raw entrypoint joined onto the CWD. discover_project_root (Line 764) walks the canonicalized entrypoint looking for the exact same marker file, and its result becomes the ProjectRoot at Line 733. Reach a source file through a symlinked directory and the two walks resolve different trees: module resolution is rooted at the real project while lint levels come from whatever bamts.toml sits above the symlink path. No error, no warning, just quietly different lint levels than the project asked for.
The insulting part is that levels(args) is called at Line 759 — twenty-odd lines after the canonical root is already sitting in a variable. Pass it in and delete this function.
🔧 Derive the lint config from the resolved root
-fn lint_config_path(args: &CliArgs) -> Option<PathBuf> {
- let start = args.entrypoint.as_deref().map_or_else(
- || std::env::current_dir().ok(),
- |entrypoint| {
- let path = Path::new(entrypoint);
- let parent = path.parent().unwrap_or_else(|| Path::new("."));
- if parent.is_absolute() {
- Some(parent.to_path_buf())
- } else {
- std::env::current_dir()
- .ok()
- .map(|directory| directory.join(parent))
- }
- },
- )?;
- start
- .ancestors()
- .map(|directory| directory.join("bamts.toml"))
- .find(|path| path.is_file())
-}
+fn lint_config_path(root: &Path) -> Option<PathBuf> {
+ let candidate = root.join("bamts.toml");
+ candidate.is_file().then_some(candidate)
+}levels then takes the root that load_program_frontend already resolved:
fn levels(args: &CliArgs, root: &Path) -> Result<LintTable, DriverError> {
// ...
if let Some(path) = lint_config_path(root) { /* unchanged */ }and Line 759 becomes let levels = levels(args, root.path())?;. Note the unit tests at Line 863 onward call levels(&args) and will need the extra argument.
🤖 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 `@crates/bamts-cli/src/driver.rs` around lines 337 - 356, Remove the
independent lint_config_path ancestor search and change levels to accept the
resolved project root path. Use root.path() at the existing levels call after
load_program_frontend has resolved the ProjectRoot, then locate bamts.toml
relative to that path while preserving the existing lint configuration behavior.
Update all tests and callers of levels to pass the root argument.
| let warnings = require_clean_frontend(args, frontend)?; | ||
| let executable = lower_program( | ||
| &frontend.program, | ||
| &frontend.output, | ||
| LowerOptions { | ||
| javascript_compatibility: is_javascript(unit.script_kind), | ||
| javascript_compatibility: true, | ||
| }, | ||
| ) | ||
| .map_err(DriverError::Lower)?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
LowerOptions is constructed with javascript_compatibility: true in two places and args.js_compat is read in neither. The root cause is a single missing wiring step: the CLI parses and documents --js-compat/--compat/--allow-js, but the driver never threads that state into lowering, and the lowering call was copy-pasted instead of shared.
crates/bamts-cli/src/driver.rs#L392-L400: extract thelower_programcall into a helper that derivesLowerOptionsfromargs.js_compat, and call it here.crates/bamts-cli/src/driver.rs#L420-L427: replace the duplicatedlower_program/LowerOptionsblock with that same helper.
📍 Affects 1 file
crates/bamts-cli/src/driver.rs#L392-L400(this comment)crates/bamts-cli/src/driver.rs#L420-L427
🤖 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 `@crates/bamts-cli/src/driver.rs` around lines 392 - 400, Extract the
duplicated lowering logic into a shared helper in driver.rs that builds
LowerOptions using args.js_compat and invokes lower_program. Update the lowering
call at crates/bamts-cli/src/driver.rs lines 392-400 to use the helper, and
replace the duplicate block at lines 420-427 with the same helper; both sites
should honor the parsed CLI compatibility setting instead of hardcoding true.
| fn require_clean_frontend( | ||
| args: &CliArgs, | ||
| frontend: &LoadedProgramFrontend, | ||
| ) -> Result<String, DriverError> { | ||
| let rendered = render_program_diagnostics(args, frontend); | ||
| if frontend | ||
| .output | ||
| .modules() | ||
| .iter() | ||
| .any(|module| module.has_errors()) | ||
| { | ||
| Err(DriverError::Diagnostics { rendered }) | ||
| } else { | ||
| Ok(rendered) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
require_clean_frontend and check are the same function written twice.
Both render the program diagnostics, both test modules().any(|m| m.has_errors()), both return DriverError::Diagnostics { rendered }. The only difference is what happens on success. Two copies of the error condition means two places to forget when the definition of "clean" changes.
♻️ Express `check` in terms of the shared predicate
fn check(args: &CliArgs, frontend: &LoadedProgramFrontend) -> Result<CommandOutcome, DriverError> {
- let rendered = render_program_diagnostics(args, frontend);
- if frontend
- .output
- .modules()
- .iter()
- .any(|module| module.has_errors())
- {
- return Err(DriverError::Diagnostics { rendered });
- }
+ let rendered = require_clean_frontend(args, frontend)?;
Ok(CommandOutcome {
stderr: rendered.into_bytes(),
..CommandOutcome::default()
})
}🤖 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 `@crates/bamts-cli/src/driver.rs` around lines 813 - 828, Refactor the `check`
implementation to reuse `require_clean_frontend` for the shared diagnostics
rendering and module-error predicate, adapting only its success result to
preserve `check`’s existing return behavior. Remove the duplicated
clean-frontend condition from `check` while leaving `require_clean_frontend` as
the single definition of the validation logic.
|
|
||
| #[test] | ||
| fn cli_rule_override_beats_a_later_group_override() { | ||
| let args = parse_args([ | ||
| "check", | ||
| "-A", | ||
| "explicit-any", | ||
| "-D", | ||
| "escape-hatches", | ||
| "main.ts", | ||
| ]) | ||
| .expect("arguments parse"); | ||
| let table = levels(&args).expect("overrides resolve"); | ||
| let explicit_any = rule_by_name("explicit-any").expect("registered rule").id(); | ||
| let implicit_any = rule_by_name("implicit-any").expect("registered rule").id(); | ||
| assert_eq!(table.level(explicit_any), LintLevel::Allow); | ||
| assert_eq!(table.level(implicit_any), LintLevel::Deny); | ||
| } | ||
|
|
||
| #[test] | ||
| fn lowering_forbid_is_a_typed_usage_error() { | ||
| let args = parse_args([ | ||
| "check", | ||
| "-F", | ||
| "explicit-any", | ||
| "-A", | ||
| "explicit-any", | ||
| "main.ts", | ||
| ]) | ||
| .expect("arguments parse"); | ||
| assert!(matches!( | ||
| levels(&args), | ||
| Err(DriverError::Usage(ArgsError::ForbiddenLintOverride { .. })) | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn strict_cli_profile_keeps_javascript_rules_nonfatal() { | ||
| let args = parse_args(["check", "--strict", "vendored.js"]).expect("arguments parse"); | ||
| let table = levels(&args).expect("profile resolves"); | ||
| let footgun = rule_by_name("invalid-number-formatting-options") | ||
| .expect("registered footgun") | ||
| .id(); | ||
| let typescript_only = rule_by_name("explicit-any").expect("registered rule").id(); | ||
| assert_eq!( | ||
| table.level_for_source(footgun, SourceDialect::JavaScript), | ||
| LintLevel::Warn | ||
| ); | ||
| assert_eq!( | ||
| table.level_for_source(typescript_only, SourceDialect::JavaScript), | ||
| LintLevel::Allow | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
These lint tests read whatever bamts.toml happens to live above the working directory.
levels(&args) calls lint_config_path, which walks CWD's ancestors for bamts.toml. main.ts and vendored.js don't exist, but that doesn't matter — the walk still hits real directories, and this very PR adds corpus/bamts.toml. The moment a bamts.toml lands at the workspace root, apply_config fires and these exact-level assertions start failing for reasons that have nothing to do with the code under test. Tests that depend on ambient filesystem state aren't tests.
Fixing lint_config_path to take an explicit root (see Line 337) makes these hermetic for free.
🤖 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 `@crates/bamts-cli/src/driver.rs` around lines 862 - 914, Update
lint_config_path and its callers, including levels, to accept an explicit test
root rather than walking from the ambient current working directory. Pass a
controlled temporary or nonexistent root from these lint override and strict
profile tests so apply_config cannot discover repository bamts.toml files, while
preserving production behavior through the appropriate real root.
| fn report_usage_error(error: &impl std::fmt::Display) -> i32 { | ||
| let message = format!("error: {error}\n\n{}", help_message()); | ||
| if write_stderr(message.as_bytes()) == 0 { | ||
| 2 | ||
| } else { | ||
| 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
You wrote the helper and then didn't use it where the duplicate already lives.
report_usage_error is byte-for-byte the body of the Err(error) arm in main (Line 28-33): same format!("error: {error}\n\n{}", help_message()), same 2/1 exit split. Route that arm through the helper so there's one definition of "usage error output".
♻️ Collapse the duplicate
- Err(error) => {
- let message = format!("error: {error}\n\n{}", help_message());
- if write_stderr(message.as_bytes()) == 0 {
- 2
- } else {
- 1
- }
- }
+ Err(error) => report_usage_error(&error),🤖 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 `@crates/bamts-cli/src/main.rs` around lines 51 - 58, Update the Err(error) arm
in main to delegate usage-error reporting to report_usage_error(error) instead
of duplicating the formatting and write_stderr exit-code logic, leaving
report_usage_error as the single implementation.
| pub struct AsExpression { | ||
| pub expression: Box<Expr>, | ||
| pub type_node: Box<Ty>, | ||
| pub type_node: Option<Box<Ty>>, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
as const is encoded as None and then never tested. One root cause: the new Option<Box<Ty>> payload smuggles a language construct into the absence of a type, and because the only updated test wraps the old case in Some(...), the new arm has zero coverage anywhere in the emitter suite.
crates/bamts-compiler/src/syntax.rs#L1538-L1541: replace the in-bandNonewith an explicitAsTarget { Const, Type(Box<Ty>) }, or at minimum document on the field thatNonemeansas constpercrates/bamts-compiler/src/parser.rslines 2986-3076.crates/bamts-compiler/src/emitter.rs#L3541-L3559: add a sibling test that buildsAsExpression { type_node: None }and asserts(a + b) * c;still erases with the required parentheses.
📍 Affects 2 files
crates/bamts-compiler/src/syntax.rs#L1538-L1541(this comment)crates/bamts-compiler/src/emitter.rs#L3541-L3559
🤖 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 `@crates/bamts-compiler/src/syntax.rs` around lines 1538 - 1541, The
AsExpression representation in crates/bamts-compiler/src/syntax.rs lines
1538-1541 must make the as-const target explicit: prefer an AsTarget enum with
Const and Type(Box<Ty>) variants, or document that type_node: None means as
const if retaining the Option. Add coverage in
crates/bamts-compiler/src/emitter.rs lines 3541-3559 by constructing
AsExpression with type_node: None and asserting erased output preserves the
required parentheses for (a + b) * c;.
| fn value<H: Host>(&mut self, machine: &mut Machine<'_, H>) -> Result<Value, String> { | ||
| self.ws(); | ||
| match self.peek() { | ||
| Some(b'n') => { | ||
| Some(unit) if unit == u16::from(b'n') => { | ||
| self.literal("null")?; | ||
| Ok(Value::NULL) | ||
| } | ||
| Some(b't') => { | ||
| Some(unit) if unit == u16::from(b't') => { | ||
| self.literal("true")?; | ||
| Ok(Value::TRUE) | ||
| } | ||
| Some(b'f') => { | ||
| Some(unit) if unit == u16::from(b'f') => { | ||
| self.literal("false")?; | ||
| Ok(Value::FALSE) | ||
| } | ||
| Some(b'"') => self | ||
| .string() | ||
| .and_then(|s| allocate_string(machine, s).map_err(|_| "Out of memory".to_owned())), | ||
| Some(b'[') => self.array(machine), | ||
| Some(b'{') => self.object(machine), | ||
| Some(b'-' | b'0'..=b'9') => self.number(), | ||
| Some(unit) if unit == u16::from(b'"') => { | ||
| let string = self.string()?; | ||
| allocate_string(machine, string).map_err(|_| "Out of memory".to_owned()) | ||
| } | ||
| Some(unit) if unit == u16::from(b'[') => self.array(machine), | ||
| Some(unit) if unit == u16::from(b'{') => self.object(machine), | ||
| Some(unit) | ||
| if unit == u16::from(b'-') | ||
| || (u16::from(b'0')..=u16::from(b'9')).contains(&unit) => | ||
| { | ||
| self.number() | ||
| } | ||
| _ => Err(self.error_unexpected()), | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any existing recursion/nesting bound in the runtime limits.
fd -t f 'lib.rs' crates/bamts-runtime/src --exec rg -nP -C3 'max_(depth|nesting|recursion|call_depth|frames)|struct Limits' {}Repository: metaphorics/bamTiScript
Length of output: 2759
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the JSON parser/serializer implementation around the claimed recursion points.
file=crates/bamts-runtime/src/builtins/json.rs
wc -l "$file"
sed -n '1,220p' "$file"
printf '\n--- parser/value area ---\n'
sed -n '220,520p' "$file"
printf '\n--- serializer/reviver area ---\n'
sed -n '520,860p' "$file"
# Find any depth/nesting related state in this file.
printf '\n--- depth-related symbols in json.rs ---\n'
rg -n '\b(depth|nest|recurs|call_depth|max_call_depth|MAX_JSON_DEPTH)\b' "$file" || trueRepository: metaphorics/bamTiScript
Length of output: 28402
Cap JSON nesting in crates/bamts-runtime/src/builtins/json.rs:412-441,574-598
parse() still recurses through value → array/object, and stringify()/walk_reviver() recurse the same way, with no nesting cap. Limits.max_call_depth does not protect this; deeply nested input will take the Rust stack down and abort the process instead of returning a JSON error. Thread an explicit depth limit through both paths.
🤖 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 `@crates/bamts-runtime/src/builtins/json.rs` around lines 412 - 441, Add an
explicit JSON nesting-depth counter and enforce a fixed limit throughout parsing
and serialization. Thread the depth through value(), array(), object(),
stringify(), and walk_reviver(), incrementing it on nested arrays/objects and
returning a JSON error when the limit is exceeded; do not rely on
Limits.max_call_depth.
| let Ok(s) = machine | ||
| .to_string(args.first().copied().unwrap_or(Value::UNDEFINED))? | ||
| .to_utf8_strict() | ||
| else { | ||
| return Ok(BuiltinOutcome::Value(crate::number_value(f64::NAN))); | ||
| }; | ||
| let s = trim_js(&s); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
parseFloat("1é") panics — you now feed arbitrary UTF-8 into a byte-index slice.
The prefix search below does s[..i] for every i in 1..=s.len(), and slicing a &str off a char boundary is a panic, not an error. Any non-ASCII byte after a numeric prefix kills the process. Restrict the scan to ASCII byte positions; numeric literals are ASCII anyway.
🐛 Only slice at char boundaries
- let mut end = 0;
- for i in 1..=s.len() {
- if s[..i].parse::<f64>().is_ok() {
- end = i
- }
- }
+ let mut end = 0;
+ for i in 1..=s.len() {
+ if s.is_char_boundary(i) && s[..i].parse::<f64>().is_ok() {
+ end = i
+ }
+ }While you're in there: that loop is O(n²) float parses over attacker-controlled length. Walk the numeric prefix once instead.
🤖 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 `@crates/bamts-runtime/src/builtins/number.rs` around lines 156 - 162, Update
the numeric-prefix scanning in the parseFloat implementation around trim_js so
it only examines ASCII/character-boundary positions, preventing invalid UTF-8
slicing for inputs such as “1é”. Replace the repeated prefix parsing loop with a
single-pass scan that identifies the longest valid ASCII numeric prefix and
parses it once, preserving existing parseFloat behavior for valid and invalid
inputs.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (19)
crates/bamts-compiler/src/program.rs (2)
784-815: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Is there an AST node for `import("...")` in type position? rg -nP 'ImportType|TypeNode::Import|import_type' crates/bamts-compiler/src/syntax.rs crates/bamts-compiler/src/parser.rs | head -40Repository: metaphorics/bamTiScript
Length of output: 865
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== program.rs around the edge collector ==\n' sed -n '760,835p' crates/bamts-compiler/src/program.rs printf '\n== parser.rs import type parsing ==\n' sed -n '4960,5335p' crates/bamts-compiler/src/parser.rs printf '\n== syntax.rs relevant node/token declarations ==\n' sed -n '270,310p' crates/bamts-compiler/src/syntax.rs sed -n '1730,1885p' crates/bamts-compiler/src/syntax.rsRepository: metaphorics/bamTiScript
Length of output: 21459
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== token kind definitions and keyword handling ==\n' rg -n "KwImport|Identifier|keyword" crates/bamts-compiler/src -g '!**/target/**' | head -120 printf '\n== member/property parsing ==\n' rg -n "Dot|Member|property|parse.*member|parse.*call" crates/bamts-compiler/src/parser.rs crates/bamts-compiler/src/syntax.rs | head -120Repository: metaphorics/bamTiScript
Length of output: 23571
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== lexer/tokenizer source files ==\n' git ls-files crates/bamts-compiler/src | sed -n '1,200p' | rg 'lex|token|scan|syntax|parser' printf '\n== keyword tokenization for import ==\n' rg -n "import" crates/bamts-compiler/src | head -200Repository: metaphorics/bamTiScript
Length of output: 21243
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== program.rs TypeNode::Import usage ==\n' rg -n "TypeNode::Import|ImportType|scan_types|scan_type|TypeNode" crates/bamts-compiler/src/program.rs printf '\n== parser syntax for type AST traversal ==\n' sed -n '835,980p' crates/bamts-compiler/src/program.rsRepository: metaphorics/bamTiScript
Length of output: 202
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== program.rs TypeNode::Import usage ==\n' rg -n "TypeNode::Import|ImportType|scan_types|scan_type|TypeNode" crates/bamts-compiler/src/program.rs printf '\n== type traversal around the edge collector ==\n' sed -n '835,980p' crates/bamts-compiler/src/program.rsRepository: metaphorics/bamTiScript
Length of output: 202
Parse
TypeNode::Importhere instead of guessing from token triples.foo.import("./x")still producesKwImport, LParen, StringLiteral, so this scan happily invents a module edge from a member call and can turn it intoUnresolvedModule.program.rsalready hasTypeNode::Import; use the AST and drop the token-window hack.🤖 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 `@crates/bamts-compiler/src/program.rs` around lines 784 - 815, Replace the token-window scan in the module-edge collection logic with traversal/parsing of the existing TypeNode::Import AST nodes. Only add TypeOnly edges for actual import type nodes, excluding member calls such as foo.import("./x"), and remove the tokens/windows-based detection and related deduplication logic.
1332-1341: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
parse_hexoverflows on a long\u{...}escape — debug panic, silent wrap in release.
value * 16 + digitis uncheckedu32, so"\u{ffffffffff}"in any source file panics the compiler in a debug build and wraps to a bogus code point otherwise.cook_unicode_escapeinlower.rsalready usessaturating_mul/saturating_addfor exactly this input — which is the real problem: this file re-implements string-literal escape cooking a second time and the two copies have already diverged. Fix the arithmetic, then collapseunquoteonto the crate's single cooking routine.🐛 Stop the overflow
bytes.iter().try_fold(0_u32, |value, byte| { char::from(*byte) .to_digit(16) - .map(|digit| value * 16 + digit) + .and_then(|digit| value.checked_mul(16)?.checked_add(digit)) })📝 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.fn parse_hex(bytes: &[u8]) -> Option<u32> { if bytes.is_empty() { return None; } bytes.iter().try_fold(0_u32, |value, byte| { char::from(*byte) .to_digit(16) .and_then(|digit| value.checked_mul(16)?.checked_add(digit)) }) }🤖 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 `@crates/bamts-compiler/src/program.rs` around lines 1332 - 1341, Update parse_hex to avoid unchecked u32 overflow while parsing long hexadecimal escapes, preserving rejection of values that exceed the valid range. Then remove the duplicate string-literal escape-cooking path and make unquote reuse the existing cook_unicode_escape routine in lower.rs, keeping escape behavior consistent across the compiler.crates/bamts-runtime/src/builtins/object.rs (1)
433-444: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Is there any non-configurable / frozen validation on the define path? rg -nP -C4 'fn define_descriptor' crates/bamts-runtime/src rg -nP -C2 'configurable' crates/bamts-runtime/src/builtins/object.rs | rg -n 'is_configurable|reject|type_error' # Any test asserting defineProperty on a frozen object throws? rg -nP -C3 'freeze' crates/bamts-runtime/src --glob '*.rs' | rg -n 'defineProperty|define_descriptor'Repository: metaphorics/bamTiScript
Length of output: 1003
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the property-definition path and any descriptor validation helpers. sed -n '880,960p' crates/bamts-runtime/src/builtins/mod.rs printf '\n--- object.rs ---\n' sed -n '360,470p' crates/bamts-runtime/src/builtins/object.rs printf '\n--- descriptor/search ---\n' rg -n "into_property|ValidateAndApplyPropertyDescriptor|is_configurable|is_writable|non-configurable|non-writable|type_error\\(" crates/bamts-runtime/src printf '\n--- tests ---\n' rg -n "defineProperty|freeze\\(|Object::freeze|frozen|non-configurable|non-writable" crates/bamts-runtime/src --glob '*.rs'Repository: metaphorics/bamTiScript
Length of output: 24923
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the actual define_descriptor logic and the surrounding tests that exercise it. sed -n '960,1035p' crates/bamts-runtime/src/builtins/mod.rs printf '\n--- object tests around defineProperty ---\n' sed -n '1360,1465p' crates/bamts-runtime/src/builtins/object.rs printf '\n--- any frozen/non-configurable assertions in the repo tests ---\n' rg -n -C3 "define accessor on non-configurable object|non-configurable object|frozen object|isFrozen|Object\.freeze" crates/bamts-runtime/src/lib.rs crates/bamts-runtime/src/builtins/object.rsRepository: metaphorics/bamTiScript
Length of output: 7619
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check how descriptor merging works and whether frozen/non-configurable behavior is tested. sed -n '240,290p' crates/bamts-runtime/src/builtins/object.rs printf '\n--- frozen implementation/tests ---\n' sed -n '660,770p' crates/bamts-runtime/src/builtins/mod.rs printf '\n--- defineProperty test coverage around freeze ---\n' sed -n '980,1045p' crates/bamts-runtime/src/builtins/object.rs sed -n '1220,1310p' crates/bamts-runtime/src/builtins/object.rs rg -n -C4 "freeze\\(|isFrozen|defineProperty.*is_err|non-configurable|non-writable|configurable" crates/bamts-runtime/src/builtins/object.rs crates/bamts-runtime/src/builtins/mod.rsRepository: metaphorics/bamTiScript
Length of output: 39407
Implement real descriptor validation before
properties.insert.
Machine::define_descriptoronly blocks new keys on non-extensible objects; if the key already exists, it overwrites the descriptor outright. That means frozen and non-configurable properties can still be redefined.into_propertyonly fills defaults — it does not enforce the descriptor-compatibility rules.🤖 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 `@crates/bamts-runtime/src/builtins/object.rs` around lines 433 - 444, Update apply_property_descriptor and the Machine::define_descriptor path to validate the requested descriptor against the existing own descriptor before properties.insert occurs. Enforce non-configurable and frozen-property compatibility rules, while preserving the existing non-extensible check for new keys; do not rely on PropertyDescriptor::into_property, which only supplies defaults.crates/bamts-runtime/src/builtins/string.rs (2)
118-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
as u16saturates;ToUint16wraps.String.fromCharCodeis wrong for out-of-range arguments.Rust's float→int
ascast is saturating, sofromCharCode(65601)yieldsU+FFFFinstead ofU+0041, and any negative argument yieldsU+0000instead of wrapping. The spec is unambiguous: each argument goes through ToUint16, i.e. modulo 2^16 after truncation.🐛 Proper ToUint16
let mut out = EcmaStringBuilder::with_capacity(args.len()); for arg in args { - out.push_unit(value_number(machine.to_number(*arg)?) as u16); + let number = value_number(machine.to_number(*arg)?); + let unit = if number.is_finite() { + (number.trunc() as i64).rem_euclid(0x1_0000) as u16 + } else { + 0 + }; + out.push_unit(unit); }📝 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.let mut out = EcmaStringBuilder::with_capacity(args.len()); for arg in args { let number = value_number(machine.to_number(*arg)?); let unit = if number.is_finite() { (number.trunc() as i64).rem_euclid(0x1_0000) as u16 } else { 0 }; out.push_unit(unit); }🤖 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 `@crates/bamts-runtime/src/builtins/string.rs` around lines 118 - 121, Update the String.fromCharCode implementation around EcmaStringBuilder and value_number so each converted argument applies ECMAScript ToUint16 semantics: truncate the numeric value and wrap modulo 2^16 before producing the u16 code unit. Replace the saturating Rust as u16 conversion while preserving normal in-range character behavior and the existing error propagation from machine.to_number.
650-692: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Is there an existing maximum string length / unit ceiling to reuse? rg -nP -C3 'MAX_STRING|max_string|len_units\(\)\s*>|Invalid string length' crates/bamts-runtime/src crates/bamts-bytecode/srcRepository: metaphorics/bamTiScript
Length of output: 2157
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- pad/repeat slices ---' sed -n '630,760p' crates/bamts-runtime/src/builtins/string.rs echo echo '--- runtime string-limit search ---' rg -n 'max_string_units|max string units|Invalid string length|len_units\(\).*MAX|MAX_STRING|string length' crates/bamts-runtime/srcRepository: metaphorics/bamTiScript
Length of output: 4208
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- builder and allocation helpers ---' rg -n 'fn allocate_string|struct EcmaStringBuilder|with_capacity\(|heap budget|Limits|budget|charge.*string|string_units' crates/bamts-runtime/src crates/bamts-bytecode/src echo echo '--- relevant helper slices ---' for f in \ crates/bamts-runtime/src/builtins/string.rs \ crates/bamts-runtime/src/string.rs \ crates/bamts-runtime/src/lib.rs \ crates/bamts-runtime/src/machine.rs do if [ -f "$f" ]; then echo "### $f" sed -n '1,220p' "$f" | sed -n '1,220p' echo fi doneRepository: metaphorics/bamTiScript
Length of output: 38442
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- allocate_string and related helpers ---' sed -n '90,180p' crates/bamts-runtime/src/builtins/mod.rs echo echo '--- EcmaStringBuilder implementation ---' sed -n '200,280p' crates/bamts-bytecode/src/string.rsRepository: metaphorics/bamTiScript
Length of output: 5040
Reject oversized
padStart/repeatlengths before allocating
padStart(Infinity)becomesusize::MAX, and both helpers build the full buffer withVec::with_capacity(...)/collect()beforeallocate_stringcan enforce any budget. A large finiterepeatcount has the same failure mode. Clamp against the runtime string ceiling first, or these calls can OOM/abort the 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 `@crates/bamts-runtime/src/builtins/string.rs` around lines 650 - 692, Update the pad helper and the corresponding repeat implementation to validate the requested output length against the runtime’s established string-size ceiling before converting to usize or allocating/collecting buffers. Reject Infinity and oversized finite lengths with the existing failure behavior, preserving normal padding and repetition for valid lengths.crates/bamts-runtime/src/builtins/symbol.rs (1)
147-159: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Hand-rolled slot decoding that skips the segment check.
Machine::runtime_slotexists exactly so nobody does this: it validates the heap segment and the slot bound before you touchheap. Here you take anyHeapRef, subtract one, and index the runtime heap — so a reference from a different segment aliases an unrelated entry, and if that entry happens to be aSymbolyou hand its description toSymbol.prototype.description.call(foreign). Cross-heap type confusion on a.call()-able builtin. Use the validated accessor.🔒 Route through the validated accessor
fn symbol_description<H: Host>( machine: &Machine<'_, H>, value: Value, ) -> Result<EcmaString, EvalFailure> { - let Some(Decoded::HeapRef(id)) = value.decode() else { - return Err(type_error("Symbol method called on incompatible receiver")); - }; - let index = id.slot() as usize - 1; - match machine.heap.get(index) { - Some(HeapEntry::Symbol { description }) => Ok(description.clone()), + let Some(index) = machine.runtime_slot(value).map_err(EvalFailure::Runtime)? else { + return Err(type_error("Symbol method called on incompatible receiver")); + }; + match machine.heap.get(index) { + Some(HeapEntry::Symbol { description }) => Ok(description.clone()), _ => Err(type_error("Symbol method called on incompatible receiver")), } }📝 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.fn symbol_description<H: Host>( machine: &Machine<'_, H>, value: Value, ) -> Result<EcmaString, EvalFailure> { let Some(index) = machine.runtime_slot(value).map_err(EvalFailure::Runtime)? else { return Err(type_error("Symbol method called on incompatible receiver")); }; match machine.heap.get(index) { Some(HeapEntry::Symbol { description }) => Ok(description.clone()), _ => Err(type_error("Symbol method called on incompatible receiver")), } }🤖 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 `@crates/bamts-runtime/src/builtins/symbol.rs` around lines 147 - 159, Update symbol_description to resolve the receiver through Machine::runtime_slot instead of manually decoding HeapRef, subtracting one, and indexing machine.heap. Preserve the existing incompatible-receiver TypeError for invalid, foreign-segment, out-of-range, or non-Symbol values, while returning the cloned description for validated Symbol entries.crates/bamts-runtime/src/external_modules.rs (1)
821-832: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
expect("hex input is a string")panics on script-supplied input. This aborts the runtime.
chunks_exact(2)cuts the byte slice, not characters, so any multi-byte UTF-8 character at an odd byte offset lands split across two chunks andstd::str::from_utf8returnsErr.crypto.createHash('sha256').update('aéb', 'hex')is61 C3 A9 62— even length, so the length guard passes, and the first chunk is[0x61, 0xC3], which is invalid UTF-8. Thatexpectthen panics: a guest script unwinding/aborting the host, in a crate whose whole point is bounded, typed failures.Validate ASCII hex on the bytes directly instead of round-tripping through
str.🐛 Decode from bytes, no `expect`
fn decode_hex(text: &str) -> Result<Vec<u8>, EvalFailure> { - if !text.len().is_multiple_of(2) { + let bytes = text.as_bytes(); + if !bytes.len().is_multiple_of(2) { return Err(type_error("invalid hexadecimal hash input")); } - text.as_bytes() - .chunks_exact(2) + bytes + .chunks_exact(2) .map(|pair| { - let digits = std::str::from_utf8(pair).expect("hex input is a string"); - u8::from_str_radix(digits, 16).map_err(|_| type_error("invalid hexadecimal hash input")) + let nibble = |byte: u8| { + char::from(byte) + .to_digit(16) + .ok_or_else(|| type_error("invalid hexadecimal hash input")) + }; + Ok(((nibble(pair[0])? << 4) | nibble(pair[1])?) as u8) }) .collect() }Please add a test with a non-ASCII
hexpayload; the existing coverage only feeds it ASCII.📝 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.fn decode_hex(text: &str) -> Result<Vec<u8>, EvalFailure> { let bytes = text.as_bytes(); if !bytes.len().is_multiple_of(2) { return Err(type_error("invalid hexadecimal hash input")); } bytes .chunks_exact(2) .map(|pair| { let nibble = |byte: u8| { char::from(byte) .to_digit(16) .ok_or_else(|| type_error("invalid hexadecimal hash input")) }; Ok(((nibble(pair[0])? << 4) | nibble(pair[1])?) as u8) }) .collect() }🤖 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 `@crates/bamts-runtime/src/external_modules.rs` around lines 821 - 832, Update decode_hex to validate each byte pair directly as ASCII hexadecimal digits, removing the UTF-8 conversion and expect call so all script-supplied input returns type_error instead of panicking. Preserve the existing even-length and invalid-input behavior, and add coverage for a non-ASCII hex payload such as “aéb”.crates/bamts-runtime/src/host_objects.rs (3)
80-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Any global installed after host_objects::install would be invisible on globalThis. rg -nP -C4 'globals\.insert|host_objects::install|builtins::install' crates/bamts-runtime/src # Does property assignment on globalThis feed back into the globals map anywhere? rg -nP -C4 'globalThis' crates/bamts-runtime/srcRepository: metaphorics/bamTiScript
Length of output: 19278
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- host_objects.rs ---' sed -n '1,120p' crates/bamts-runtime/src/host_objects.rs echo echo '--- intrinsics.rs (global helpers / install) ---' sed -n '1,120p' crates/bamts-runtime/src/intrinsics.rs echo sed -n '280,340p' crates/bamts-runtime/src/intrinsics.rs echo sed -n '740,790p' crates/bamts-runtime/src/intrinsics.rs echo echo '--- lib.rs globals handling ---' sed -n '2220,2265p' crates/bamts-runtime/src/lib.rs echo echo '--- vm.rs globalThis usage ---' sed -n '300,330p' crates/bamts-runtime/src/vm.rsRepository: metaphorics/bamTiScript
Length of output: 13495
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- locate global access helpers ---' rg -n -C3 'fn global\(|fn set_global|self\.globals|globals\.' crates/bamts-runtime/src/lib.rs crates/bamts-runtime/src/vm.rs crates/bamts-runtime/src/intrinsics.rs echo echo '--- relevant lib.rs region around global helpers ---' sed -n '80,180p' crates/bamts-runtime/src/lib.rs echo echo '--- relevant vm.rs region around name resolution / assignment ---' rg -n -C4 'IdentifierReference|Binding|put_global|get_global|resolve' crates/bamts-runtime/src/vm.rs crates/bamts-runtime/src/lib.rsRepository: metaphorics/bamTiScript
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C4 'fn global\(|self\.globals\.insert|self\.globals\.get|globalThis|global\(' crates/bamts-runtime/src/lib.rs crates/bamts-runtime/src/vm.rs crates/bamts-runtime/src/intrinsics.rs crates/bamts-runtime/src/external_modules.rs echo echo '--- lib.rs first 260 lines ---' sed -n '1,260p' crates/bamts-runtime/src/lib.rs echo echo '--- vm.rs around assignment and identifier resolution ---' rg -n -C5 'Assign|Identifier|Global|Environment|Mutable' crates/bamts-runtime/src/vm.rsRepository: metaphorics/bamTiScript
Length of output: 23825
globalThisis a detached snapshot, not the live global record.
You build it from a one-time copy and never wire it back. SoglobalThis.foo = 1won’t create a global binding, and later globals won’t appear onglobalThis. If that split is intentional, document it; otherwise this needs to be the real global object.🤖 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 `@crates/bamts-runtime/src/host_objects.rs` around lines 80 - 87, Update the global initialization flow around globalThis so it uses the live global object/record rather than a detached object populated by a one-time globals.iter() copy. Ensure writes through globalThis create or update global bindings and later global additions remain visible through globalThis, while preserving the existing global and globalThis self-references.
331-338: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
DateandCollectionnow print as{ }.You folded
Date,Collection,BuiltinIterator, andScriptinto the generic named-property formatter. But this runtime deliberately keeps that state out of the property map —crates/bamts-runtime/src/intrinsics.rsasserts exactly that (assert!(machine.own_property_keys(date).unwrap().is_empty())indate_state_is_typed_and_unforgeable, and the same forMap). So the loop finds zero enumerable named properties and emits{ }.Node prints
1970-01-01T00:00:00.000Zfor a Date andMap(3) { 1 => 10, ... }for a Map. This file's own comments claim byte-exact Node v24.18.0 parity. Pick one: either honour the parity claim with per-variant formatting, or stop claiming it.🐛 Format the typed variants instead of falling through
- HeapEntry::Object { properties, .. } - | HeapEntry::Script { properties, .. } - | HeapEntry::Date { properties, .. } - | HeapEntry::BuiltinIterator { properties, .. } - | HeapEntry::Collection { properties, .. } => { + HeapEntry::Date { .. } => self.format_date(index), + HeapEntry::Collection { .. } => self.format_collection(index, depth), + HeapEntry::BuiltinIterator { .. } => Ok("Object [Array Iterator] {}".to_owned()), + HeapEntry::Object { properties, .. } + | HeapEntry::Script { properties, .. } => {📝 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.HeapEntry::Date { .. } => self.format_date(index), HeapEntry::Collection { .. } => self.format_collection(index, depth), HeapEntry::BuiltinIterator { .. } => Ok("Object [Array Iterator] {}".to_owned()), HeapEntry::Object { properties, .. } | HeapEntry::Script { properties, .. } => { if depth >= 2 { return Ok("[Object]".to_owned()); }🤖 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 `@crates/bamts-runtime/src/host_objects.rs` around lines 331 - 338, Update the formatter around the HeapEntry variant match to handle Date and Collection with dedicated Node-compatible formatting instead of sending them through the generic named-property loop; Date must render its ISO timestamp and Collection must render its type, size, and entries. Keep the generic property formatter for ordinary objects and other variants only where their properties are actually stored, preserving the existing depth handling and byte-exact parity claim.
395-401: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Every string in this file now takes a scenic route through UTF-16 and back. One root cause: a lossy conversion helper is being used to obtain bytes and strings that the code already had in hand, so each call is a transcode, an extra allocation, and — at the helper itself — a
expect()on an outcome that cannot fail.
crates/bamts-runtime/src/host_objects.rs#L395-L401: collapseenv_value_text_lossytotext.to_utf8_lossy(), deleting theinto_bytes()/String::from_utf8/expect(...)round trip, and rename it — nothing here concernsenvvalues.crates/bamts-runtime/src/host_objects.rs#L224-L228: drop theEcmaString::from_utf8(&line)hop entirely and handline.as_bytes()towrite_stdout/write_stderr;lineis already valid UTF-8.📍 Affects 1 file
crates/bamts-runtime/src/host_objects.rs#L395-L401(this comment)crates/bamts-runtime/src/host_objects.rs#L224-L228🤖 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 `@crates/bamts-runtime/src/host_objects.rs` around lines 395 - 401, The UTF-8 helpers perform unnecessary round trips and use an incorrect env-specific name. In crates/bamts-runtime/src/host_objects.rs lines 395-401, replace the implementation of env_value_text_lossy with direct text.to_utf8_lossy() and rename the helper to reflect its general purpose; update all callers accordingly. At lines 224-228, remove the EcmaString::from_utf8 conversion and pass line.as_bytes() directly to write_stdout and write_stderr.crates/bamts-runtime/src/intrinsics.rs (1)
318-323: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
You replaced a
BTreeMaplookup with a linear scan. On global resolution.
globalsis still aBTreeMap, and you now iterate the whole thing doing an ASCII comparison per entry, in an engine where global lookup is about as hot as it gets. The map is keyed byEcmaString— build the key and use it, or keep an ASCII side-index. Don't throw away the ordering you're already paying to maintain.Second problem:
debug_assert!(name.is_ascii())means a non-ASCII name is a loud bug in debug and a silentNonein release. Silent misses in global resolution are the worst possible failure mode.♻️ Use the map as a map
pub(crate) fn global(&self, name: &str) -> Option<Value> { - debug_assert!(name.is_ascii()); - self.globals - .iter() - .find_map(|(candidate, value)| candidate.eq_ascii(name).then_some(*value)) + self.globals.get(&EcmaString::from_utf8(name)).copied() }This only works if
EcmaString'sOrdmatches code-unit order for the ASCII subset — which it must, or theBTreeMapis already lying to you.📝 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.pub(crate) fn global(&self, name: &str) -> Option<Value> { self.globals.get(&EcmaString::from_utf8(name)).copied() }🤖 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 `@crates/bamts-runtime/src/intrinsics.rs` around lines 318 - 323, Update the global method to perform a BTreeMap lookup using an EcmaString key instead of iterating and comparing entries linearly; preserve the existing value-copy behavior. Remove the debug-only ASCII assumption and handle non-ASCII names consistently with the map lookup so resolution does not silently diverge between debug and release builds.crates/bamts-runtime/src/lib.rs (2)
2222-2255: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Linear binding scan on every module-scoped global store.
store_globalresolves cell →BindingIdwithbinding_cells.iter().position(...)before it even knows whether it needs to complain, so a module with N bindings pays an O(N) scan per assignment in the interpreter's hot path — and then leans onexpectto paper over a mapping that can't be inverted uniquely by construction.load_globalat least only scans on the TDZ path. Store the binding index alongside the cell when you link constants (L1279-1295) and both functions become O(1) lookups with noexpect.⚡ Keep the binding index, not just the cell
- constant_cells: Vec<Option<CellId>>, + /// Per constant: the module binding it names, and that binding's cell. + constant_cells: Vec<Option<(BindingId, CellId)>>,
load_global/store_globalthen read theBindingIddirectly instead of searchingbinding_cells.🤖 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 `@crates/bamts-runtime/src/lib.rs` around lines 2222 - 2255, Update constant linking around the module-linking logic near lines 1279-1295 to retain each resolved cell’s BindingId alongside the cell reference. Refactor load_global and store_global to read that stored binding index directly instead of scanning binding_cells with position; remove the expect-based reverse lookup while preserving the existing imported/namespace immutability check.
4239-4248: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Script-triggerable panic, duplicated: code-point iteration
expects that every code point is valid.EcmaStringdeliberately preserves unpaired surrogates, socode_points()yields0xD800for"\uD800"and a validatingpush_code_pointturns that into a process-killingexpect.[..."\uD800"]andArray.from("\uD800")are the two doors. The fix is the same in both places: build the piece from raw units (EcmaString::from_units/slice_units) instead of round-tripping through a code-point validator, and delete theexpect.
crates/bamts-runtime/src/lib.rs#L4239-L4248: replace the builder/push_code_pointmapping initerate_valueswith unit slicing between consecutive code-point offsets — the patterniterator_next(L4574-L4583) already uses correctly.crates/bamts-runtime/src/builtins/array.rs#L129-L143: same replacement inArray.from, or extract the corrected split into one shared helper and call it from both sites.📍 Affects 2 files
crates/bamts-runtime/src/lib.rs#L4239-L4248(this comment)crates/bamts-runtime/src/builtins/array.rs#L129-L143🤖 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 `@crates/bamts-runtime/src/lib.rs` around lines 4239 - 4248, Prevent surrogate-triggered panics in both iterate_values at crates/bamts-runtime/src/lib.rs#L4239-L4248 and Array.from at crates/bamts-runtime/src/builtins/array.rs#L129-L143 by replacing builder/push_code_point/expect mapping with raw-unit slicing between consecutive code-point offsets, following iterator_next’s existing approach. Preserve unpaired surrogates via EcmaString::from_units or slice_units, and remove the validating expect at both sites; a shared helper is acceptable.crates/bamts-runtime/src/native.rs (1)
1575-1601: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
You hand an untrusted
bytecode_pcstraight into a slice index.
shadow.bytecode_pccomes back fromNativeEntryTable::invoke— a public trait, handed a&mut ShadowFrame, so the value is whatever the implementation left there.error_at→Machine::error_at_in_moduledoesmetadata.code()[pc]. One out-of-range word and the host panics while it is trying to report an error. Even your own codegen writespc + 1as a resume token, solenis reachable without any malice. Clamp it at the boundary.🛡️ Clamp before it reaches the indexing
+ let reported_pc = |code_len: usize| { + (shadow.bytecode_pc as usize).min(code_len.saturating_sub(1)) + }; + let code_len = self.module(module).functions()[function].code().len();and use
reported_pc(code_len)in both theThrowandpending_fatal_kindarms instead ofshadow.bytecode_pc as usize.📝 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.let reported_pc = |code_len: usize| { (shadow.bytecode_pc as usize).min(code_len.saturating_sub(1)) }; let code_len = self.module(module).functions()[function].code().len(); Ok(CompletionTag::Throw) => { let origin = self .pending_throw .take() .map_or(ThrowOrigin::Bytecode, |pending| pending.origin); Err(NativeError::Runtime(self.error_at( module, RuntimeErrorKind::UncaughtThrow { value: out.value, origin, }, function, reported_pc(code_len), ))) } Ok(CompletionTag::Suspend | CompletionTag::FatalTrap) => { if let Some(error) = self.pending_abi_error.take() { Err(NativeError::Abi(error)) } else if let Some(error) = self.pending_error.take() { Err(NativeError::Runtime(error)) } else if let Some(kind) = self.pending_fatal_kind.take() { Err(NativeError::Runtime(self.error_at( module, kind, function, reported_pc(code_len), )))🤖 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 `@crates/bamts-runtime/src/native.rs` around lines 1575 - 1601, Clamp shadow.bytecode_pc before passing it to error_at, using reported_pc(code_len) in both the CompletionTag::Throw and pending_fatal_kind error paths. Ensure the computed value cannot exceed the code length, including the reachable resume token at len, and replace the direct shadow.bytecode_pc as usize usage in both arms.crates/bamts-runtime/src/regexp.rs (3)
244-263: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Look-behind rescans the entire input from index 0, on every attempt.
For each evaluation you restart the body match at every code point from
0up tostate.position.execalready evaluates the pattern at every start position, so a pattern like(?<=a)bis O(n²) body matches over the input — andmatch_nodereturnsVec<State>, meaning you materialize every backtracking alternative rather than exploring lazily. Feed this(?<=(a+)+)bfrom user-supplied JS and you don't get slow, you get OOM.The standard fix is to match the look-behind body right-to-left from
state.position, not to brute-force every start offset.🤖 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 `@crates/bamts-runtime/src/regexp.rs` around lines 244 - 263, Replace the behind branch in the look-behind handling around match_node with a right-to-left evaluation starting at state.position, rather than rescanning every code-point boundary from zero. Ensure the body is traversed backward while preserving the existing look-behind match-position constraint, and avoid materializing alternatives for every candidate start. Keep forward matching and existing state/backtracking behavior unchanged.
467-471: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
ECMAScript Canonicalize regexp IgnoreCase non-unicode ASCII code unit rule💡 Result:
In ECMAScript, the Canonicalize abstract operation determines how characters match when the ignoreCase (i) flag is present [1][2]. The behavior differs significantly depending on whether the RegExp is Unicode-aware (using the u or v flag) or Unicode-unaware [2][3]. For Unicode-unaware regular expressions (no u or v flag), ECMAScript uses the Unicode Default Case Conversion algorithm, which is the same as String.prototype.toUpperCase [2][3]. A critical rule in this mode is that it prevents code points outside the Basic Latin block (ASCII) from being mapped to code points within it [2][3]. Specifically, if an input character has a numeric value of 128 or greater (non-ASCII), but its upper-cased equivalent has a numeric value less than 128 (ASCII), the algorithm returns the original character rather than the ASCII conversion [1]. This ensures compatibility with older ECMAScript standards (ES5 and earlier) by preventing unexpected case folding for non-ASCII characters [4]. In contrast, Unicode-aware regular expressions (with u or v flag) utilize Unicode simple case folding as defined in the Unicode Character Database (CaseFolding.txt) [2][3]. This algorithm does not have the same restriction against folding non-ASCII characters into the ASCII range; for example, it allows U+017F (LATIN SMALL LETTER LONG S) to fold to U+0073 (LATIN SMALL LETTER S) and U+212A (KELVIN SIGN) to fold to U+006B (LATIN SMALL LETTER K) [2][3]. Summary of Key Rules for Non-Unicode-Aware (i-only) mode: 1. It uses Unicode Default Case Conversion (effectively toUpperCase) [2][3]. 2. If the conversion result consists of multiple code units, the original character is returned [1][4]. 3. If the input character is >= 128 (non-ASCII) but its conversion result is < 128 (ASCII), the original character is returned, effectively preventing it from folding into the Basic Latin range [1].
Citations:
- 1: https://tc39.es/proposal-regexp-modifiers/
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/ignoreCase
- 3: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/RegExp/ignoreCase
- 4: https://tc39.github.io/archives/bugzilla/2256/
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="crates/bamts-runtime/src/regexp.rs" echo "== outline ==" ast-grep outline "$FILE" --view expanded | sed -n '1,220p' echo echo "== relevant lines around fold and range matching ==" sed -n '420,500p' "$FILE" | cat -n echo echo "== tests around line 905 ==" sed -n '860,940p' "$FILE" | cat -nRepository: metaphorics/bamTiScript
Length of output: 10987
foldis the wrong operation here.
- ECMAScript
Canonicalizeuses uppercase/default case conversion, and in non-/umode it must not fold non-ASCII into ASCII.U+212Amust stay put;U+017Fis only relevant as a/ufold, not a no-/uexample.ClassItem::Rangealso folds the endpoints. That can invert[A-_]/iinto an empty range and shrink[A-z]/iso it drops[\\]^_. Canonicalize the candidate only; keep the range bounds raw.🤖 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 `@crates/bamts-runtime/src/regexp.rs` around lines 467 - 471, Replace the lowercasing logic in fold with ECMAScript Canonicalize behavior: use uppercase/default conversion and preserve non-ASCII characters in non-/u mode, including U+212A. Update ClassItem::Range handling so only each candidate value is canonicalized; keep the range endpoints raw to preserve ranges such as [A-_]/i and [A-z]/i.
602-616: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
new RegExp("[]")and[^]now throw. That's not JavaScript, that's POSIX.The
!firstguard means the very first]inside a class is swallowed as a literal character instead of closing it. ECMAScriptClassRangesmay be empty:[]is a valid class that matches nothing, and[^]is the idiomatic "match absolutely anything including newlines". Both of those walk straight off the end of the unit stream and land onErr("unterminated character class").Neither case appears in your test module, which is why this survived a full rewrite.
🐛 Let the class close immediately
let mut items = Vec::new(); - let mut first = true; while let Some(token) = self.peek() { - if token == 0x5d && !first { + if token == 0x5d { self.position += 1; return Ok(CharacterClass { negated, items }); } - first = false; let left = self.parse_class_item()?;And add the coverage:
#[test] fn empty_and_negated_empty_classes_are_valid() { assert!(regex("[]", "").exec(&text("a"), 0).is_none()); assert_eq!(regex("[^]", "").exec(&text("\n"), 0).unwrap().range, 0..1); }📝 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.fn parse_class(&mut self) -> Result<CharacterClass, RegexError> { let negated = if self.peek() == Some(0x5e) { self.position += 1; true } else { false }; let mut items = Vec::new(); while let Some(token) = self.peek() { if token == 0x5d { self.position += 1; return Ok(CharacterClass { negated, items }); }🤖 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 `@crates/bamts-runtime/src/regexp.rs` around lines 602 - 616, Update parse_class so a closing ] terminates the class even when it is the first token, allowing empty [] and negated empty [^] classes to parse correctly. Preserve the existing negated behavior and add coverage for both patterns, verifying [] matches nothing and [^] matches a newline.crates/bamts-verification/src/corpus.rs (1)
803-819: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Does the driver or runtime expose a fuel/deadline knob usable as a JIT bound? rg -nP -C3 'fuel|deadline|max_steps|Limits\s*\{' crates/bamts-cli/src/driver.rs crates/bamts-cli/src/args.rsRepository: metaphorics/bamTiScript
Length of output: 161
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant files first, then inspect the small slices around the JIT/AOT timeout handling. ast-grep outline crates/bamts-verification/src/corpus.rs --view expanded >/tmp/corpus_outline.txt || true ast-grep outline crates/bamts-cli/src/driver.rs --view expanded >/tmp/driver_outline.txt || true ast-grep outline crates/bamts-cli/src/args.rs --view expanded >/tmp/args_outline.txt || true printf '\n## corpus.rs outline\n' sed -n '1,220p' /tmp/corpus_outline.txt printf '\n## driver.rs outline\n' sed -n '1,220p' /tmp/driver_outline.txt printf '\n## args.rs outline\n' sed -n '1,220p' /tmp/args_outline.txt printf '\n## corpus.rs lines around run_jit and related code\n' nl -ba crates/bamts-verification/src/corpus.rs | sed -n '760,860p' printf '\n## driver.rs relevant text search\n' rg -n -C 4 'execute\(|run_process|timeout|child|kill|spawn|fuel|deadline|max_steps|limit|Limit|Limits' crates/bamts-cli/src/driver.rs crates/bamts-cli/src/args.rs crates/bamts-cli/src -g '!**/target/**' || trueRepository: metaphorics/bamTiScript
Length of output: 17611
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for the timeout used by the verifier and the AOT subprocess boundary. rg -n -C 3 'spec\.timeout\(\)|run_process\(|ExecutionTarget::Jit|ExecutionTarget::Aot|driver::execute|started\.elapsed\(\)' crates/bamts-verification/src crates/bamts-cli/src -g '!**/target/**' || trueRepository: metaphorics/bamTiScript
Length of output: 9172
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the exact implementation slices if the outlines are available. printf '\n## corpus.rs slice\n' nl -ba crates/bamts-verification/src/corpus.rs | sed -n '780,845p' printf '\n## driver.rs slice\n' nl -ba crates/bamts-cli/src/driver.rs | sed -n '1,260p' printf '\n## args.rs slice\n' nl -ba crates/bamts-cli/src/args.rs | sed -n '1,260p'Repository: metaphorics/bamTiScript
Length of output: 221
🏁 Script executed:
#!/bin/bash set -euo pipefail # Pin down whether JIT is in-process and whether any killable boundary exists. printf '\n## corpus.rs around the JIT/AOT split\n' nl -ba crates/bamts-verification/src/corpus.rs | sed -n '780,860p' printf '\n## driver.rs implementation\n' nl -ba crates/bamts-cli/src/driver.rs | sed -n '1,260p' printf '\n## search for process boundary helpers\n' rg -n -C 3 'run_process|Command::new|spawn\(|wait\(|kill\(|timeout|deadline|fuel|max_steps|ExecutionTarget::Jit|ExecutionTarget::Aot' crates/bamts-cli/src crates/bamts-verification/src -g '!**/target/**' || trueRepository: metaphorics/bamTiScript
Length of output: 240
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## corpus.rs JIT/AOT slice\n' sed -n '793,848p' crates/bamts-verification/src/corpus.rs printf '\n## corpus.rs process timeout helper slice\n' sed -n '1022,1135p' crates/bamts-verification/src/corpus.rs printf '\n## driver.rs run/execute slice\n' sed -n '255,455p' crates/bamts-cli/src/driver.rsRepository: metaphorics/bamTiScript
Length of output: 12664
Add a killable timeout to JIT runs
crates/bamts-verification/src/corpus.rs:803-819—driver::execute(&args)runs in-process, and the timeout is checked only after it returns. A looping JIT case can hang the harness forever. Pass a real deadline/fuel budget into the JIT path, or run it behind a killable subprocess boundary; thiselapsed()flag is not a bound.🤖 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 `@crates/bamts-verification/src/corpus.rs` around lines 803 - 819, Update run_jit so spec.timeout() is enforced during execution rather than only converted into the post-run elapsed() flag. Use the existing driver/JIT timeout or fuel mechanism if available, or move execution behind a killable subprocess boundary, ensuring looping cases terminate and still produce the appropriate driver_outcome timeout result.crates/bamts/src/lib.rs (1)
197-211: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unbounded ancestor walk for
tsconfig.jsonmakes the project root depend on the filesystem above your project.
path.ancestors().skip(1)climbs all the way to/. The first straytsconfig.jsonanywhere above the entrypoint silently becomes the project root, which then decides which filesProgramLoaderis even allowed to see. On a shared CI box a/tmp/tsconfig.jsonchanges the compilation result of the doctest above and oftests::fixture, and for a repo that advertises determinism and verified programs, "the answer depends on what's in my home directory" is not an acceptable resolution rule.Bound the search: stop at the first directory containing a project marker (
bamts.toml/.git), or take the root explicitly as an argument.🤖 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 `@crates/bamts/src/lib.rs` around lines 197 - 211, The root discovery logic around config_path and root_path must not search unbounded ancestors for tsconfig.json. Bound the search at the first project marker directory containing bamts.toml or .git, and resolve tsconfig.json only within that bounded range; preserve the existing parent-directory fallback when no config is found.
There was a problem hiding this comment.
Actionable comments posted: 21
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
crates/bamts-runtime/src/builtins/mod.rs (1)
598-620: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
error_constructorreusesthiseven when it was never called withnew.The
_constructingflag is discarded, soError.call(existingErrorInstance, "boom")now stompsmessage,cause, andstackon a live object instead of allocating a fresh one. Per spec, callingErroras a plain function is equivalent tonew Error—NewTargetis the active function, never the caller's receiver. Thethis-reuse path exists to makesuper(...)from a subclass work; gate it onconstructing(or on a realnew.targetcheck) so the function-call path always allocates.🐛 Gate receiver reuse on construction
-fn error_constructor<H: Host>( - machine: &mut Machine<'_, H>, - this: Value, - args: &[Value], - _constructing: bool, -) -> Result<BuiltinOutcome, EvalFailure> { +fn error_constructor<H: Host>( + machine: &mut Machine<'_, H>, + this: Value, + args: &[Value], + constructing: bool, +) -> Result<BuiltinOutcome, EvalFailure> { @@ - let object = if machine.inherits_from_prototype(this, prototype)? { + let object = if constructing && machine.inherits_from_prototype(this, prototype)? { this } else {🤖 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 `@crates/bamts-runtime/src/builtins/mod.rs` around lines 598 - 620, Update error_constructor to use the _constructing flag when deciding whether to reuse this: only reuse a receiver that inherits from the error prototype during construction, while plain function calls always allocate a fresh error object. Preserve the existing allocation and prototype behavior for constructed calls.crates/bamts-runtime/src/builtins/array.rs (2)
135-145: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMapper runs after the iterator is fully drained — observable ordering divergence.
The spec interleaves: next() → mapfn → next(). Here the iterator is exhausted first, then the mapper runs over the collected vector. A mapper that throws on element 0 must stop iteration (and trigger IteratorClose); with this code the iterator has already run to completion. Same problem for mappers that observe/mutate the source mid-iteration. The new
array_from_preserves_mapper_order_and_thisargtest can't catch it because the mapper is pure.🤖 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 `@crates/bamts-runtime/src/builtins/array.rs` around lines 135 - 145, The Array.from mapping path around iterable_values must apply the callback immediately as each iterator element is consumed, rather than collecting all elements before mapping. Update the iteration flow so next() and callback execution are interleaved, propagating callback errors at the current element and preserving IteratorClose behavior; retain the existing this_arg and index semantics.
135-1164: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRestore
Array.fromarray-like support
iterable_values(source)only followsSymbol.iterator, soArray.from({ length: 3 })and similar array-likes now throw instead of being expanded. The newarray_from_rejects_non_iterabletest bakes in that regression; restore the array-like fallback and update the test to expect array-like output.🤖 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 `@crates/bamts-runtime/src/builtins/array.rs` around lines 135 - 1164, Update the Array.from implementation around iterable_values and its caller to fall back to array-like conversion when the source lacks Symbol.iterator, using the source length and indexed values. Preserve iterator precedence when a custom iterator exists, and replace array_from_rejects_non_iterable with coverage asserting an object such as { length: 3 } produces the expected array-like result.crates/bamts-runtime/src/intrinsics.rs (1)
370-375: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
global()is now a linear scan with a per-key string compare, on a hot path.You traded
BTreeMap<String>::get(O(log n), one compare per level) foriter().find_map(eq_ascii)over every global in the realm. And this isn't a bootstrap-only helper:constructor_prototypeinuint8array.rs/collections.rs,date::constructor, andrequire_promise_constructorinpromise.rsall call it per operation. Everynew Date(), everyPromise.resolve, everyUint8Array.prototype.joinnow walks the whole global table.Keep an ASCII-keyed index (
BTreeMap<&'static str, Value>or aVec<(EcmaString, Value)>sorted for binary search) alongside, or cache the handful of constructors these call sites need inBuiltinTable— you already do exactly that forobject_to_string,iterator_prototype, and the Promise targets.#!/bin/bash # How many call sites hit intrinsics.global on non-bootstrap paths? rg -nP --type=rust -C1 '\.global\(' crates/bamts-runtime/src | rg -v '#\[test\]' | head -60🤖 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 `@crates/bamts-runtime/src/intrinsics.rs` around lines 370 - 375, Restore efficient lookup in Intrinsics::global instead of scanning every global with eq_ascii. Add or reuse an ASCII-keyed index, such as a sorted structure supporting binary search, or cache the frequently requested constructors in BuiltinTable alongside object_to_string, iterator_prototype, and the Promise targets. Update global and its initialization paths while preserving the existing Option<Value> behavior.crates/bamts-runtime/src/host_objects.rs (1)
337-344: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFolding
Collection,Promise,GeneratorandTimeoutinto the plain-object arm makesconsole.loglie.Those entries keep their payload in typed hidden state, not in
properties— that's the whole point of the newHeapEntry::Collection. So this arm iterates an empty property map and prints{ }. Node printsMap(1) { 1 => 2 },Set(2) { 1, 2 },Promise { <pending> },Timeout { ... }. For a runtime whose gate is byte-differential corpus output against Node, silently rendering everyMapas{ }is a real defect, not a cosmetic one. Give each of these variants its own arm.🐛 Sketch
+ HeapEntry::Collection { entries, .. } => { + // render `Map(n) { k => v, ... }` / `Set(n) { v, ... }` + // from the typed entries, honouring the depth limit. + } + HeapEntry::Promise { .. } => Ok("Promise { <pending> }".to_owned()), HeapEntry::Object { properties, .. } | HeapEntry::Script { properties, .. } | HeapEntry::Date { properties, .. } | HeapEntry::BuiltinIterator { properties, .. } - | HeapEntry::Collection { properties, .. } | HeapEntry::Generator { properties, .. } - | HeapEntry::Promise { properties, .. } | HeapEntry::Timeout { properties, .. } => {#!/bin/bash # Does any corpus program console.log a Map/Set/Promise? rg -nP -C2 'console\.log\([^)]*(new (Map|Set)|Promise)' corpus🤖 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 `@crates/bamts-runtime/src/host_objects.rs` around lines 337 - 344, Separate Collection, Generator, Promise, and Timeout from the plain-object match arm in the relevant heap-entry formatting logic. Add dedicated handling for each variant that reads its typed hidden state and renders Node-compatible output, including Map/Set contents and Promise/Timeout status, while leaving ordinary object, script, Date, and BuiltinIterator property formatting unchanged.crates/bamts-runtime/src/builtins/string.rs (1)
87-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate symbol coercion on the call path, not construction.
string_constructor_text()clearly does the symbol-descriptive-string shortcut, butbuiltins/string.rsuses it unconditionally before checkingconstructing. That meansnew String(Symbol("x"))will wrap"Symbol(x)"instead of going throughToStringand throwingTypeError.🤖 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 `@crates/bamts-runtime/src/builtins/string.rs` around lines 87 - 97, Update the argument-conversion flow around string_constructor_text so symbol coercion is selected by the constructing flag: constructed String objects must use ordinary ToString semantics and throw TypeError for symbols, while the non-constructing call path retains the symbol descriptive-string behavior. Keep allocation and result boxing unchanged.crates/bamts-bytecode/src/lib.rs (1)
1943-1944: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe string-unit ceiling is a wire-format contract copy-pasted across two crates. The decoder owns the limit but doesn't export it, so the compiler hardcodes
1 << 20a second time and merely comments that it matches. Change one and the compiler emits modules its own decoder rejects, with no build error.
crates/bamts-bytecode/src/lib.rs#L1943-L1944: hoist the value into apub const MAX_STRING_UNITS: u32 = 1 << 20;besideMAX_CONSTANTS/MAX_FUNCTIONSand reference it fromDecodeLimits::default().crates/bamts-compiler/src/lower.rs#L107-L109: delete the localconst MAX_STRING_UNITSand import the bytecode constant instead of restating the ceiling in a doc comment.crates/bamts-compiler/src/lower.rs#L437-L441: comparevalue.len_units()against the imported constant so the>boundary provably matches the decoder'sboundedcheck.🤖 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 `@crates/bamts-bytecode/src/lib.rs` around lines 1943 - 1944, Centralize the string-unit ceiling as pub const MAX_STRING_UNITS: u32 = 1 << 20 beside MAX_CONSTANTS/MAX_FUNCTIONS in crates/bamts-bytecode/src/lib.rs, and use it in DecodeLimits::default(). In crates/bamts-compiler/src/lower.rs at lines 107-109, remove the local constant and import the bytecode MAX_STRING_UNITS; at lines 437-441, use that imported constant when comparing value.len_units().
🟡 Other comments (7)
crates/bamts-verification/tests/corpus_differential.rs-43-48 (1)
43-48: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBoth generated fixtures assume
<workspace>/targetexists. The build directory is relocatable viaCARGO_TARGET_DIR/--target-dir, sofs::writecan fail withNotFoundand both tests die on a misleadingexpectmessage instead of a real diagnosis.
crates/bamts-verification/tests/corpus_differential.rs#L43-L48: callfs::create_dir_allonpath.parent()before writing the self-capture fixture.crates/bamts-verification/tests/corpus_differential.rs#L96-L99: apply the samecreate_dir_allbefore writing the local-TDZ fixture.🤖 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 `@crates/bamts-verification/tests/corpus_differential.rs` around lines 43 - 48, Ensure the generated fixture directory exists before writing files: in crates/bamts-verification/tests/corpus_differential.rs at lines 43-48, call fs::create_dir_all on path.parent() before the self-capture fixture’s fs::write, and apply the same change at lines 96-99 before writing the local-TDZ fixture.crates/bamts-verification/src/workspace_guard.rs-438-443 (1)
438-443: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
script-compilernow also enablesdep:bamts-bytecode, which makes the graph comment a lie.The expectation block for
bamts-node -> bamts-bytecodestill claims it is "Enabled only byaot-main". Two features enable it now. In a file whose entire job is to be an executable spec, a comment that contradicts the assertions two hundred lines below it is worse than no comment.📝 Fix the stale annotation
- // Enabled only by `aot-main`, which decodes the canonical bytecode - // embedded in a linked AOT image before handing it to the engine. + // Enabled by `aot-main`, which decodes the canonical bytecode + // embedded in a linked AOT image, and by `script-compiler`, which + // emits bytecode for host-compiled classic scripts. ("bamts-bytecode".to_owned(), internal_dependency(true, &[])),🤖 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 `@crates/bamts-verification/src/workspace_guard.rs` around lines 438 - 443, Update the stale expectation annotation for the bamts-node → bamts-bytecode dependency graph to state that both aot-main and script-compiler enable it. Keep the existing require_feature_set assertion unchanged and ensure the comment matches the feature relationships it documents.crates/bamts-runtime/src/lib.rs-2936-2953 (1)
2936-2953: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
instantiate_modulesrebuildsregistry.modulesfrom the static program and drops any installed script rows — guarded only by adebug_assert.
install_script_reservingpushes aModuleInstanceper dynamic module and assertsregistry.modules.len() == dynamic_base + dynamic.len(). Then this reassignment collects onlyprogram.modules(), so if instantiation ever runs after a script install,self.dynamicstill holds the script whileregistry.modulesno longer has its row — and the nextregistry.modules[module.get() as usize]is an out-of-bounds panic in release. Adebug_assert!documents the ordering; it doesn't enforce it where it matters.Either make the ordering unrepresentable (instantiate during construction) or preserve the dynamic tail instead of discarding it.
🛡️ Preserve the dynamic tail
- self.registry.modules = program + let dynamic_tail = self.registry.modules.split_off(self.dynamic_base.min(self.registry.modules.len())); + self.registry.modules = program .modules() .iter() .map(|module| ModuleInstance { binding_cells: vec![None; module.bindings.len()], constant_cells: vec![None; module.code.constants().len()], namespace: None, state: ModuleState::Unevaluated, }) .collect(); + self.registry.modules.extend(dynamic_tail);🤖 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 `@crates/bamts-runtime/src/lib.rs` around lines 2936 - 2953, Update instantiate_modules so rebuilding registry.modules preserves the existing dynamic module tail recorded by self.dynamic, rather than collecting only program.modules(). Retain the static module initialization behavior and append or reuse each installed script row so registry.modules remains aligned with dynamic_base + dynamic.len() even if instantiation occurs after install_script_reserving.crates/bamts-runtime/src/lib.rs-4612-4622 (1)
4612-4622: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCollapse the nested
if; clippy is complaining and it reads better.♻️ Proposed fix
- if let HeapEntry::Function { - module, function, .. - } = self.heap[index] - { - if self.module_code(module).functions()[function.get() as usize] - .flags() - .is_async - { - return self.throw_type("construct", call_pc); - } - } + if let HeapEntry::Function { + module, function, .. + } = self.heap[index] + && self.module_code(module).functions()[function.get() as usize] + .flags() + .is_async + { + return self.throw_type("construct", call_pc); + }🤖 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 `@crates/bamts-runtime/src/lib.rs` around lines 4612 - 4622, Collapse the nested condition in the heap-entry handling around the HeapEntry::Function match by combining the function-pattern check with the async flag check. Preserve the existing self.throw_type("construct", call_pc) return behavior for async functions and leave non-function or non-async entries unchanged.Source: Linters/SAST tools
crates/bamts-runtime/src/builtins/date.rs-46-51 (1)
46-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSingle-argument
Datenever goes through ToPrimitive.
machine.string_value(value)only matches a rawHeapEntry::String. A boxedStringobject, or any object whose@@toPrimitive/valueOf/toStringyields a date string, silently falls intoto_numberand becomesNaN. Spec order is: ToPrimitive(value) first, then branch on whether the result is a String. Route through the primitive conversion you already have and branch on the result.🤖 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 `@crates/bamts-runtime/src/builtins/date.rs` around lines 46 - 51, Update the single-argument path in the Date implementation to apply the existing ToPrimitive conversion before deciding how to parse the value. Branch on the resulting primitive: parse ISO dates for strings and use numeric conversion otherwise, preserving the existing NaN behavior for invalid date strings.crates/bamts-runtime/src/external_modules.rs-175-180 (1)
175-180: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError-string inconsistency for the same Node error code.
The detached-dash-value paths in this same function use
"ERR_PARSE_ARGS_INVALID_OPTION_VALUE"as the operation text, but the boolean-inline path invents"boolean parseArgs option has a value". Node raisesERR_PARSE_ARGS_INVALID_OPTION_VALUEfor both. Pick one convention so diagnostics are greppable.🐛 Align the operation string
if let Some(text) = inline { if config.strict { - return Err(type_error("boolean parseArgs option has a value")); + return Err(type_error("ERR_PARSE_ARGS_INVALID_OPTION_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 `@crates/bamts-runtime/src/external_modules.rs` around lines 175 - 180, The boolean-inline branch in the parseArgs handling function uses an inconsistent type_error operation string. Replace its `"boolean parseArgs option has a value"` text with the existing `"ERR_PARSE_ARGS_INVALID_OPTION_VALUE"` convention used by the detached-dash-value paths, while preserving the strict-mode error behavior.crates/bamts-compiler/src/lower.rs-1885-1890 (1)
1885-1890: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winStop cloning the switch bodies
StmtisNode<Statement>, andNodeowns the subtree, so.iter().cloned()clones the whole case body just to flatten it. Pass borrowed statements intoinstantiate_declarationsinstead; the same pattern in the scanner has the same waste.🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 1885 - 1890, Update the switch handling around instantiate_declarations to flatten borrowed case consequent statements instead of using iter().cloned(), avoiding deep cloning of Stmt subtrees. Adjust instantiate_declarations as needed to accept borrowed statements while preserving declaration instantiation behavior, and apply the same borrowing pattern in the scanner’s corresponding switch-body handling.
🧹 Nitpick comments (17)
crates/bamts/src/lib.rs (2)
78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Diagnosticsprints a count and throws the actual diagnostics away.Anyone consuming this facade as a library gets "program has 3 error diagnostic(s)" and nothing else —
source()returnsNonetoo (Line 97), so the detail is unreachable through theErrortrait. The CLI has its own renderer; every other embedder is left blind. Render at least the messages.♻️ Include the diagnostic text
- Self::Diagnostics { diagnostics } => write!( - formatter, - "program has {} error diagnostic(s)", - diagnostics.len() - ), + Self::Diagnostics { diagnostics } => { + write!( + formatter, + "program has {} error diagnostic(s)", + diagnostics.len() + )?; + for diagnostic in diagnostics { + write!(formatter, "\n {}", diagnostic.message())?; + } + Ok(()) + }Adjust
message()to whatever the diagnostic accessor is actually called.🤖 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 `@crates/bamts/src/lib.rs` around lines 78 - 82, Update the Diagnostics Display implementation in the Self::Diagnostics match arm to include each diagnostic’s message text, not only diagnostics.len(). Use the diagnostics accessor actually provided by the diagnostic type, and preserve the existing count/context while making all diagnostic details reachable through the Error display.
334-391: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixtures leak on the failure path.
Every
?betweenscript_fixtureandremove_fixture—compile_source_file,evaluate,drain_microtasks,wait_for_timer_expiry— returns early and leaves the temp directory behind. Not fatal (script_fixturewipes the directory up-front on the next run), but a guard type with aDropimpl, like the one incrates/bamts-cli/tests/cli.rsLines 406-408, would make this unconditional and delete the manual cleanup calls scattered across ~15 tests.🤖 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 `@crates/bamts/src/lib.rs` around lines 334 - 391, Make fixture cleanup unconditional across the checkpoint helpers and related tests by introducing a guard type with a Drop implementation, following the existing guard pattern in the CLI tests. Update script_fixture usage in run_microtask_checkpoint, run_timer_checkpoint, and the other affected tests to retain the guard for the fixture directory, remove manual remove_fixture calls, and ensure cleanup still occurs when compile, evaluation, or timer operations return an error.crates/bamts-cli/build.rs (1)
66-78: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrecompute the mtime before sorting.
fs::metadata()here makes the comparator do repeated syscalls, and if a file changes mid-sort the ordering can stop being a total order. Cache(modified, path)once per candidate, then sort the pairs and unwrap the path.🤖 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 `@crates/bamts-cli/build.rs` around lines 66 - 78, Update the compatible-candidate sorting flow to precompute each candidate’s modification time once before sorting, storing it together with the candidate path. Sort these cached pairs using the modification time and existing path tie-breaker, then unwrap or extract the paths for the final compatible list; remove filesystem metadata calls from the comparator.crates/bamts-runtime/src/native.rs (1)
1063-1146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe callee trampoline has no fuel charge and no iteration ceiling.
Every
BuiltinOutcome::Callcontinuation and everyCalleeKind::Boundflatten loops back around here without consuming fuel or bumping any counter. One builtin that hands back a continuation naming its own callee, or a bound target that resolves to another bound object indefinitely, and this thread spins forever inside adispatchthat the fuel limiter can no longer interrupt. Fuel exists precisely so no bytecode-reachable path can hang the process; don't leave a hole in it.♻️ Charge the trampoline
loop { + if let Err(kind) = self.machine.borrow_mut().consume_fuel(1) { + self.pending_fatal_kind.set(Some(kind)); + return InvokeOutcome::Fatal; + } let kind = self.machine.borrow().callee_kind(callee);🤖 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 `@crates/bamts-runtime/src/native.rs` around lines 1063 - 1146, Update the callee trampoline loop around CalleeKind dispatch to consume fuel and enforce the runtime’s existing iteration-limit behavior on every iteration, including BuiltinOutcome::Call continuations and CalleeKind::Bound flattening. Ensure exhaustion exits through the established fuel-limit outcome instead of allowing repeated self-continuations or bound targets to spin indefinitely.crates/bamts-runtime/src/vm.rs (1)
219-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePick one lookup style for the
node:vmexport.
run_in_this_contexttwo functions up uses.get(...).and_then(...).expect("node:vm installs runInThisContext"); this one indexes the maps directly, so a wiring mistake surfaces as a bareBTreeMapindex panic with no message instead of a named expectation. Same invariant, same failure mode — make them read the same.🤖 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 `@crates/bamts-runtime/src/vm.rs` around lines 219 - 222, Update the `runInNewContext` lookup in the `constructing` branch to match `run_in_this_context`: use `.get(...).and_then(...)` and an explicit `.expect(...)` describing the required `node:vm` export, rather than direct map indexing.crates/bamts-runtime/src/host_objects.rs (1)
59-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
24.18.0is written twice; deriveprocess.versionfrom it.Two literals that must stay in lockstep will not stay in lockstep. One
const NODE_VERSION: &str = "24.18.0";andformat!("v{NODE_VERSION}").🤖 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 `@crates/bamts-runtime/src/host_objects.rs` around lines 59 - 67, Define a single NODE_VERSION constant for the Node version and reuse it in both the versions.node assignment and process.version construction; build process.version with the required “v” prefix via format!("v{NODE_VERSION}") instead of duplicating the literal.crates/bamts-runtime/src/regexp.rs (1)
833-837: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect — and the laziness matters.
then_some((byte - b'0') as usize)would evaluate the subtraction for every unit, so any unit belowb'0'(sayb'!') underflows and panics in a debug build.then(|| …)only runs it onceis_ascii_digitholds. Keep it; a// lazy: avoids u8 underflow on non-digitsnote would stop someone "simplifying" it back.🤖 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 `@crates/bamts-runtime/src/regexp.rs` around lines 833 - 837, Preserve the lazy closure in decimal_digit when converting an ASCII digit to usize: keep is_ascii_digit().then(|| (byte - b'0') as usize) rather than replacing it with then_some, so subtraction is evaluated only for valid digits. Add a concise comment documenting that laziness prevents u8 underflow on non-digits.crates/bamts-runtime/src/intrinsics.rs (1)
196-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeven copies of
set_x/x()+expect("… install …").
generator_prototype,promise_prototype, and the threepromise_*_targetspairs are byte-for-byte the same shape as the four slots above them, and the struct now carries elevenOption<Value>realm slots. A small macro (or aRealmSlot<T>newtype holding the panic message) collapses ~90 lines and makes the next slot a one-liner instead of another copy-paste.🤖 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 `@crates/bamts-runtime/src/intrinsics.rs` around lines 196 - 239, Consolidate the repeated setter/getter and installation-check logic for generator_prototype, promise_prototype, and the promise_*_targets slots using a shared macro or RealmSlot<T> abstraction. Preserve each slot’s existing value type, setter/getter API, and panic message while reducing the duplicated Option storage and access patterns; update the surrounding realm slot definitions and methods consistently.crates/bamts-runtime/src/builtins/promise.rs (1)
67-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOnly
resolve/reject/all— norace,allSettled,any, orSymbol.species.That's a defensible subset for a bounded runtime, but nothing in the file says so. Since
require_promise_constructoralready hard-codes "the one true Promise", add a short module comment stating which combinators the realm intentionally omits so a corpus failure onPromise.racereads as "not implemented" rather than "broken".🤖 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 `@crates/bamts-runtime/src/builtins/promise.rs` around lines 67 - 80, Add a concise module-level comment near the Promise built-in definitions documenting that the runtime intentionally supports only resolve, reject, and all, and omits race, allSettled, any, and Symbol.species. Keep the existing implementation unchanged and align the note with require_promise_constructor’s single Promise model.crates/bamts-runtime/src/builtins/collections.rs (1)
305-334: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoff
Map/Setconstruction bypasses the observableset/addmethods.
AddEntriesFromIterable(§24.1.1.2) doesGet(target, "set")once, checks callability, and calls it for every entry;Setlikewise callsadd. Here you jump straight intomap_put/set_put, so a subclass overridingset/add, or a monkey-patchedMap.prototype.set, is invisible. Same class of divergence as theforEachsnapshot problem. Worth a comment recording the deliberate deviation if you're not fixing it now.Also applies to: 336-360
🤖 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 `@crates/bamts-runtime/src/builtins/collections.rs` around lines 305 - 334, Update map_like_constructor and the corresponding Set construction path to obtain the target’s set/add method once, validate that it is callable, and invoke it for each iterable entry instead of calling map_put/set_put directly. Preserve existing entry validation and WeakMap key checks, and ensure overridden or monkey-patched methods are observed during construction.crates/bamts-runtime/src/builtins/regexp.rs (1)
57-75: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStop compiling the same pattern twice and stop leaning on
expectto paper over it.Line 57 already compiles and validates; Line 69 throws that away and re-parses the entire pattern just to read
flags().canonical(). That is a gratuitous second full parse on everynew RegExp, and the.expect("validated")turns any future divergence between the two calls into a runtime panic inside a builtin. Keep the firstRegexand read the flags off it.♻️ Reuse the validated compile
- compile(machine, &pattern, &flags)?; + let compiled = compile(machine, &pattern, &flags)?; + let canonical_flags = compiled.flags().canonical(); let mut properties = PropertyMap::default(); for (name, value, writable) in [ ( "source", allocate_string(machine, canonical_source(&pattern))?, false, ), ( "flags", - allocate_string( - machine, - Regex::compile(&pattern, &flags) - .expect("validated") - .flags() - .canonical(), - )?, + allocate_string(machine, canonical_flags)?, false, ),🤖 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 `@crates/bamts-runtime/src/builtins/regexp.rs` around lines 57 - 75, Update the RegExp construction flow around the initial compile call to retain the validated Regex value, then use that value to obtain the canonical flags for the “flags” property. Remove the second Regex::compile invocation and its expect, while preserving the existing validation and property allocation behavior.crates/bamts-cli/tests/cli.rs (1)
199-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBare
assert!(output.stdout.is_empty())throws away the evidence.When these fail in CI you get "assertion failed" and nothing else, and then somebody wastes an afternoon re-running it locally. Print what was actually produced.
♻️ Attach the offending output
- assert!(output.stdout.is_empty()); + assert!( + output.stdout.is_empty(), + "unexpected stdout: {}", + String::from_utf8_lossy(&output.stdout) + );Also applies to: 224-224
🤖 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 `@crates/bamts-cli/tests/cli.rs` at line 199, Update the stdout assertions in the CLI tests at both affected locations to include the actual output when output.stdout is unexpectedly non-empty. Preserve the existing emptiness check while attaching a useful diagnostic message containing the produced stdout.crates/bamts-compiler/src/lower.rs (5)
5064-5077: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the nested
if; clippy flagged it.
if let ... && ... { if direct.replace(index).is_some() { ... } }— fold the inner condition into the chain.♻️ Fix
if let Statement::Expression(expression) = statement.data() && let Expression::Call(call) = expression.expression.data() && !call.optional && matches!(call.callee.data(), Expression::Super) + && direct.replace(index).is_some() { - if direct.replace(index).is_some() { - return Err(self.unsupported( - statement.range(), - UnsupportedConstruct::DerivedConstructorShape, - )); - } + return Err(self.unsupported( + statement.range(), + UnsupportedConstruct::DerivedConstructorShape, + )); }🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 5064 - 5077, Collapse the nested condition in the statement scan by adding the direct.replace(index).is_some() check to the existing if-let chain. Preserve the current DerivedConstructorShape error and return behavior when a second direct super call is found.Source: Linters/SAST tools
6928-6933: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the braced-surrogate case you're currently not testing.
cooking_preserves_lone_surrogate_unitscovers\uD800(thepush_unitpath) and\u{1F603}, but never\u{D800}— which is the one that takes thepush_code_pointbranch flagged at Line 6720. One extra assertion turns a suspected panic into a caught regression.💚 Add
fn cooking_preserves_lone_surrogate_units() { assert_eq!(cook_escapes("\\uD800").as_units(), [0xD800]); + assert_eq!(cook_escapes("\\u{D800}").as_units(), [0xD800]); assert_eq!(cook_escapes("\\uD83D\\uDE03").as_units(), [0xD83D, 0xDE03]); assert_eq!(cook_escapes("\\u{1F603}").as_units(), [0xD83D, 0xDE03]); }Also applies to: 6988-7001
🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 6928 - 6933, Extend the test cooking_preserves_lone_surrogate_units with an assertion for the braced surrogate escape \u{D800}, expecting the single code unit 0xD800. Keep the existing lone-surrogate and supplementary-code-point assertions unchanged, and apply the same coverage wherever this test is duplicated.
3869-3879: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
debug_assert_eq!is not a guard; make the non-defaultcase a hard error.Under
ProgramModulethis unconditionally stores into"*default*"and only checks thatexported == "default"in debug builds. Today the re-export path bails out earlier (Line 3909) so no caller violates it, but that's an accident of control flow one refactor away. In release, a non-default name would be silently written to the wrong binding — an export pointing at the wrong value, which is exactly the class of bug you cannot debug from a stack trace.🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 3869 - 3879, Replace the debug-only assertion in the ProgramModule branch of the lowering method with a production-enforced failure when exported is not "default"; only call store_binding for the valid default case, preserving the existing *default* binding behavior.
2321-2323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
return; clippy already told you.The arm is in expression position, so
returnis noise and will tripclippy::needless_returnif the workspace denies warnings.♻️ Fix
Expression::Super => { // Derived constructors handle their one supported `super(...)` // call directly so it uses the captured parent and existing receiver. - return Err(self.unsupported(range, UnsupportedConstruct::DerivedConstructorShape)); + Err(self.unsupported(range, UnsupportedConstruct::DerivedConstructorShape)) }🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 2321 - 2323, Remove the explicit return from the derived-constructor arm in the lowering logic, leaving the Err expression as the arm’s value. Preserve the existing unsupported error and range unchanged.Source: Linters/SAST tools
356-388: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the metadata-id ordering here —
ProgramModule.nameis just aConstantId, andcrates/bamts-compiler/src/program.rsfeeds[module.name, ...]into this path. Keep an explicit comment or assertion that these linkage strings must be interned first; one extraintern()above this loop will silently repoint module names.🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 356 - 388, Document in assemble_with_linkage_strings that linkage_strings must be interned before any other constants because ProgramModule.name relies on the resulting ConstantId ordering. Add an explicit comment or assertion immediately around the initial builder interning loop, and preserve this ordering so no earlier intern call can repoint module names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 4cc2b40e-df82-4c9a-b242-efb36fb4ac5c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
crates/bamts-bytecode/src/lib.rscrates/bamts-cli/Cargo.tomlcrates/bamts-cli/build.rscrates/bamts-cli/tests/cli.rscrates/bamts-codegen/src/aot.rscrates/bamts-codegen/src/jit.rscrates/bamts-codegen/src/lib.rscrates/bamts-compiler/src/lower.rscrates/bamts-native/src/native_bridge.rscrates/bamts-node/Cargo.tomlcrates/bamts-node/src/lib.rscrates/bamts-node/src/timers.rscrates/bamts-runtime/src/builtins/array.rscrates/bamts-runtime/src/builtins/collections.rscrates/bamts-runtime/src/builtins/date.rscrates/bamts-runtime/src/builtins/mod.rscrates/bamts-runtime/src/builtins/object.rscrates/bamts-runtime/src/builtins/promise.rscrates/bamts-runtime/src/builtins/regexp.rscrates/bamts-runtime/src/builtins/string.rscrates/bamts-runtime/src/builtins/symbol.rscrates/bamts-runtime/src/builtins/timers.rscrates/bamts-runtime/src/builtins/uint8array.rscrates/bamts-runtime/src/external_modules.rscrates/bamts-runtime/src/host_objects.rscrates/bamts-runtime/src/intrinsics.rscrates/bamts-runtime/src/lib.rscrates/bamts-runtime/src/native.rscrates/bamts-runtime/src/regexp.rscrates/bamts-runtime/src/vm.rscrates/bamts-verification/src/corpus.rscrates/bamts-verification/src/workspace_guard.rscrates/bamts-verification/tests/corpus_differential.rscrates/bamts/Cargo.tomlcrates/bamts/src/lib.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
🪛 Clippy (1.97.1)
crates/bamts-runtime/src/builtins/uint8array.rs
[warning] 40-40: manual saturating arithmetic
(warning)
crates/bamts-runtime/src/intrinsics.rs
[warning] 537-537: this if statement can be collapsed
(warning)
crates/bamts-compiler/src/lower.rs
[warning] 2323-2323: unneeded return statement
(warning)
[warning] 5065-5065: this if statement can be collapsed
(warning)
crates/bamts-runtime/src/lib.rs
[warning] 4612-4612: this if statement can be collapsed
(warning)
[warning] 7494-7494: methods with the following characteristics
(warning)
[warning] 7503-7503: methods with the following characteristics
(warning)
[warning] 7530-7530: methods with the following characteristics
(warning)
[warning] 7535-7535: methods with the following characteristics
(warning)
| pub(super) fn install_generator_prototype<H: Host>( | ||
| heap: &mut Vec<HeapEntry>, | ||
| builtins: &mut BuiltinTable<H>, | ||
| ) { | ||
| let prototype = ordinary(heap, Some(builtins.iterator_prototype())); | ||
| let next = install_function(heap, builtins, "next", 1, generator_next::<H>); | ||
| define_data(heap, prototype, "next", next); | ||
| builtins.set_generator_prototype(prototype); | ||
| } | ||
|
|
||
| fn generator_next<H: Host>( | ||
| _machine: &mut Machine<'_, H>, | ||
| this: Value, | ||
| args: &[Value], | ||
| _constructing: bool, | ||
| ) -> Result<BuiltinOutcome, EvalFailure> { | ||
| Ok(BuiltinOutcome::GeneratorNext { | ||
| generator: this, | ||
| resume_value: args.first().copied().unwrap_or(Value::UNDEFINED), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does anything close iterators via `return`, and is a Generator toStringTag installed anywhere?
rg -nP --type=rust -C3 'iterator_close|IteratorClose|"return"' crates/bamts-runtime/src
rg -nP --type=rust -C2 '"Generator"' crates/bamts-runtime/srcRepository: metaphorics/bamTiScript
Length of output: 666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the collection/runtime files first.
ast-grep outline crates/bamts-runtime/src/builtins/collections.rs --view expanded || true
ast-grep outline crates/bamts-runtime/src/lib.rs --view expanded || true
# Inspect the generator prototype install and iterator-close path.
sed -n '130,190p' crates/bamts-runtime/src/builtins/collections.rs
sed -n '2368,2388p' crates/bamts-runtime/src/lib.rs
# Search for any other generator prototype members or toStringTag wiring.
rg -n --type=rust 'generator_prototype|Generator|toStringTag|install_generator_prototype|GeneratorNext|GeneratorReturn|GeneratorThrow' crates/bamts-runtime/srcRepository: metaphorics/bamTiScript
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the generator prototype tests and any helper that installs prototype tags.
sed -n '1128,1305p' crates/bamts-runtime/src/builtins/collections.rs
# Look for any generator-return/throw builtins or prototype wiring.
rg -n --type=rust 'GeneratorReturn|GeneratorThrow|generator_.*return|generator_.*throw|define_to_string_tag|set_generator_prototype|generator_prototype\(' crates/bamts-runtime/src
# Inspect the iterator-close caller and generator resume handling with tight context.
sed -n '2358,2386p' crates/bamts-runtime/src/lib.rs
sed -n '6460,6530p' crates/bamts-runtime/src/lib.rs
sed -n '6488,6528p' crates/bamts-runtime/src/native.rsRepository: metaphorics/bamTiScript
Length of output: 13566
Add %GeneratorPrototype% return and throw
install_generator_prototype stops at next, but generators need return and throw too. Without return, IteratorClose won’t actually close a generator on early loop exits, so finally blocks can be skipped. If this is intentional, document the subset.
🤖 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 `@crates/bamts-runtime/src/builtins/collections.rs` around lines 143 - 163,
Extend install_generator_prototype to define the standard return and throw
methods alongside next, using the appropriate generator completion behavior so
IteratorClose can terminate generators and execute pending finally blocks. Add
the corresponding builtin handlers near generator_next and register all three
methods on the generator prototype.
| fn define_to_string_tag(heap: &mut [HeapEntry], object: Value, symbol: Value, value: Value) { | ||
| let HeapEntry::Object { properties, .. } = &mut heap[heap_index(object)] else { | ||
| unreachable!() | ||
| }; | ||
| properties.insert( | ||
| PropertyKey::Symbol(heap_index(symbol) as u32), | ||
| Property::Data { | ||
| value, | ||
| writable: false, | ||
| enumerable: false, | ||
| configurable: true, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the shared helper's signature and the attribute set it installs.
ast-grep run --pattern 'fn define_to_string_tag($$$) { $$$ }' --lang rust crates/bamts-runtime/src
rg -nP --type=rust -C2 '\bdefine_to_string_tag\s*\(' crates/bamts-runtime/srcRepository: metaphorics/bamTiScript
Length of output: 5023
🏁 Script executed:
#!/bin/bash
sed -n '91,110p' crates/bamts-runtime/src/builtins/mod.rs
printf '\n---\n'
sed -n '40,90p' crates/bamts-runtime/src/builtins/collections.rsRepository: metaphorics/bamTiScript
Length of output: 3100
Drop the local define_to_string_tag helper. crates/bamts-runtime/src/builtins/mod.rs already provides the real helper that takes &str and handles the string allocation; collections.rs is just copying the same property write and forcing Map/Set to be preallocated first. Call the shared helper directly and delete the duplicate.
🤖 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 `@crates/bamts-runtime/src/builtins/collections.rs` around lines 816 - 829,
Remove the local define_to_string_tag helper from collections.rs. Update its
Map/Set call sites to use the shared builtins::define_to_string_tag from mod.rs,
passing the tag as &str so the shared helper performs allocation; eliminate the
preallocation and direct property-write logic.
| fn parse_iso_date(text: &EcmaString) -> Option<f64> { | ||
| let units = text.as_units(); | ||
| if units.len() != 24 | ||
| || units[4] != u16::from(b'-') | ||
| || units[7] != u16::from(b'-') | ||
| || units[10] != u16::from(b'T') | ||
| || units[13] != u16::from(b':') | ||
| || units[16] != u16::from(b':') | ||
| || units[19] != u16::from(b'.') | ||
| || units[23] != u16::from(b'Z') | ||
| { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
ECMAScript Date Time String Format required forms date-only YYYY-MM-DD
💡 Result:
The ECMAScript specification defines a standardized Date Time String Format as a simplification of the ISO 8601 Extended Format [1][2]. This format supports both full date-time strings and date-only forms [2][3]. For date-only forms, the ECMAScript specification explicitly supports the following: YYYY (e.g., "2026") [2][4] YYYY-MM (e.g., "2026-07") [2][4] YYYY-MM-DD (e.g., "2026-07-30") [2][4] Key Requirements and Behaviors: 1. Numeric Format: All numbers must be decimal (base 10) [5][3]. 2. Component Defaults: If the month (MM) or day (DD) fields are absent, they default to "01" [2][3]. 3. Interpretation of Date-Only vs. Date-Time: - Historically, there has been nuance in how implementations interpret these strings regarding time zones [4][6]. - According to the ECMAScript specification, date-only forms (e.g., YYYY-MM-DD) are interpreted as UTC (effectively treated as the start of the day in UTC, i.e., T00:00Z) [2][4][7]. - In contrast, date-time forms (containing a time component like T00:00) are generally treated as local time [2][4][7]. Developers are encouraged to use the YYYY-MM-DD format as it is standardized, widely supported across modern environments, and avoids the ambiguity often associated with implementation-defined non-standard date formats [8][2][7].
Citations:
- 1: https://stackoverflow.com/questions/51715259/what-are-valid-date-time-strings-in-javascript
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date
- 3: http://www.xahlee.info/js_es2011/JavaScript_15.9_date.html
- 4: Date Time String Format: default time zone difference from ES5 not web-compatible tc39/ecma262#87
- 5: https://learn.microsoft.com/en-us/openspecs/ie_standards/ms-es3ex/e13c8110-e94f-49ef-9316-0bef4dd05833
- 6: https://learn.microsoft.com/en-us/openspecs/ie_standards/ms-es6/ec1701fd-1111-1111-a08e-442f6b9a87cf
- 7: https://www.devdoc.net/web/developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Date/parse.html
- 8: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date
🏁 Script executed:
set -euo pipefail
# Map the file structure first
ast-grep outline crates/bamts-runtime/src/builtins/date.rs --view expanded || true
# Find parse_iso_date usages and nearby test mentions
rg -n "parse_iso_date|Date Time String Format|new Date\\(|offset form|T00:00:00Z|\\+HH:MM|Invalid Date" crates/bamts-runtime/src -S || true
# Read the relevant section of the file with line numbers
sed -n '1,360p' crates/bamts-runtime/src/builtins/date.rsRepository: metaphorics/bamTiScript
Length of output: 10918
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
def parse_iso_date(text: str):
units = [ord(c) for c in text]
if len(units) != 24 \
or units[4] != ord('-') \
or units[7] != ord('-') \
or units[10] != ord('T') \
or units[13] != ord(':') \
or units[16] != ord(':') \
or units[19] != ord('.') \
or units[23] != ord('Z'):
return None
def dec(start, length):
value = 0
for ch in units[start:start+length]:
if not (ord('0') <= ch <= ord('9')):
return None
value = value * 10 + (ch - ord('0'))
return value
year = dec(0, 4)
month = dec(5, 2)
day = dec(8, 2)
hour = dec(11, 2)
minute = dec(14, 2)
second = dec(17, 2)
millisecond = dec(20, 3)
if None in (year, month, day, hour, minute, second, millisecond):
return None
if not (1 <= month <= 12) or day == 0 or day > 31 or hour > 23 or minute > 59 or second > 59:
return None
return (year, month, day, hour, minute, second, millisecond)
for s in [
"2024-01-01",
"2024-01-01T00:00:00Z",
"2024-01-01T00:00:00.000Z",
"2024-01-01T00:00:00.000+00:00",
]:
print(s, "=>", parse_iso_date(s))
PYRepository: metaphorics/bamTiScript
Length of output: 300
parse_iso_date is far too strict for Date strings. It only accepts the exact YYYY-MM-DDTHH:MM:SS.mmmZ shape, so date-only inputs and the common ...Z form without milliseconds both fall through to NaN. If this is intentional, document that restriction here; otherwise parse the full ECMAScript date-time string grammar, including optional time, optional seconds/ms, and Z/±HH:MM.
🤖 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 `@crates/bamts-runtime/src/builtins/date.rs` around lines 148 - 160, Expand
parse_iso_date to accept the ECMAScript date-time string grammar rather than
requiring the fixed 24-unit layout: support date-only values, optional time and
seconds/milliseconds, and UTC or ±HH:MM offsets. Preserve rejection of malformed
inputs and ensure each accepted form produces the correct timestamp instead of
NaN.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (17)
crates/bamts-compiler/src/lower.rs (7)
484-536: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
BindingIdentity::Function(String)is keyed by bare name, so every function-scoped binding in a scan collides with same-named bindings in nested functions.bind_functionrecords the owning root but throws that information away when building the identity, and the resulting sets (captured,runtime_cells,initialized) are flat and shared across all roots in one scan. Three distinct symptoms fall out of that single decision.
crates/bamts-compiler/src/lower.rs#L484-L536: add the owning function root (or an equivalent discriminator) to theFunctionvariant sobinding_identityproduces distinct keys per function, and stop ignoring thesiteargument forDeclarationScope::Function.crates/bamts-compiler/src/lower.rs#L1113-L1123: onceFunctionidentities are discriminated, replace the meaningless sharedbinding_site(range)— currently identical for every hoistedvarand silently discarded — with the real per-binding site.crates/bamts-compiler/src/lower.rs#L5573-L5590: the sharedinitializedset is the sharpest edge: a nested function'svar xinsertsFunction("x")duringpreseed_vars, after which the outerxis treated as initialized and skips theruntime_cellsinsertion, so a binding that needs an uninitialized-sentinel cell silently loses TDZ depending on whether the use sits before or after the nested function. Keying by root fixes it; verify with a test where an outer capturedlet/varis used both before and after a nested function declaring the same name.📍 Affects 1 file
crates/bamts-compiler/src/lower.rs#L484-L536(this comment)crates/bamts-compiler/src/lower.rs#L1113-L1123crates/bamts-compiler/src/lower.rs#L5573-L5590🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 484 - 536, Disambiguate function-scoped BindingIdentity keys by including each owning function root (or equivalent discriminator), and make binding_identity use the actual site for DeclarationScope::Function; update crates/bamts-compiler/src/lower.rs:484-536 accordingly. In crates/bamts-compiler/src/lower.rs:1113-1123, have bind_function preserve and pass the real per-binding site instead of the shared binding_site(range). Ensure initialized and related capture/runtime-cell sets are isolated by function root, then add coverage for an outer captured let/var used both before and after a nested function declaring the same name, covering crates/bamts-compiler/src/lower.rs:5573-5590.
538-540: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Lexical(BindingSite)discards the name, so any two bindings that share a site alias to one cell.Identity is
(start, end)and nothing else. Real declarations get distinct identifier ranges, so today this holds — but the code already synthesizes sites from non-identifier ranges (binding_site(range)for"*default*"at Line 3876, and the anonymous-class fallback at Line 4843). The moment two synthesized sites coincide, two differently-named bindings share one cell and one value. That is a silent miscompile, not a lint.Put the name in the key, or assert uniqueness when inserting into
predeclared_cells.♻️ Cheapest fix
enum BindingIdentity { Function(String), - Lexical(BindingSite), + Lexical(String, BindingSite), }🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 538 - 540, Update binding_site and the Lexical(BindingSite) identity used by predeclared_cells so synthesized bindings cannot alias solely because their ranges coincide; include the binding name in the key. Preserve distinct identity for real declarations while ensuring differently named sites always receive separate cells.
1192-1200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document the deliberate dead read, or someone will "clean it up" and delete TDZ enforcement.
let _ = self.cell_value(builder, cell, range)?;emits a realGetPropertywhose only job is to trap the temporal dead zone before the store. Nothing in the code says so. It reads exactly like a leftover, it will be flagged by every dead-code pass, and removing it silently turnsx = 1; let x;from aReferenceErrorinto a success.♻️ Add the comment
Binding::Cell(cell) => { + // The discarded read is load-bearing: it triggers the + // uninitialized-sentinel check so assignment before + // declaration throws instead of quietly initializing. let _ = self.cell_value(builder, cell, range)?; self.store_cell(builder, cell, value, range) }📝 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.if let Some(binding) = self.resolve(name) { return match binding { Binding::Local(home) => self.move_to(range, home, value), Binding::Cell(cell) => { // The discarded read is load-bearing: it triggers the // uninitialized-sentinel check so assignment before // declaration throws instead of quietly initializing. let _ = self.cell_value(builder, cell, range)?; self.store_cell(builder, cell, value, range) } }; }🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 1192 - 1200, Add an explanatory comment immediately before the discarded self.cell_value call in the Binding::Cell branch of the resolve handling, stating that the read intentionally emits a TDZ-checking GetProperty and must occur before store_cell; preserve the call and its error propagation.
1297-1307: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Does any existing test cover mutually-recursive nested function declarations? rg -nP -C4 'function\s+\w+\s*\(\s*\)\s*\{[^}]*\b(a|b|foo|bar)\s*\(' crates/bamts-compiler/src/lower.rs rg -nP -C6 'mutual|forward.?reference|recursi' crates/bamts-compiler/src/lower.rs crates/bamts-compiler/testsRepository: metaphorics/bamTiScript
Length of output: 161
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant file and inspect the declaration-instantiation path. wc -l crates/bamts-compiler/src/lower.rs sed -n '1240,1335p' crates/bamts-compiler/src/lower.rs printf '\n---\n' sed -n '2260,2325p' crates/bamts-compiler/src/lower.rs printf '\n---\n' sed -n '4735,4795p' crates/bamts-compiler/src/lower.rs printf '\n---\n' sed -n '7300,7365p' crates/bamts-compiler/src/lower.rsRepository: metaphorics/bamTiScript
Length of output: 11297
🏁 Script executed:
#!/bin/bash set -euo pipefail # Look for tests around nested/forward function declarations. rg -n -C4 'function outer|declaredLater|LoadGlobal|CaptureKey::Name|instantiate_declarations' crates/bamts-compiler/src/lower.rs crates/bamts-compiler/testsRepository: metaphorics/bamTiScript
Length of output: 22944
Predeclare function bindings before building sibling closures
instantiate_declarations()still builds function closures in source order, so a later sibling likebis invisible whenais captured. That leavesbas a free name (LoadGlobal) instead of a local capture for legal hoisted nested functions; predeclare function names in pass 1, then materialize the bodies in pass 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 `@crates/bamts-compiler/src/lower.rs` around lines 1297 - 1307, Update instantiate_declarations() to use two passes: first predeclare every ImmediateDeclarationKind::Function binding before constructing any sibling closures, then instantiate each function body in source order using the existing instantiate_function_declaration() flow. Ensure forward-referenced sibling functions resolve as local captures rather than LoadGlobal names.
3024-3028: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash ast-grep run --pattern 'fn lower_member($$$) { $$$ }' --lang rust crates/bamts-compiler/src/lower.rs ast-grep run --pattern 'fn lower_callee($$$) { $$$ }' --lang rust crates/bamts-compiler/src/lower.rs rg -nP -C4 'optional_chain|chain_short|\?\.\w+\.\w+\(' crates/bamts-compiler/src/lower.rsRepository: metaphorics/bamTiScript
Length of output: 1029
🏁 Script executed:
#!/bin/bash sed -n '2948,3125p' crates/bamts-compiler/src/lower.rsRepository: metaphorics/bamTiScript
Length of output: 6487
🏁 Script executed:
#!/bin/bash sed -n '3125,3215p' crates/bamts-compiler/src/lower.rsRepository: metaphorics/bamTiScript
Length of output: 3443
🏁 Script executed:
#!/bin/bash rg -n -C3 'GetProperty|get property|property access' crates -g '*.rs'Repository: metaphorics/bamTiScript
Length of output: 50379
🏁 Script executed:
#!/bin/bash sed -n '2200,2295p' crates/bamts-compiler/src/lower.rs printf '\n--- tests ---\n' sed -n '7960,8035p' crates/bamts-compiler/src/lower.rsRepository: metaphorics/bamTiScript
Length of output: 6693
🏁 Script executed:
#!/bin/bash rg -n -C4 'optional_member_call|optional chain|a\?\.b\.c|obj\?\.method|chain' crates/bamts-compiler/src/lower.rs crates/bamts-parser/src crates/bamts-ast/srcRepository: metaphorics/bamTiScript
Length of output: 7882
🏁 Script executed:
#!/bin/bash sed -n '3523,3575p' crates/bamts-runtime/src/lib.rs printf '\n--- optional-chain tests ---\n' rg -n -C3 'a\?\.b\.c|optional_member_call_skips_arguments_and_preserves_receiver|optional_call' crates/bamts-compiler/src/lower.rsRepository: metaphorics/bamTiScript
Length of output: 4658
🏁 Script executed:
#!/bin/bash sed -n '2100,2215p' crates/bamts-compiler/src/lower.rs printf '\n--- resolve_get ---\n' rg -n -C4 'fn resolve_get|resolve_get\(' crates/bamts-runtime/src/lib.rsRepository: metaphorics/bamTiScript
Length of output: 6139
🏁 Script executed:
#!/bin/bash sed -n '5154,5215p' crates/bamts-runtime/src/lib.rs printf '\n--- call dispatch ---\n' rg -n -C4 'execute_call|CallRequest|Instruction::Call' crates/bamts-runtime/src/lib.rsRepository: metaphorics/bamTiScript
Length of output: 15591
Handle nested optional chains in call lowering
a?.b.c()still lowers the innera?.btoundefinedand then unconditionally reads/calls.c, so a nullishareachesCallinstead of short-circuiting the whole chain.🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 3024 - 3028, Update call lowering around the optional-member handling in the relevant lowering method so nested chains such as a?.b.c() preserve short-circuiting across the entire chain. Ensure the inner optional member remains part of the call’s optional-chain context rather than being materialized as undefined before the subsequent member access, while preserving existing behavior for direct optional-member calls.
4406-4429: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does a generator function object have a prototype property in ECMAScript, and is it constructible?💡 Result:
Yes. In ECMAScript, generator function objects have a own “prototype” property (used to initialize the prototype of the generator object returned when the generator function is called). This is specified by the GeneratorFunction instance creation algorithm that defines “prototype” on generator function instances. [1][2] No. Generator function objects are not constructible: calling them with new is not allowed (they do not have [[Construct]]). This is consistent with the spec’s note that async generator functions are not constructible, and MDN’s description that generator functions’ prototype property is used when called without new, and specifically that generator functions cannot be called with new. [2][3]
Citations:
- 1: https://read262.netlify.app/ecmascript-language-functions-and-classes/generator-function-definitions/
- 2: https://tc39.es/ecma262/2023/multipage/fundamental-objects.html
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/prototype
🌐 Web query:
Do async generator function objects have an own prototype property in ECMAScript?💡 Result:
Yes, async generator function objects do have their own prototype property in ECMAScript [1][2]. Whenever an async generator function is created (either via the async function* syntax or the AsyncGeneratorFunction constructor), a new, distinct ordinary object is automatically created to serve as its prototype property [3][2][4]. This own prototype property is used to initialize the [[Prototype]] internal slot of any async generator object returned when that specific async generator function is invoked [1][3][2]. This property has the attributes { [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: false } [2][4]. It is distinct from the AsyncGenerator prototype object (often referred to as AsyncGeneratorFunction.prototype.prototype), which is the prototype of this own prototype property [1][2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AsyncGeneratorFunction/prototype
- 2: https://tc39.es/ecma262/2024/multipage/control-abstraction-objects.html
- 3: https://tc39.es/proposal-async-iteration/
- 4: https://read262.netlify.app/control-abstraction-objects/asyncgeneratorfunction-objects/
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the surrounding implementation so the rewrite matches the actual lowering path. FILE="crates/bamts-compiler/src/lower.rs" # Show a compact outline around the relevant function. ast-grep outline "$FILE" --match build_constructible_function_value --view expanded || true # Print the relevant slice with line numbers. sed -n '4388,4445p' "$FILE" | cat -nRepository: metaphorics/bamTiScript
Length of output: 2615
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="crates/bamts-compiler/src/lower.rs" ast-grep outline "$FILE" --match build_constructible_function_value --view expanded || true printf '\n---\n' sed -n '4388,4445p' "$FILE" | cat -nRepository: metaphorics/bamTiScript
Length of output: 2620
🌐 Web query:
ECMAScript generator function object's prototype property has constructor property or not💡 Result:
In ECMAScript, every generator function (a function created via the function* syntax or the GeneratorFunction constructor) has a prototype property [1][2]. Unlike regular functions, the object that serves as the value of this prototype property does not contain a constructor property that refers back to the generator function itself [3][4]. This behavior is a notable departure from standard function instances, where the prototype object typically includes a non-enumerable constructor property pointing back to the function [5]. For generator functions, this specific prototype object is created as an empty ordinary object when the generator function is instantiated [3][4]. For clarity, it is important to distinguish this from other objects in the generator prototype chain: 1. GeneratorFunction.prototype (which is the prototype of the generator function object itself) does have a constructor property that points to the GeneratorFunction constructor [3][4]. 2. The Generator prototype object (also known as GeneratorFunction.prototype.prototype) does have a constructor property, which points to GeneratorFunction.prototype [6][7]. However, the specific prototype object assigned to an individual generator function's prototype property does not possess this constructor property by default [3][4]. Consequently, if you inspect this object, you will not find an own property named constructor referencing the generator function [8].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/GeneratorFunction
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/GeneratorFunction/prototype
- 3: https://read262.netlify.app/control-abstraction-objects/generatorfunction-objects/
- 4: https://read262.jedfox.com/control-abstraction-objects/generatorfunction-objects/
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/prototype
- 6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Generator
- 7: https://esdiscuss.org/topic/generatorfunctionprototype-should-probably-be-a-function
- 8: https://esdiscuss.org/history/2013-08-14T17%3A20%3A16.000Z-allen.wirfs-brock.com?path=%2Ftopic%2Fgeneratorfunctionprototype-should-probably-be-a-function
🌐 Web query:
ECMAScript GeneratorFunctionCreate constructor property prototype object💡 Result:
In the ECMAScript specification, generator functions have a "prototype" property, but unlike standard constructor functions, they are not callable with the new operator [1]. When a generator function is created (e.g., via a function declaration or expression), the specification defines a specific process for setting up its prototype: 1. A new ordinary object is created, which serves as the generator function's "prototype" property [2][3][4]. 2. The [[Prototype]] internal slot of this newly created object is set to %GeneratorFunction.prototype.prototype% (often referred to as Generator.prototype) [2][3][4][5][6]. 3. This object is then assigned to the "prototype" property of the generator function object with attributes { [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: false } [2][3][4]. When the generator function is invoked, this "prototype" object becomes the prototype of the generator object that is returned [5][7][6]. It is important to note the distinction between the generator function's "prototype" property and the prototype of the generator function object itself: - The generator function instance has a "prototype" property, which is used to initialize the prototype chain of generator objects created by that specific function [5][7][6]. - The generator function object itself inherits from %GeneratorFunction.prototype% [8][7][6]. - Unlike standard constructor functions, the object assigned to a generator function's "prototype" property does not typically have a "constructor" property pointing back to the generator function [8].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/prototype
- 2: https://tc39.es/ecma262/multipage/ecmascript-language-functions-and-classes.html
- 3: https://tc39.es/ecma262/pr/3770/multipage/ecmascript-language-functions-and-classes.html
- 4: https://read262.netlify.app/ecmascript-language-functions-and-classes/generator-function-definitions/
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/GeneratorFunction
- 6: https://github.com/mdn/content/blob/main/files/en-us/web/javascript/reference/global_objects/generatorfunction/prototype/index.md
- 7: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/GeneratorFunction/prototype
- 8: https://read262.netlify.app/control-abstraction-objects/generatorfunction-objects/
Split prototype handling for generator functions
This guard is too broad.
function*andasync function*still need their ownprototype; only theconstructorback-link is wrong for them. Create the prototype for generator kinds, but skipprototype.constructor = closureunless the function is actually constructible.🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 4406 - 4429, Update the prototype setup in the function-lowering block so generator and async-generator functions still receive a prototype object. Keep assigning prototype.constructor to closure only for non-async, non-generator functions, while ensuring the prototype property is also attached for generator kinds.
4736-4747: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
panic!on a capture-plan disagreement turns a compiler bug into a crash on user input.
CapturePlanis built byCapturePlan::for_function/for_constructor, whilecompute_capturesruns a separateFreeVarScannerpass. This arm fires whenever those two disagree about whether a name is celled — and givenBindingIdentity::Functionis keyed by bare name (Line 484), disagreement is not hypothetical. The payoff is a panic in the middle of compilation instead of aLowerErrorwith a source range.Return a
LowerErrorhere like theNonearm already does. If you genuinely believe it's unreachable, that's whatdebug_assert!plus a defensive error path is for.🤖 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 `@crates/bamts-compiler/src/lower.rs` around lines 4736 - 4747, Replace the panic in the CaptureKey::Name arm of the capture-resolution match with a source-ranged LowerError, matching the existing None arm behavior when a named capture resolves to Binding::Local. Preserve successful Binding::Cell resolution and ensure disagreement between capture planning and free-variable scanning cannot crash compilation on user input.crates/bamts-runtime/src/builtins/object.rs (1)
65-72: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Function.prototype.call/apply/binddo not belong inobject.rs.This file installs
ObjectandObject.prototype. Hanging the threeFunction.prototypemethods off the end ofobject::install— and definingfunction_call/function_apply/function_bindhere — is pure convenience wiring that anyone grepping for function-object semantics will never find. Move them to their ownbuiltins/function.rsand call it frombuiltins::install.🤖 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 `@crates/bamts-runtime/src/builtins/object.rs` around lines 65 - 72, Move the Function.prototype call/apply/bind installation loop and the function_call, function_apply, and function_bind implementations out of object.rs into a new builtins/function.rs module. Add the module’s installer to builtins::install, while keeping object::install limited to Object and Object.prototype setup and preserving the existing handlers and lengths.crates/bamts-runtime/src/builtins/uint8array.rs (2)
13-33: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
A
Uint8Arraybacked by oneEcmaString-keyed property per byte is the wrong data structure.Each element becomes
PropertyKey::Named(EcmaString::from_utf8(&index.to_string()))in aPropertyMap, plus a fullProperty::Datarecord. A 64 KiB buffer means 65 536 heap-allocated UTF-16 key strings and 65 536 map nodes — hence the elaborateuint8array_property_bytesaccounting you had to invent just to preflight the damage. Indexing is a string-keyed map lookup per access instead of a pointer offset.It also gets the object model wrong: an integer-indexed exotic object must not expose
lengthas an own non-configurable data property, out-of-range index writes must be dropped rather than creating new properties, and index enumeration order is a spec-mandated invariant rather than aBTreeMapaccident. Store the bytes in a dedicatedHeapEntry(aVec<u8>) the wayHeapEntry::Collectiondoes forMap/Set, and keeplengthand indices computed.#!/bin/bash # Confirm how other exotic runtime objects store payloads, for a consistent shape. ast-grep run --pattern 'enum HeapEntry { $$$ }' --lang rust crates/bamts-runtime/src/lib.rs rg -nP --type=rust -C2 'Uint8Array' crates/bamts-runtime/src crates/bamts-node/srcAlso applies to: 35-35
🤖 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 `@crates/bamts-runtime/src/builtins/uint8array.rs` around lines 13 - 33, Refactor the Uint8Array implementation around install and its related constructor/accessor logic to store bytes in a dedicated HeapEntry payload such as Vec<u8>, rather than one named PropertyMap entry per byte. Compute length and indexed reads dynamically, drop out-of-range writes, and preserve integer-index enumeration order independently of BTreeMap ordering; remove the uint8array_property_bytes accounting and avoid exposing length as an own non-configurable data property.
35-35: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Run rustfmt. This is not reviewable code.
The whole constructor is crammed onto Line 35. Line 40 holds four separate functions on one line. Lines 228 and 409 do the same to the tests. Nobody can diff, blame, or bisect this. There is no world in which
cargo fmt --checkpasses on this file, so either it's excluded from fmt or CI isn't enforcing it — both are bugs of their own.While you're in there, Line 40's
next.checked_mul(10).unwrap_or(usize::MAX)isnext.saturating_mul(10); Clippy is already shouting about it.#!/bin/bash # Is this file excluded from formatting, and does CI enforce fmt? rg -nP 'rustfmt::skip|ignore\s*=' crates/bamts-runtime/src/builtins/uint8array.rs fd -H 'rustfmt.toml|\.rustfmt\.toml' --exec cat {} rg -nP -C3 'cargo fmt|fmt --check|clippy' .githubAlso applies to: 40-40, 228-232, 409-419
🤖 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 `@crates/bamts-runtime/src/builtins/uint8array.rs` at line 35, Run rustfmt on the entire uint8array.rs file and ensure the repository’s formatting configuration and CI fmt check include it. While formatting, replace the checked multiplication expression in the affected helper with saturating_mul, and format the constructor and tests so each function and statement is reviewable.Source: Linters/SAST tools
crates/bamts-runtime/src/intrinsics.rs (1)
18-36: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Where are Call/ConstructCall produced and consumed, and is the register window still available there? ast-grep run --pattern 'BuiltinOutcome::Call { $$$ }' --lang rust crates/bamts-runtime/src ast-grep run --pattern 'BuiltinOutcome::ConstructCall { $$$ }' --lang rust crates/bamts-runtime/srcRepository: metaphorics/bamTiScript
Length of output: 1632
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== intrinsics.rs outline ===" ast-grep outline crates/bamts-runtime/src/intrinsics.rs --view expanded echo echo "=== intrinsics.rs around BuiltinOutcome ===" sed -n '1,120p' crates/bamts-runtime/src/intrinsics.rs | cat -n echo echo "=== vm.rs around Call/ConstructCall creation and handling ===" sed -n '340,460p' crates/bamts-runtime/src/vm.rs | cat -n echo echo "=== builtins/object.rs around Call creation ===" sed -n '730,810p' crates/bamts-runtime/src/builtins/object.rs | cat -nRepository: metaphorics/bamTiScript
Length of output: 18095
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== BuiltinOutcome consumers ===" rg -n "BuiltinOutcome::(Call|ConstructCall|Value|GeneratorNext)|match .*BuiltinOutcome|enum BuiltinOutcome" crates/bamts-runtime/src echo echo "=== call_builtin handling ===" sed -n '458,560p' crates/bamts-runtime/src/intrinsics.rs | cat -n echo echo "=== call/apply/bound-function producers ===" rg -n "arguments: Vec::new\(\)|arguments: .*to_vec\(\)|arguments," crates/bamts-runtime/src/builtins crates/bamts-runtime/src/vm.rsRepository: metaphorics/bamTiScript
Length of output: 37692
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== native.rs around BuiltinOutcome handling ===" sed -n '1060,1105p' crates/bamts-runtime/src/native.rs | cat -n sed -n '2020,2060p' crates/bamts-runtime/src/native.rs | cat -n echo echo "=== lib.rs around BuiltinOutcome handling ===" sed -n '4488,4535p' crates/bamts-runtime/src/lib.rs | cat -n sed -n '4568,4595p' crates/bamts-runtime/src/lib.rs | cat -n sed -n '4878,4908p' crates/bamts-runtime/src/lib.rs | cat -nRepository: metaphorics/bamTiScript
Length of output: 11097
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '860,905p' crates/bamts-runtime/src/builtins/object.rs | cat -nRepository: metaphorics/bamTiScript
Length of output: 1964
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== BoundCallable definition and bound-call handling ===" rg -n "struct BoundCallable|enum NativeCallable|flatten_bound|BoundCallable" crates/bamts-runtime/src sed -n '1,120p' crates/bamts-runtime/src/native.rs | cat -n sed -n '430,520p' crates/bamts-runtime/src/lib.rs | cat -nRepository: metaphorics/bamTiScript
Length of output: 11465
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '4134,4195p' crates/bamts-runtime/src/lib.rs | cat -nRepository: metaphorics/bamTiScript
Length of output: 2714
Stop copying call arguments through
BuiltinOutcomeincrates/bamts-runtime/src/intrinsics.rs:21-30.Call/ConstructCallnow carry ownedVec<Value>s, soFunction.prototype.call/applyand bound-function flattening allocate and copy on a hot path. Keep the register-window slice plus count, or use an inline small buffer for the common case.🤖 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 `@crates/bamts-runtime/src/intrinsics.rs` around lines 18 - 36, Update the BuiltinOutcome Call and ConstructCall variants to avoid owning copied Vec<Value> argument lists; preserve the register-window slice with its argument count, or use an inline small-buffer representation. Adjust all producers and consumers, including Function.prototype.call/apply and bound-function flattening, to pass and read the new representation without allocating or copying hot-path arguments.crates/bamts-runtime/src/lib.rs (2)
979-993: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Arraycharges one byte no matter how many elements it holds.
initial_bytesfoldsArrayinto the flat=> 1arm, so a pre-populated array is free. Meanwhilearray_pushcharges 8 per element andadd_promise_all_elementchargessize_of::<Value>()— so the intended rate is known and this path just doesn't pay it. Every allocation site that hands over a populatedVecexploits it:create_array(values)fromPromise.all/iterable_values,materialize_arguments(elements: args), andCreateCell.iterable_valuescallsensure_allocation_capacity, which only checks — it never adds toheap_bytes. Net effect: script-controlled N values enter the heap without touchingmax_heap_bytes. For a crate whose whole selling point is deterministic limits, that's not a rounding error.
ExternalModuleNamespace { specifier }has the same shape — it owns anEcmaStringand is charged 1 — though that one is host-installed and bounded.🛡️ Charge for the payload you actually allocated
Self::Object { properties, .. } | Self::Timeout { properties, .. } => { properties.charge_bytes().saturating_add(1) } - Self::Array { .. } - | Self::Function { .. } + Self::Array { + elements, + properties, + .. + } => elements + .len() + .saturating_mul(std::mem::size_of::<Value>()) + .saturating_add(properties.charge_bytes()) + .saturating_add(1), + Self::ExternalModuleNamespace { specifier } => { + specifier.len_units().saturating_mul(2).saturating_add(1) + } + Self::Function { .. } | Self::Script { .. } | Self::ModuleNamespace { .. } - | Self::ExternalModuleNamespace { .. } | Self::ProcessEnv { .. }📝 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.Self::Object { properties, .. } | Self::Timeout { properties, .. } => { properties.charge_bytes().saturating_add(1) } Self::Array { elements, properties, .. } => elements .len() .saturating_mul(std::mem::size_of::<Value>()) .saturating_add(properties.charge_bytes()) .saturating_add(1), Self::ExternalModuleNamespace { specifier } => { specifier.len_units().saturating_mul(2).saturating_add(1) } Self::Function { .. } | Self::Script { .. } | Self::ModuleNamespace { .. } | Self::ProcessEnv { .. } | Self::Date { .. } | Self::BuiltinIterator { .. } | Self::Iterator { .. } | Self::Promise { .. } | Self::PromiseResolver { .. } | Self::PromiseFinally { .. } | Self::PromiseAll { .. } | Self::AsyncActivation { .. } | Self::PromiseAllElement { .. } => 1,🤖 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 `@crates/bamts-runtime/src/lib.rs` around lines 979 - 993, Update the Value::initial_bytes calculation so Array is charged for every element in its stored Vec, using the same per-element rate as array_push and add_promise_all_element, rather than the flat 1-byte arm. Also account for the owned specifier payload in ExternalModuleNamespace instead of charging only 1, while preserving the existing base charges and other variant behavior.
5217-5278: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Two hand-maintained copies of ECMAScript property lookup.
resolve_get_asciiisresolve_getwith the key type swapped, andown_get_asciiisown_getre-typed — ~250 lines of copy-paste carrying the RegExp flag table twice, the module-namespace export walk twice, the Array length/index rules twice, andprimitive_getinlined in one copy and called in the other. They agree today; they will not agree after the first bug fix, and the arm grouping has already drifted (NativeFunctionis its own arm inown_get, folded into the Object group here). The ASCII fast path is a good idea — the duplication is not the way to get it.Make the ASCII form a thin adapter over one implementation: introduce a borrowed key view (
enum KeyRef<'a> { Ascii(&'a str), Key(&'a PropertyKey) }witheq_name/array_indexhelpers), implement the lookup once against it, and have both entry points construct the view. No allocation, one copy of the semantics.Also applies to: 5324-5449
🤖 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 `@crates/bamts-runtime/src/lib.rs` around lines 5217 - 5278, Eliminate the duplicated property-lookup semantics between resolve_get_ascii/own_get_ascii and resolve_get/own_get by introducing a borrowed KeyRef<'a> view with Ascii and Key variants plus shared eq_name and array_index helpers. Refactor the common lookup implementation, including primitive, RegExp, module-namespace, array, and prototype handling, to consume KeyRef; make both entry points construct the appropriate view without allocation while preserving existing behavior.crates/bamts-runtime/src/native.rs (2)
206-218: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CodeRef::Dynamicsilently ignores themoduleargument.The signature says "give me the bytecode for module N", the Dynamic arm hands back
modules()[0]regardless. Today that happens to work because installed scripts are single-module programs, but nothing in this type enforces that, and the next person who installs a two-module dynamic program gets the wrong function table with no diagnostic — just wrong code executing. At minimum assert the invariant instead of praying.🛡️ Assert the single-module invariant
impl CodeRef<'_> { fn code(&self, module: ModuleId) -> &Module<Verified> { match self { Self::Root(program) => { &program .module(module) .expect("verified native module id remains in bounds") .code } - Self::Dynamic(program) => &program.modules()[0].code, + Self::Dynamic(program) => { + debug_assert_eq!( + program.modules().len(), + 1, + "dynamic module registry holds single-module scripts" + ); + &program.modules()[program.entry().get() as usize].code + } } } }📝 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.impl CodeRef<'_> { fn code(&self, module: ModuleId) -> &Module<Verified> { match self { Self::Root(program) => { &program .module(module) .expect("verified native module id remains in bounds") .code } Self::Dynamic(program) => { debug_assert_eq!( program.modules().len(), 1, "dynamic module registry holds single-module scripts" ); &program.modules()[program.entry().get() as usize].code } } } }🤖 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 `@crates/bamts-runtime/src/native.rs` around lines 206 - 218, Update CodeRef::code so the CodeRef::Dynamic arm validates the single-module invariant before returning modules()[0].code: assert that the requested module is the sole valid module (or otherwise assert the dynamic program contains exactly one module and the argument identifies it), then preserve the existing module-0 return for valid dynamic programs.
435-452: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Module-graph evaluation recurses on the Rust stack three times over, with no depth budget anywhere. All three evaluators walk dependency edges by direct self-recursion. Cycles are handled; depth is not. Every other resource in this engine is metered — call depth, registers, fuel — so a deep import chain is the one input that kills the process outright instead of returning a
RuntimeError. One shared depth budget (or an explicit worklist) fixes all three.
crates/bamts-runtime/src/native.rs#L435-L452: boundevaluate_reference_module's dependency recursion againstmax_call_depthor an explicit graph-depth counter.crates/bamts-runtime/src/native.rs#L1814-L1825: apply the same budget toevaluate_import's dependency recursion.crates/bamts-runtime/src/native.rs#L2149-L2165: apply the same budget toevaluate_linked_module's dependency recursion.📍 Affects 1 file
crates/bamts-runtime/src/native.rs#L435-L452(this comment)crates/bamts-runtime/src/native.rs#L1814-L1825crates/bamts-runtime/src/native.rs#L2149-L2165🤖 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 `@crates/bamts-runtime/src/native.rs` around lines 435 - 452, Add one shared depth budget to the dependency traversal performed by evaluate_reference_module, evaluate_import, and evaluate_linked_module, decrementing or checking it before each recursive dependency call and returning the established RuntimeError when exhausted; ensure the same budget is propagated across all three evaluators rather than creating independent limits, while preserving existing cycle handling and evaluation results. Apply this change at crates/bamts-runtime/src/native.rs:435-452, 1814-1825, and 2149-2165.crates/bamts-runtime/src/vm.rs (1)
231-251: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
runInNewContextdoes not create a new context — it runs untrusted code against the live globals.Any non-
undefinedcontextObjectis rejected, so the only usable call isrunInNewContext(code), and that path goes straight tocall_entrywithglobalThisas the receiver and the host realm's globals fully reachable. The whole point of this API's name is isolation; callers who reach forvm.runInNewContextare doing so because they believe the script cannot see the ambient global object. Silently downgrading torunInThisContextsemantics is worse than not shipping the export at all.Either make the no-context case allocate a fresh, empty global for the compiled entry, or throw
"runInNewContext is unsupported"unconditionally until it does. Do not export a security-shaped API that provides no isolation.🤖 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 `@crates/bamts-runtime/src/vm.rs` around lines 231 - 251, Update the runInNewContext implementation so it never executes code against the host realm: either create and use a fresh empty global context for the no-context path, or reject every invocation with the existing “runInNewContext is unsupported” error until isolation is implemented. Ensure the function does not fall through to call_entry with the ambient global receiver.crates/bamts-verification/tests/corpus_differential.rs (1)
182-213: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Four copies of the same test body.
task_106_sync_cases_match_node_in_jit_and_aotandtask_107_node_cases_match_node_in_jit_and_aotare byte-identical apart from the ID constant and two strings — and the two generated-fixture tests above repeat the same discover/run/compare/join ritual again. Copy-paste the fifth time and someone will fix a bug in one and not the other four.♻️ Extract the gate body
fn assert_gate(ids: &[&str], label: &str, order_message: &str) { let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("../.."); let corpus = load_corpus(&root).expect("the pinned corpus must parse and validate"); let discovered = corpus .cases .iter() .filter(|case| ids.contains(&case.id.as_str())) .map(|case| case.id.as_str()) .collect::<Vec<_>>(); assert_eq!(discovered, ids, "{order_message}"); let oracle = NodeOracle::discover(&root).expect("the pinned Node oracle must be available"); let bamts = BamtsRunner::new(&root); let mut failures = Vec::new(); for id in ids { let case = corpus.case(id).expect("gated case must exist"); let expected = oracle.run_case(case); for mode in ExecutionMode::ALL { compare_case(&case.id, mode, &expected, &bamts.run_case(case, mode), &mut failures); } } assert!( failures.is_empty(), "{label} differential failures:\n{}", failures.join("\n\n") ); }The generated-fixture tests can share a second helper that takes
(id, source)and returns thefailuresvector.Also applies to: 215-246
🤖 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 `@crates/bamts-verification/tests/corpus_differential.rs` around lines 182 - 213, Extract the repeated corpus differential gate logic from task_106_sync_cases_match_node_in_jit_and_aot and the corresponding Task 107 test into a shared assert_gate helper accepting IDs, a label, and an order message. Update both tests to call it while preserving their case selection, ordering validation, Node oracle execution, JIT/AOT comparisons, and failure reporting. Also extract the repeated generated-fixture setup into a helper accepting (id, source) and returning the failures vector, then update both generated-fixture tests to reuse it.
|
@codex review |
|
@coderabbitai ultrareview |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 56 minutes. |
💡 Codex ReviewbamTiScript/crates/bamts-runtime/src/native.rs Line 2709 in 702a9e7  Drain queued jobs before dropping the native engine
When a linked program schedules a promise reaction, resumes an bamTiScript/crates/bamts-runtime/src/lib.rs Lines 3232 to 3233 in 702a9e7 Return a Promise from dynamic import
For a source-level Preserve typed-array invariants after construction
Every bamTiScript/crates/bamts-runtime/src/lib.rs Line 4450 in 702a9e7  Handle functions that are both async and generators
An bamTiScript/crates/bamts-runtime/src/lib.rs Lines 1576 to 1580 in 702a9e7 Charge retained timer arguments against allocation limits
Each successful ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary by cubic
Moves the engine to a canonical whole‑program container with exact UTF‑16 strings, and bumps bytecode/ABI versions. Adds whole‑program compile/execute, a strictness lint surface with CLI
explain, and updates JIT/AOT/runtime and Node host to the new linkage.New Features
bamts-bytecode: AddsProgramenvelope and linkage; bumpsFORMAT_VERSIONto 3 andPROGRAM_VERSIONto 3; introducesEcmaString; improves decode errors for truncated payloads.bamts-codegen: LowersPrograminstead of modules; updates JIT/AOT to ABI v3 with 32 helpers and module‑qualified symbols.bamts-runtime: Executes linked programs; supports closed‑graph dynamic imports; exactEcmaStringfor globals/keys; preserves symbol/exotic property identities; completesbind/apply; enforces local TDZ; adds promise microtask queue; resumes timers/suspended calls; improvesUint8Arrayconstruction (coercion and array‑like inputs); installs synchronous corpus builtins; adds VM entry for host‑compiled scripts.bamts-node: Adds timers provider backed by Tokio; exposes a classic‑script compiler viaset_script_compiler(feature‑gated); completes builtin providers.bamts-compiler: Resolves complete program graphs and lowers executable programs; adds 86 semantic strictness rules and generatesRULES.md; exposes a classic‑script compile API; fixes self‑referential captures, class declaration timing, and completion after caught throws.bamts-cli: Checks/compiles/runs whole programs; addsexplainfor lint rules; embeds the current AOT runtime.Migration
bamts-node, optionally set a script compiler viaNodeHost::set_script_compiler(...)if you need host‑compiled scripts.Written for commit 702a9e7. Summary will update on new commits.