Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CI benchmark jobs and local benchmark scripts plus a comparator; changes Gas from signed i64 to unsigned u64 and consolidates GasCounter into a single class; updates VM gas fields and related API signatures to use the new Gas type; replaces some bounds-checked instruction/page accesses with unchecked calls and adds a fast-path and explicit jump handling in the interpreter; removes lazy page initialization and eager allocation flag, introduces a fixed-size PageCache and always-allocate path in Arena; adds a reusable EXTENDED_BUF for program decoding; and small test, lint, and .gitignore adjustments. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
tomusdrw
marked this pull request as ready for review
February 27, 2026 23:40
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assembly/gas.ts (1)
10-14:⚠️ Potential issue | 🔴 CriticalChange Gas type from i64 to u64 to prevent gas accounting bypass.
The
Gastype is signed (i64), allowing external APIs to accept and set negative gas values. Whensub()is called with a negative input (e.g.,sub(-50)withgas=100), the subtraction increases the counter to 150 rather than decreasing it. The post-operation clamp at line 23 only catches negative results, not the increase case, breaking gas accounting invariants and enabling unlimited gas execution.APIs expose this directly:
vmInit()acceptsinput.gas: i64andpvmResume()acceptsgas: i64parameters, both callingset()without validation.Harden by changing
Gastou64, removing redundanti64()conversions, and reorderingsub()to check overflow before subtraction:Suggested fix
-/** Gas type. */ -export type Gas = i64; +/** Gas type. */ +export type Gas = u64; /** Create a new gas counter instance. */ -export function gasCounter(gas: i64): GasCounter { +export function gasCounter(gas: Gas): GasCounter { return new GasCounter(gas); } export class GasCounter { constructor(private gas: Gas) {} set(g: Gas): void { - this.gas = i64(g); + this.gas = g; } get(): Gas { return this.gas; } `@inline` sub(g: Gas): boolean { - this.gas = this.gas - i64(g); - if (this.gas < i64(0)) { - this.gas = i64(0); + if (this.gas < g) { + this.gas = 0; return true; } + this.gas -= g; return false; } }Also update
VmInput.gasandVmOutput.gas,VmPause.gastypes tou64inapi-types.ts, andpvmResume()signature inapi-utils.ts:231.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assembly/gas.ts` around lines 10 - 14, Change the Gas type from signed i64 to unsigned u64 across the assembly code and public APIs to prevent negative gas bypass: update the Gas type alias and replace all i64(...) casts in the Gas class (constructor, set) with u64 usage, remove redundant conversions, and update the sub() method to check for overflow/underflow before performing subtraction (i.e., validate requested decrement against current gas and clamp to zero or return error) so a negative input cannot increase gas. Also update related API types and signatures to use u64: VmInput.gas, VmOutput.gas, VmPause.gas in api-types.ts and the pvmResume(...) signature in api-utils.ts (and any vmInit(...) call sites) so callers cannot pass signed values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Line 66: The YAML comments "# Checkout base branch for baseline" and the other
mis-indented comment at the same section need to be indented to match the
surrounding mapping keys so the linter accepts the file; edit the workflow file
to align those comments with the adjacent mapping entries (same indentation
level as the nearest key/step under the job or step block), ensuring both
comment lines are shifted to the correct column to match their related mapping
content.
- Around line 82-86: The benchmark comparison uses inconsistent paths: PR
results are copied to ../results.json while baseline is placed in a different
location, but the "Compare benchmarks" step runs npm run bench:compare
../baseline.json ../results.json which won't find the files; update the workflow
so both files are written/read from the same consistent location (either copy
baseline.json and results.json into the same directory the compare step expects,
or change the bench:compare invocation to the actual paths where baseline and
results were written) and verify the npm script name (bench:compare) remains the
argument order baseline then results.
- Around line 73-77: The workflow step "Run baseline benchmark" is running
benchmarks in ./baseline without building dependencies; update the step to first
install and build the baseline checkout by running npm ci (or npm install) and
npm run build inside ./baseline before running npm run bench:baseline so
bench/run.ts can correctly import ../build/release.js and benchmark the built
artifact.
In `@assembly/gas.ts`:
- Line 5: Change the gasCounter factory signature to use the existing Gas type
alias instead of a raw i64 so the API stays consistent and future-proofs type
changes: update the function declaration for gasCounter(gas: i64): GasCounter to
gasCounter(gas: Gas): GasCounter and ensure any callsites and imports/aliases
referencing Gas are present and compile with the new parameter type.
In `@assembly/memory.ts`:
- Around line 165-173: The loop over the Map iterator uses an untyped loop
variable `i`; change its declaration to a fixed-size type (u32) to match
AssemblyScript conventions and avoid `@ts-ignore` suppression. Specifically, in
the block that gets `const keys = pages.keys();` and iterates `for (let i = 0; i
< keys.length; i++) { const key = keys[i]; this.cache.insert(key,
pages.get(key)); }`, declare `i` as `let i: u32 = 0` (and update the comparison
and increment accordingly if needed) and remove the unnecessary `@ts-ignore`
comments so the code type-checks without suppressions.
In `@bench/compare.ts`:
- Line 108: Guard the percentage calculations against zero denominators by
checking baselineTrace.medianMs (and the other baseline fields used at the other
two sites) before doing the division: compute diffPercent as follows — if
baselineTrace.medianMs === 0 then set diffPercent to 0 when diffMs === 0, or to
Number.POSITIVE_INFINITY/Number.NEGATIVE_INFINITY depending on diffMs sign,
otherwise compute (diffMs / baselineTrace.medianMs) * 100; apply the same
zero-check logic to the analogous calculations at the other two locations (the
computations at lines 160 and 168) so you never divide by zero and downstream
classification/reporting sees a well-defined numeric value.
- Around line 39-40: The parsed threshold (const threshold =
parseFloat(values.threshold!)) must be validated before use: after parsing,
check that threshold is a finite number and not negative (e.g.,
Number.isFinite(threshold) && threshold >= 0); if the value is NaN, infinite, or
negative, emit a clear error message referencing the --threshold arg
(values.threshold) and terminate with a non-zero exit (or throw) so regression
checks don't run with an invalid threshold. Ensure this validation occurs
immediately after parseFloat and before any comparisons that use threshold.
In `@bench/run.ts`:
- Around line 48-50: Parsed iteration values ITERATIONS and WARMUP are not being
validated, which allows NaN/0/negative values to produce an empty samples array
and break median/min/max/p95 computations; validate the parsed integers right
after parsing (ensure ITERATIONS is an integer >= 1 and WARMUP is an integer >=
0 and < ITERATIONS), throw or exit with a clear error if invalid, and
additionally guard the stats computation (where median/min/max/p95 are
calculated from samples) to handle an empty samples array by skipping stats or
returning a clear error so computations never run on empty data.
---
Outside diff comments:
In `@assembly/gas.ts`:
- Around line 10-14: Change the Gas type from signed i64 to unsigned u64 across
the assembly code and public APIs to prevent negative gas bypass: update the Gas
type alias and replace all i64(...) casts in the Gas class (constructor, set)
with u64 usage, remove redundant conversions, and update the sub() method to
check for overflow/underflow before performing subtraction (i.e., validate
requested decrement against current gas and clamp to zero or return error) so a
negative input cannot increase gas. Also update related API types and signatures
to use u64: VmInput.gas, VmOutput.gas, VmPause.gas in api-types.ts and the
pvmResume(...) signature in api-utils.ts (and any vmInit(...) call sites) so
callers cannot pass signed values.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (63)
bench/traces/trace-001.logis excluded by!**/*.logbench/traces/trace-002.logis excluded by!**/*.logbench/traces/trace-003.logis excluded by!**/*.logbench/traces/trace-004.logis excluded by!**/*.logbench/traces/trace-005.logis excluded by!**/*.logbench/traces/trace-006.logis excluded by!**/*.logbench/traces/trace-007.logis excluded by!**/*.logbench/traces/trace-008.logis excluded by!**/*.logbench/traces/trace-009.logis excluded by!**/*.logbench/traces/trace-010.logis excluded by!**/*.logbench/traces/trace-011.logis excluded by!**/*.logbench/traces/trace-012.logis excluded by!**/*.logbench/traces/trace-013.logis excluded by!**/*.logbench/traces/trace-014.logis excluded by!**/*.logbench/traces/trace-015.logis excluded by!**/*.logbench/traces/trace-016.logis excluded by!**/*.logbench/traces/trace-017.logis excluded by!**/*.logbench/traces/trace-018.logis excluded by!**/*.logbench/traces/trace-019.logis excluded by!**/*.logbench/traces/trace-020.logis excluded by!**/*.logbench/traces/trace-021.logis excluded by!**/*.logbench/traces/trace-022.logis excluded by!**/*.logbench/traces/trace-023.logis excluded by!**/*.logbench/traces/trace-024.logis excluded by!**/*.logbench/traces/trace-025.logis excluded by!**/*.logbench/traces/trace-026.logis excluded by!**/*.logbench/traces/trace-027.logis excluded by!**/*.logbench/traces/trace-028.logis excluded by!**/*.logbench/traces/trace-029.logis excluded by!**/*.logbench/traces/trace-030.logis excluded by!**/*.logbench/traces/trace-031.logis excluded by!**/*.logbench/traces/trace-032.logis excluded by!**/*.logbench/traces/trace-033.logis excluded by!**/*.logbench/traces/trace-034.logis excluded by!**/*.logbench/traces/trace-035.logis excluded by!**/*.logbench/traces/trace-036.logis excluded by!**/*.logbench/traces/trace-037.logis excluded by!**/*.logbench/traces/trace-038.logis excluded by!**/*.logbench/traces/trace-039.logis excluded by!**/*.logbench/traces/trace-040.logis excluded by!**/*.logbench/traces/trace-041.logis excluded by!**/*.logbench/traces/trace-042.logis excluded by!**/*.logbench/traces/trace-043.logis excluded by!**/*.logbench/traces/trace-044.logis excluded by!**/*.logbench/traces/trace-045.logis excluded by!**/*.logbench/traces/trace-046.logis excluded by!**/*.logbench/traces/trace-047.logis excluded by!**/*.logbench/traces/trace-048.logis excluded by!**/*.logbench/traces/trace-049.logis excluded by!**/*.logbench/traces/trace-050.logis excluded by!**/*.logbench/traces/trace-051.logis excluded by!**/*.logbench/traces/trace-052.logis excluded by!**/*.logbench/traces/trace-053.logis excluded by!**/*.logbench/traces/trace-054.logis excluded by!**/*.logbench/traces/trace-055.logis excluded by!**/*.logbench/traces/trace-056.logis excluded by!**/*.logbench/traces/trace-057.logis excluded by!**/*.logbench/traces/trace-058.logis excluded by!**/*.logbench/traces/trace-059.logis excluded by!**/*.logbench/traces/trace-060.logis excluded by!**/*.logbench/traces/trace-061.logis excluded by!**/*.logbench/traces/trace-062.logis excluded by!**/*.logbench/traces/trace-063.logis excluded by!**/*.log
📒 Files selected for processing (12)
.github/workflows/build.ymlassembly/gas.tsassembly/instructions/outcome.tsassembly/interpreter.tsassembly/memory-page.tsassembly/memory.tsassembly/program.tsbench/.gitignorebench/compare.tsbench/run.tsbiome.jsoncpackage.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
assembly/api-utils.ts (1)
101-108: 🧹 Nitpick | 🔵 TrivialInconsistency:
runProgramstill usesi64whilepvmResumeusesGas.For API consistency, consider updating
runProgramto useGastype as well.export function runProgram( program: StandardProgram, - initialGas: i64 = 0, + initialGas: Gas = u64(0), programCounter: u32 = 0, logs: boolean = false, useSbrkGas: boolean = false, dumpMemory: boolean = false, ): VmOutput { const vmInput = new VmInput(program.program, program.memory, program.registers); - vmInput.gas = i64(initialGas); + vmInput.gas = initialGas;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assembly/api-utils.ts` around lines 101 - 108, Update runProgram to accept and propagate the Gas type instead of i64: change the parameter initialGas's type from i64 to Gas and ensure VmInput.gas is assigned using that Gas value (convert only if VmInput.gas expects a different internal representation). Locate the runProgram function and adjust the signature and the assignment to vmInput.gas accordingly (referencing runProgram, VmInput, VmOutput, initialGas, and Gas) so it matches pvmResume's API consistency.assembly/api-debugger.ts (1)
116-129:⚠️ Potential issue | 🟡 MinorInconsistent return type:
getGasLeftreturnsi64whilesetGasLeftacceptsGas(u64).This asymmetry can cause data loss or unexpected behavior when round-tripping gas values. Since
Gasis nowu64, the getter should returnGasfor consistency.Proposed fix
-export function getGasLeft(): i64 { +export function getGasLeft(): Gas { if (interpreter === null) { - return i64(0); + return u64(0); } const int = <Interpreter>interpreter; return int.gas.get(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assembly/api-debugger.ts` around lines 116 - 129, The getter getGasLeft currently returns i64 while setGasLeft uses Gas (u64), causing an inconsistent API; change getGasLeft's return type to Gas and have it return the interpreter's gas via int.gas.get() as a Gas (preserve Interpreter and interpreter usage and keep setGasLeft unchanged), i.e., update the getGasLeft signature from i64 to Gas and ensure the returned value is the u64 Gas value returned by int.gas.get().
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
79-88:⚠️ Potential issue | 🔴 CriticalBenchmark artifact paths are still inconsistent and can break comparison.
baseline.jsonandresults.jsonare written/read from different directories, sobench:comparemay fail to locate one of them.Proposed fix
- name: Run PR benchmark run: | npm run bench:ci - cp ./bench/results.json ../results.json + cp ./bench/results.json ./results.json @@ - name: Compare benchmarks run: | - npm run bench:compare ../baseline.json ../results.json -- --threshold 5 + npm run bench:compare ./baseline.json ./results.json -- --threshold 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 79 - 88, The workflow writes baseline.json to the repository root (cp ./bench/baseline.json ../baseline.json) but reads results.json from a different path in the Compare step, causing bench:compare to fail; make the artifact paths consistent by producing both artifacts in the same location before running npm run bench:compare (adjust the copy in the "Run PR benchmark" step or the paths passed to npm run bench:compare) so that the filenames baseline.json and results.json referenced by the "Run PR benchmark" and "Compare benchmarks" steps resolve to the same directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 67-71: The checkout step using actions/checkout@v4 currently sets
ref: ${{ github.base_ref }} which uses a moving branch name; change that ref to
the fixed commit SHA from the PR payload by replacing ref: ${{ github.base_ref
}} with ref: ${{ github.event.pull_request.base.sha }} so the baseline checkout
(path: ./baseline) uses an immutable commit for reproducible results.
In `@bench/compare.ts`:
- Around line 172-180: The template literals that print the difference are not
wrapped in console.log, causing syntax errors; locate the block that computes
totalDiffPercent (using totalDiff and baselineSuite.summary.totalTraceMedianMs)
and wrap each standalone template literal/string output in console.log calls
(e.g., replace the bare template literal lines that end with `);` with
console.log(templateString)) so the formatted strings are actually logged and
the stray `);` is removed.
In `@bench/run.ts`:
- Line 219: The call to runProgram(exe, gas, pc, false, false, true) is passing
true for the dumpMemory flag which adds memory-dump overhead to benchmarks;
update the invocation of runProgram (the call site using variables exe, gas, pc)
to pass false for the dumpMemory parameter so the call becomes runProgram(...,
false) for that flag, ensuring accurate benchmark timing without memory dump
overhead.
- Around line 95-121: The benchRun function is missing its closing brace and
return; close the function after computing med/min/max/p95 and return a
BenchResult object (matching the BenchResult shape used elsewhere) e.g. include
name and numeric fields medianMs, minMs, maxMs, p95Ms (values from med, min,
max, p95) so formatResult can consume it; ensure benchRun ends before the
formatResult function declaration and keep references to median(samples) and
percentile(samples, 95) unchanged.
---
Outside diff comments:
In `@assembly/api-debugger.ts`:
- Around line 116-129: The getter getGasLeft currently returns i64 while
setGasLeft uses Gas (u64), causing an inconsistent API; change getGasLeft's
return type to Gas and have it return the interpreter's gas via int.gas.get() as
a Gas (preserve Interpreter and interpreter usage and keep setGasLeft
unchanged), i.e., update the getGasLeft signature from i64 to Gas and ensure the
returned value is the u64 Gas value returned by int.gas.get().
In `@assembly/api-utils.ts`:
- Around line 101-108: Update runProgram to accept and propagate the Gas type
instead of i64: change the parameter initialGas's type from i64 to Gas and
ensure VmInput.gas is assigned using that Gas value (convert only if VmInput.gas
expects a different internal representation). Locate the runProgram function and
adjust the signature and the assignment to vmInput.gas accordingly (referencing
runProgram, VmInput, VmOutput, initialGas, and Gas) so it matches pvmResume's
API consistency.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 79-88: The workflow writes baseline.json to the repository root
(cp ./bench/baseline.json ../baseline.json) but reads results.json from a
different path in the Compare step, causing bench:compare to fail; make the
artifact paths consistent by producing both artifacts in the same location
before running npm run bench:compare (adjust the copy in the "Run PR benchmark"
step or the paths passed to npm run bench:compare) so that the filenames
baseline.json and results.json referenced by the "Run PR benchmark" and "Compare
benchmarks" steps resolve to the same directory.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.github/workflows/build.ymlassembly/api-debugger.tsassembly/api-types.tsassembly/api-utils.tsassembly/gas.tsassembly/instructions.tsbench/compare.tsbench/run.tsbiome.jsonc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
67-71:⚠️ Potential issue | 🟠 MajorUse immutable base SHA for reproducible baseline checkout.
Using
github.base_refchecks out a moving branch tip, so benchmark baselines can drift between runs. Use the PR payload SHA instead.Suggested fix
- name: Checkout base branch for baseline uses: actions/checkout@v4 with: - ref: ${{ github.base_ref }} + ref: ${{ github.event.pull_request.base.sha }} path: ./baseline🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 67 - 71, Replace the moving ref currently used in the checkout step named "Checkout base branch for baseline" (uses: actions/checkout@v4 with ref: ${{ github.base_ref }}) with the immutable PR payload SHA; change the ref to use github.event.pull_request.base.sha so the baseline checkout is reproducible across runs and does not drift.
84-88:⚠️ Potential issue | 🔴 CriticalBenchmark comparison paths are inconsistent and can fail.
At Line 84, PR results are copied to the parent directory, while baseline is written to repo root (from Line 79). Then compare reads both from parent (
../...), so baseline path is wrong.Suggested fix
- name: Run PR benchmark run: | npm run bench:ci - cp ./bench/results.json ../results.json + cp ./bench/results.json ./results.json # Compare results - name: Compare benchmarks run: | - npm run bench:compare ../baseline.json ../results.json -- --threshold 5 + npm run bench:compare ./baseline.json ./results.json -- --threshold 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 84 - 88, The benchmark compare step is using inconsistent paths between the copy command (cp ./bench/results.json ../results.json) and the compare invocation (npm run bench:compare ../baseline.json ../results.json -- --threshold 5); fix by making both sides use the same location — either write the PR results to the repo-root or change the compare arguments to read from the directory where baseline and results actually reside. Update the cp command or the npm run bench:compare arguments accordingly so "Compare benchmarks" references the exact same baseline and results file locations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Line 29: The inline comment spacing on the ref line (the line containing "ref:
ba76542dbf7a0c72d414a87ad2e30ce4da380448 # New test vectors.") violates
YAMLlint; update that line to use two spaces before the inline comment so it
reads "ref: ba76542dbf7a0c72d414a87ad2e30ce4da380448 # New test vectors." to
satisfy the linter.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 67-71: Replace the moving ref currently used in the checkout step
named "Checkout base branch for baseline" (uses: actions/checkout@v4 with ref:
${{ github.base_ref }}) with the immutable PR payload SHA; change the ref to use
github.event.pull_request.base.sha so the baseline checkout is reproducible
across runs and does not drift.
- Around line 84-88: The benchmark compare step is using inconsistent paths
between the copy command (cp ./bench/results.json ../results.json) and the
compare invocation (npm run bench:compare ../baseline.json ../results.json --
--threshold 5); fix by making both sides use the same location — either write
the PR results to the repo-root or change the compare arguments to read from the
directory where baseline and results actually reside. Update the cp command or
the npm run bench:compare arguments accordingly so "Compare benchmarks"
references the exact same baseline and results file locations.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/build.yml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
67-71:⚠️ Potential issue | 🟠 MajorUse a fixed SHA instead of a branch name for baseline checkout.
${{ github.base_ref }}is a branch name that points to the current HEAD of the base branch. If the base branch advances before this workflow runs, the baseline will differ between runs, making results non-reproducible.Use
${{ github.event.pull_request.base.sha }}instead, which is a fixed commit SHA from the pull request payload.Proposed fix
- name: Checkout base branch for baseline uses: actions/checkout@v4 with: - ref: ${{ github.base_ref }} + ref: ${{ github.event.pull_request.base.sha }} path: ./baseline🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 67 - 71, The workflow is checking out the base branch using ref: ${{ github.base_ref }}, which is mutable and makes baselines non-reproducible; update the checkout step that uses actions/checkout@v4 (the "Checkout base branch for baseline" step with path: ./baseline) to use the fixed commit SHA from the PR payload by replacing ref: ${{ github.base_ref }} with ref: ${{ github.event.pull_request.base.sha }} and ensure this job runs on an event that includes the pull_request payload so the base.sha value is available.
79-92:⚠️ Potential issue | 🔴 CriticalBenchmark comparison paths are inconsistent and will break the job.
Path analysis:
- Line 79: Baseline runs in
./baseline, copies to../baseline.json→ ends up at repo root- Line 88: PR benchmark runs at repo root, copies to
../results.json→ ends up at parent of repo root- Line 92: Compare reads
../baseline.jsonand../results.json→ looks in parent of repo rootThe baseline file is written to repo root but the compare step looks for it in the parent directory.
Proposed fix
- name: Run baseline benchmark run: | cd ./baseline npm ci npm run build if npm run bench:baseline 2>/dev/null; then - cp ./bench/baseline.json ../baseline.json + cp ./bench/baseline.json ./baseline.json else echo "bench:baseline script not found on base branch, skipping comparison" - echo '{}' > ../baseline.json + echo '{}' > ./baseline.json fi # Run benchmarks on current PR - name: Run PR benchmark run: | npm run bench:ci - cp ./bench/results.json ../results.json + cp ./bench/results.json ./results.json # Compare results - name: Compare benchmarks run: | - npm run bench:compare ../baseline.json ../results.json -- --threshold 5 + npm run bench:compare ./baseline/baseline.json ./results.json -- --threshold 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 79 - 92, The benchmark steps use inconsistent relative paths causing the compare step to miss files; make the baseline, PR results, and compare all reference the same locations (e.g., ./baseline.json and ./results.json). Concretely, update the cp in the baseline branch-check block (currently "cp ./bench/baseline.json ../baseline.json") and the PR benchmark cp (currently "cp ./bench/results.json ../results.json") so both write to the same directory (e.g., "cp ./bench/baseline.json ./baseline.json" and "cp ./bench/results.json ./results.json"), and update the Compare benchmarks invocation (currently "npm run bench:compare ../baseline.json ../results.json -- --threshold 5") to use those same paths ("npm run bench:compare ./baseline.json ./results.json -- --threshold 5") so the "Run PR benchmark" and "Compare benchmarks" steps find the files consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 67-71: The workflow is checking out the base branch using ref: ${{
github.base_ref }}, which is mutable and makes baselines non-reproducible;
update the checkout step that uses actions/checkout@v4 (the "Checkout base
branch for baseline" step with path: ./baseline) to use the fixed commit SHA
from the PR payload by replacing ref: ${{ github.base_ref }} with ref: ${{
github.event.pull_request.base.sha }} and ensure this job runs on an event that
includes the pull_request payload so the base.sha value is available.
- Around line 79-92: The benchmark steps use inconsistent relative paths causing
the compare step to miss files; make the baseline, PR results, and compare all
reference the same locations (e.g., ./baseline.json and ./results.json).
Concretely, update the cp in the baseline branch-check block (currently "cp
./bench/baseline.json ../baseline.json") and the PR benchmark cp (currently "cp
./bench/results.json ../results.json") so both write to the same directory
(e.g., "cp ./bench/baseline.json ./baseline.json" and "cp ./bench/results.json
./results.json"), and update the Compare benchmarks invocation (currently "npm
run bench:compare ../baseline.json ../results.json -- --threshold 5") to use
those same paths ("npm run bench:compare ./baseline.json ./results.json --
--threshold 5") so the "Run PR benchmark" and "Compare benchmarks" steps find
the files consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/build.ymlassembly/gas.ts
This was referenced Feb 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Top 10 Trace Improvements (PR vs Baseline)
Baseline - Pre #178 commit.