warp/native/deterministic.h (1)

41-49: Clarify the non-CUDA branch comment.

Line 41 says “direct accumulation,” but Lines 42-48 are a no-op. Rewording this would prevent confusion during maintenance.

✏️ Suggested comment tweak

-    // CPU path: direct accumulation (CPU kernels are sequential).
+    // Non-CUDA path: no-op for this helper (CPU execution does not use scatter buffers).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/native/deterministic.h` around lines 41 - 49, The comment "CPU path:
direct accumulation (CPU kernels are sequential)" is misleading because the
non-CUDA branch simply voids a set of unused variables (keys, values, counter,
capacity, dest_flat_idx, thread_id, value); change the comment near that void
list in deterministic.h to clearly state these variables are intentionally
unused in the non-CUDA build (e.g., "Non-CUDA build: no per-thread accumulation
— explicitly mark these kernel-specific variables as unused") so maintainers
understand why the (void)XXX lines are present.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

warp/_src/deterministic.py (1)

140-143: ⚠️ Potential issue | 🔴 Critical

Fail fast for warp.float16 deterministic reductions.

warp.float16 is still wired through the deterministic scatter/reduce path, but the native entry points only reinterpret buffers as float* or double*. Half buffers will therefore be read with the wrong element size and produce corrupted reductions. Please reject this path until a real half reducer exists.

Minimal safe guard

-        if target.value_ctype in ("float", "wp::half"):
+        if target.value_ctype == "float":
             fn = runtime.core.wp_deterministic_sort_reduce_float_device
+        elif target.value_ctype == "wp::half":
+            raise RuntimeError("Deterministic float16 atomics are not supported yet.")
         elif target.value_ctype == "double":
             fn = runtime.core.wp_deterministic_sort_reduce_double_device

Also applies to: 159-161, 177-180, 219-223

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/deterministic.py` around lines 140 - 143, The mapping
_WARP_TO_CTYPE currently includes warp.float16 but the deterministic
scatter/reduce path does not support half-precision and will read buffers with
wrong element size; update deterministic.py to fail fast whenever warp.float16
is encountered in the deterministic reduction code paths by removing or guarding
the warp.float16 entry in _WARP_TO_CTYPE and adding an explicit check that
raises a clear exception (or returns an error) when a reducer or conversion
function (the code paths that reference _WARP_TO_CTYPE) sees warp.float16;
ensure the exception message names warp.float16 and the deterministic reduction
path so callers get a clear rejection until a proper half reducer is
implemented.

warp/tests/test_deterministic.py (1)

955-956: ⚠️ Potential issue | 🟠 Major

Remove the kernel-cache clear from this test module.

wp.clear_kernel_cache() is disallowed in tests and can crash parallel CI runs.

Minimal fix

 if __name__ == "__main__":
-    wp.clear_kernel_cache()
     unittest.main(verbosity=2)

As per coding guidelines, "Never call wp.clear_kernel_cache() or wp.clear_lto_cache() in test files—not in __main__ blocks, test methods, or module scope. Cache clearing is not multi-process-safe; concurrent clears cause LLVM crashes."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/tests/test_deterministic.py` around lines 955 - 956, Remove the
disallowed kernel-cache clear from the module-level __main__ block: delete the
call to wp.clear_kernel_cache() found in the if __name__ == "__main__": section
of warp/tests/test_deterministic.py; do not replace it with any cache-clearing
call (wp.clear_lto_cache or similar) and ensure no other module-scope or
__main__-scoped cache-clear calls remain.

design/deterministic-execution.md (1)

111-112: ⚠️ Potential issue | 🟡 Minor

Use one scan convention throughout the doc.

Lines 111-112 describe an exclusive scan, but Lines 170-172 still say the total comes from the last element of an inclusive scan. Please make the writeback rule consistent with the convention the implementation actually uses.

Also applies to: 170-172

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@design/deterministic-execution.md` around lines 111 - 112, The doc is
inconsistent about scan convention: wp.utils.array_scan(contrib, prefix,
inclusive=False) is described as exclusive at lines 111-112 but later (lines
170-172) claims the total comes from the last element of an inclusive scan; pick
one convention and make the writeback rule consistent with the actual
implementation. Update the description of wp.utils.array_scan, the
example/notation for "prefix" and the writeback rule (the statement about where
the total/last-offset is read) so they all use the same convention (either
inclusive or exclusive) and mention the inclusive flag (inclusive=False/True) in
the writeback explanation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

warp/tests/test_deterministic.py (1)

1143-1145: ⚠️ Potential issue | 🟠 Major

Remove the kernel-cache clear from this test module.

wp.clear_kernel_cache() is explicitly disallowed in test files and can break parallel test runs.

Minimal fix

 if __name__ == "__main__":
-    wp.clear_kernel_cache()
     unittest.main(verbosity=2)

As per coding guidelines, "Never call wp.clear_kernel_cache() or wp.clear_lto_cache() in test files—not in __main__ blocks, test methods, or module scope. Cache clearing is not multi-process-safe; concurrent clears cause LLVM crashes."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/tests/test_deterministic.py` around lines 1143 - 1145, Remove the call
to wp.clear_kernel_cache() from the module-level __main__ block; locate the if
__name__ == "__main__": block (where wp.clear_kernel_cache() is called before
unittest.main) and delete that wp.clear_kernel_cache() invocation so tests no
longer call kernel-cache clearing from the test module.

design/deterministic-execution.md (1)

114-115: ⚠️ Potential issue | 🟡 Minor

Use one scan convention throughout the design.

The Pattern B section documents array_scan(..., inclusive=False), but the writeback section still says the total comes from the last element of an inclusive scan. Please pick one convention and make the total-count rule match it everywhere.

Also applies to: 173-175

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@design/deterministic-execution.md` around lines 114 - 115, The docs use two
different scan conventions; standardize on one and make all references
consistent: choose whether wp.utils.array_scan(contrib, prefix, inclusive=True)
or inclusive=False is the canonical API, then update Pattern B and the writeback
section (and the other occurrence at lines ~173-175) so the phrase "total comes
from the last element" matches that convention (e.g., if using inclusive=False,
state that total = prefix[-1] + contrib[-1] or if inclusive=True, state total =
prefix[-1]); update any explanatory text and examples to reference
wp.utils.array_scan(..., inclusive=...) and the single total-count rule
consistently.

warp/tests/test_deterministic.py (1)

692-753: Assert the actual slot order, not just counts/permutations.

counter_kernel should yield output == data_np, and conditional_counter_kernel should yield output[:expected_count] == data_np[data_np > threshold]. Right now a stable but wrong permutation would still pass because these tests only check counts, sorting, and cross-run reproducibility.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/tests/test_deterministic.py` around lines 692 - 753, Update the two
tests to assert actual slot order rather than just counts/permutations: in
test_counter_correctness replace the sorted/permutation check with a direct
equality assertion that output.numpy() (or output.numpy().tolist()) exactly
equals data_np (ensuring dtype/shape match) to verify counter_kernel produces
output == data_np; in test_conditional_counter replace the permutation-based
checks with a direct equality assertion that output.numpy()[:expected_count]
equals data_np[data_np > threshold] to verify conditional_counter_kernel
preserves the original relative order of selected elements; keep the existing
cross-run determinism checks but change the comparisons to elementwise equality
rather than sorted/comparison of permutations.

warp/_src/context.py (2)

7628-7641: ⚠️ Potential issue | 🟠 Major

Keep the count/scan/reduce fixups on stream, and accumulate the existing counter value.

array_scan(), warp.copy(), and run_sort_reduce() all omit stream here, so an explicit non-current stream or graph capture can reorder or escape the deterministic pass. Also, Line 7641 writes only this launch's total into counter_arr[0]; a counter that started at N will end at total instead of N + total.

Also applies to: 7670-7670

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/context.py` around lines 7628 - 7641, The code omits the stream
argument and overwrites existing counter values; update calls to
warp._src.utils.array_scan(contrib, prefix, inclusive=False) and the inclusive
scan warp._src.utils.array_scan(contrib, inclusive_out, inclusive=True) as well
as warp.copy and any run_sort_reduce invocations to pass the current non-default
stream (preserve graph capture/determinism), and change the write into
counter_arr (found by iterating kernel.adj.args for ct.array_var_label and
obtained from fwd_args[j]) to add the inclusive_out last-element to the existing
counter value instead of replacing it (i.e., read counter_arr[0], add
inclusive_out[dim_size-1], and write the sum back using warp.copy on the given
stream); apply the same stream+accumulate fix to the similar block around the
other occurrence near the 7670 location.

---

7505-7508: ⚠️ Potential issue | 🟠 Major

Raw-ctype array updates are still unsafe for deterministic replays.

Line 7507 only keeps fwd_args aligned for set_param_at_index(). The inherited set_param_at_index_from_ctype() / set_params_from_ctypes() can still replace an array descriptor while _launch_deterministic() later uses the stale Python array object for warp.copy() and run_sort_reduce(). Recorded deterministic launches can therefore replay into the wrong array, or fail once fwd_args and self.params diverge. If these APIs stay exposed, DeterministicLaunch needs to reject array-ctype updates or carry the owning Warp array alongside the packed descriptor.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/context.py` around lines 7505 - 7508, The deterministic replay bug:
set_param_at_index only updates fwd_args but inherited
set_param_at_index_from_ctype / set_params_from_ctypes can replace raw C-type
array descriptors in self.params leaving fwd_args stale, which breaks
_launch_deterministic and later warp.copy/run_sort_reduce replay; to fix, either
(A) make DeterministicLaunch reject/raise when set_param_at_index_from_ctype or
set_params_from_ctypes is used with array-ctypes, or (B) ensure those
ctype-updating paths also update the corresponding Python owning Warp array
stored in fwd_args (and keep self.params and fwd_args synchronized) so that
set_param_at_index_from_ctype / set_params_from_ctypes maintain alignment with
fwd_args before _launch_deterministic runs.

design/deterministic-execution.md (1)

114-115: ⚠️ Potential issue | 🟡 Minor

Use one scan convention in the design doc.

Line 114 documents wp.utils.array_scan(contrib, prefix, inclusive=False), but Lines 176-178 say the total comes from the last element of an inclusive scan. Please make both sections describe the same writeback rule. If the scan stays exclusive, the total is prefix[-1] + contrib[-1].

Also applies to: 176-178

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@design/deterministic-execution.md` around lines 114 - 115, The doc currently
mixes scan conventions: update both occurrences to use the same convention for
wp.utils.array_scan(contrib, prefix, inclusive=False) and the writeback rule;
either change the function call to inclusive=True and state the total is
prefix[-1], or keep inclusive=False and change the total calculation text (the
section that currently says the total comes from the last element of an
inclusive scan) to the exclusive rule: total = prefix[-1] + contrib[-1]; make
this consistent for the wp.utils.array_scan(contrib, prefix, inclusive=False)
mention and the later total computation paragraph.

warp/tests/test_deterministic.py (1)

1190-1192: ⚠️ Potential issue | 🟠 Major

Remove the kernel-cache clear from this test module.

wp.clear_kernel_cache() is explicitly disallowed in test files and can crash parallel test runs. unittest.main() is enough here.

🧹 Minimal fix

 if __name__ == "__main__":
-    wp.clear_kernel_cache()
     unittest.main(verbosity=2)

As per coding guidelines, "Never call wp.clear_kernel_cache() or wp.clear_lto_cache() in test files—not in __main__ blocks, test methods, or module scope. Cache clearing is not multi-process-safe; concurrent clears cause LLVM crashes."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/tests/test_deterministic.py` around lines 1190 - 1192, Remove the
explicit wp.clear_kernel_cache() call from the module-level __main__ block in
this test file; keep only the unittest.main(verbosity=2) invocation so the test
can run without clearing kernel caches (do not add wp.clear_lto_cache() either).
Locate the __main__ guard containing wp.clear_kernel_cache() and delete that
call, ensuring the block now simply calls unittest.main(...) and nothing else.

warp/_src/deterministic.py (1)

228-231: ⚠️ Potential issue | 🟠 Major

Counter scratch buffers still hard-code wp.int32.

CounterTarget.value_ctype is tracked, but contrib and prefix are always allocated as wp.int32. Any deterministic wp.int64/wp.uint64 counter will silently wrap in Phase 0/1. Please make these scratch buffers follow value_ctype, or fail fast for non-32-bit counters until the wider ABI is wired through end-to-end.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/deterministic.py` around lines 228 - 231, The loop that allocates
counter scratch buffers currently hardcodes dtype=warp.int32 for contrib and
prefix; change the allocation to use each CounterTarget's value_ctype (e.g., use
dtype=target.value_ctype or map that C type to the corresponding warp dtype) so
contrib and prefix match CounterTarget.value_ctype and avoid silent wrapping, or
add an explicit fast-fail that raises if target.value_ctype is not a 32-bit
integer type (when full 64-bit ABI isn't supported). Update the allocations in
the for _target in counter_targets block (the contrib and prefix
warp.zeros/warp.empty calls) to derive dtype from the target's value_ctype and
keep shape= (dim_size,) and device=device.

warp/tests/test_deterministic.py (1)

850-879: test_module_option_override() doesn't prove the override is active on CUDA.

A single approximate-sum assertion will pass whether this kernel took the deterministic path or the normal atomic path, especially on CPU where both paths are deterministic. Please mirror test_kernel_decorator_override() here: run the kernel several times on CUDA and assert bit-exact equality across runs, then keep the sum check as a secondary sanity check if you want.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/tests/test_deterministic.py` around lines 850 - 879,
test_module_option_override currently only checks approximate sums so it doesn't
verify the per-module deterministic=True override on CUDA; update the test
(function test_module_option_override) to mirror test_kernel_decorator_override
by: when device is CUDA (check device == "cuda" or via wp.get_device_name/device
backend), run the kernel per_kernel_det multiple times (e.g., 3+ runs) into
separate outputs and assert bit-exact equality of the outputs across runs (use
output.numpy() equality checks) to prove determinism, while retaining the
existing sum/assert_allclose sanity check as a secondary assertion; ensure you
still toggle wp.config.deterministic = False around the launch to verify the
per-module override takes effect.

warp/_src/deterministic.py (1)

193-193: Annotate device with DeviceLike.

These helper signatures take device untyped, which diverges from the repo's device-parameter convention and makes this launch path harder to type-check consistently.

As per coding guidelines, "Use DeviceLike type annotation (from warp._src.context) for device parameters. Import under TYPE_CHECKING to avoid circular imports."

Also applies to: 222-222, 235-235

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/deterministic.py` at line 193, The device parameter in
allocate_scatter_buffers should be annotated with DeviceLike: import DeviceLike
from warp._src.context inside a TYPE_CHECKING block (from typing import
TYPE_CHECKING) to avoid circular imports, then change the function signature to
accept device: DeviceLike; apply the same change to the other two nearby helper
functions in this module that take a device parameter so all device-typed helper
signatures use DeviceLike consistently.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

warp/_src/context.py (3)

7532-7535: ⚠️ Potential issue | 🟠 Major

Keep fwd_args synchronized for ctype-based param mutators too.

Line 7532 only syncs self.fwd_args for set_param_at_index(). Calls through set_param_at_index_from_ctype() / set_params_from_ctypes() can still leave fwd_args stale, so deterministic replay can post-process the wrong arrays.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/context.py` around lines 7532 - 7535, The fwd_args list is only
updated in set_param_at_index, so calls that mutate params via ctype helpers
leave fwd_args stale; modify set_param_at_index_from_ctype and
set_params_from_ctypes to also synchronize self.fwd_args (either by delegating
to set_param_at_index for each change or by applying the same
index-check-and-assign logic used there), honoring the adjoint flag so adjoint
updates do not overwrite fwd_args.

---

7541-7553: ⚠️ Potential issue | 🟡 Minor

Preserve the forward-hook null guard in deterministic launch routing.

These deterministic branches call _launch_deterministic() without the regular forward-hook existence check. If hooks.forward is missing, this can pass a null kernel handle to CUDA launch instead of raising the clearer Python-side error.

Also applies to: 7825-7854

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/context.py` around lines 7541 - 7553, The deterministic launch
branches call _launch_deterministic(...) without checking for hooks.forward,
which can allow a null kernel handle to reach CUDA; add the same forward-hook
null guard used in the non-deterministic path: verify hooks.forward (or
equivalent forward kernel handle) is present before invoking
_launch_deterministic, and if missing raise the same Python-side error/exception
used elsewhere so we fail early and clearly; apply the same fix to the other
deterministic branch around the second occurrence noted (near the 7825-7854
block).

---

7657-7670: ⚠️ Potential issue | 🟠 Major

Two-pass deterministic path still breaks explicit-stream ordering and counter continuity.

Line 7657/Line 7664 scans, Line 7670 counter writeback, and Line 7699 sort-reduce are not stream-bound in this path. On non-current streams/capture this can reorder work. Also, Line 7670 overwrites the counter with this-launch total instead of preserving and adding the pre-existing counter value.

Also applies to: 7699-7699

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/context.py` around lines 7657 - 7670, The two-pass deterministic
path uses array_scan and warp.copy without binding them to the current
stream/capture and overwrites the counter rather than adding to the pre-existing
value; update the logic around warp._src.utils.array_scan(contrib, ...), the
inclusive_out buffer, and the writeback that finds counter_arr via
kernel.adj.args and ct.array_var_label so that (1) all scans, the inclusive_out
usage, the warp.copy writeback, and the later sort-reduce are submitted/bound to
the same current stream/capture to preserve ordering, and (2) when writing the
total into counter_arr you first read the existing counter value from
counter_arr and add the inclusive_out last element (the this-launch total) to it
(or perform an atomic-add equivalent) instead of blindly overwriting; ensure the
same stream-binding fix is applied to the sort-reduce at the other location
(around the existing sort-reduce call).

design/deterministic-execution.md (1)

129-130: ⚠️ Potential issue | 🟡 Minor

Use one scan convention throughout the design doc.

Pattern B says the prefix pass uses inclusive=False, but the writeback section says the total comes from “the last element of the inclusive scan.” Those are different rules, so the doc currently describes two incompatible implementations.

Also applies to: 191-193

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@design/deterministic-execution.md` around lines 129 - 130, The doc
inconsistently specifies scan conventions: Pattern B states the prefix pass uses
wp.utils.array_scan(contrib, prefix, inclusive=False) while the writeback
section refers to “the last element of the inclusive scan”; pick one convention
and update all references to match. Concretely, decide whether prefix scans are
inclusive or exclusive, then change mentions in Pattern B, the writeback
section, and other occurrences (e.g., lines ~191-193) so they all reference the
same wp.utils.array_scan(..., inclusive=...) behavior and explain how the total
is derived (either from the last element of the inclusive scan or from
last+contrib for exclusive).

warp/_src/deterministic.py (2)

283-293: ⚠️ Potential issue | 🟠 Major

Match the counter scratch buffers to the counter width.

CounterTarget.value_ctype is tracked, but allocate_counter_buffers() always allocates wp.int32 for both scratch arrays. That will truncate deterministic int64/uint64 counters unless they are rejected earlier. Either allocate these buffers from value_ctype, or fail fast for non-32-bit counters.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/deterministic.py` around lines 283 - 293, The counter scratch
buffers are always allocated as warp.int32 in allocate_counter_buffers, which
will truncate 64-bit counters; update allocate_counter_buffers to use each
CounterTarget's value_ctype (CounterTarget.value_ctype) when creating contrib
and prefix (i.e., pass dtype=_target.value_ctype to warp.zeros/warp.empty) or
alternatively raise/assert if _target.value_ctype is not a 32-bit type so
non-32-bit counters fail fast; ensure you reference the function
allocate_counter_buffers and the CounterTarget.value_ctype field when making the
change.

---

189-200: ⚠️ Potential issue | 🟠 Major

Don't merge every logical counter in an array into one deterministic target.

get_or_create_counter_target() only keys on array_var_label. If deterministic mode ever sees wp.atomic_add(counter, bucket, 1), every bucket in that array will reuse one contrib/prefix stream and Phase 1 will hand out slots from a single global sequence. Please include the logical counter index in the target identity, or reject non-constant-zero counter indices before creating a CounterTarget.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/deterministic.py` around lines 189 - 200,
get_or_create_counter_target currently deduplicates targets solely by
array_var_label which causes all array elements to share one deterministic
counter; update get_or_create_counter_target to include the logical counter
index in the identity (e.g., use a tuple of (array_var_label, logical_index) or
add a logical_index field to CounterTarget) so each distinct bucket gets its own
CounterTarget, and before creating a new CounterTarget validate the provided
index expression is a constant zero (or reject/non-deterministic indices) if
your design only allows index 0; ensure you update the lookup loop to compare
the new key (array_var_label plus logical index) and append the new
CounterTarget with the logical index populated when creating it.

warp/_src/context.py (1)

1339-1341: Validate deterministic_max_records early for type/range.

This value is currently accepted broadly and coerced later in launch. Adding an upfront int + non-negative validation in option ingestion would fail fast and avoid silent coercion paths.

Also applies to: 2583-2585

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/_src/context.py` around lines 1339 - 1341, Validate
deterministic_max_records immediately before assigning into kernel_options:
ensure the value is an int (or can be safely converted) and is >= 0, raising a
TypeError for non-int types and a ValueError for negative values so callers fail
fast; then assign kernel_options["deterministic_max_records"] =
deterministic_max_records only after validation. Apply the same validation logic
at the other ingestion site referenced around lines 2583-2585 to keep behavior
consistent.

warp/tests/test_unique_module.py (1)

170-205: Keep the hash assertion runnable on CPU-only jobs.

Line 172 skips the whole test, but the module.name comparison at Lines 189-193 is device-independent. Splitting this into a CPU-safe hashing assertion plus a CUDA-only launch check would preserve coverage for the unique-module hashing change on non-CUDA runners.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@warp/tests/test_unique_module.py` around lines 170 - 205, The test currently
skips the entire test when CUDA is unavailable, but the module name hashing
assertion in test_kernel_options_affect_unique_module_identity (comparing
_scatter_normal.module.name and _scatter_deterministic.module.name) is
device-independent; change the test to perform the module.name comparison
unconditionally, and only guard the CUDA-specific array creation and wp.launch
calls with if not wp.is_cuda_available(): self.skipTest(...) or conditional
blocks around the CUDA-only code (values/indices/out_* with device="cuda:0" and
the wp.launch calls) so the hashing assertion still runs on CPU-only CI while
the deterministic launch checks remain CUDA-only.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes