fix(detect): stop probe quickly after DA1 sentinel#106
Merged
RtlZeroMemory merged 5 commits intomainfrom Mar 4, 2026
Merged
Conversation
|
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 DA1/XTVersion partial-response detection and a per-detection "drain" timeout budget to zr_detect_probe_terminal, clamping per-iteration timeouts and tracking elapsed drain time; includes helper functions and a unit test to validate the behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/zr_test.c (1)
117-120:⚠️ Potential issue | 🟡 MinorGuard context-end arithmetic against
size_toverflow.
end = mismatch_off + ctx_aftercan wrap on very large buffers, producing invalid context bounds in diagnostics. Use a saturating computation.Proposed fix
- size_t end = mismatch_off + ctx_after; - if (end > len) { - end = len; - } + size_t end = len; + if (mismatch_off < len) { + const size_t remaining = len - mismatch_off; + end = mismatch_off + (remaining < ctx_after ? remaining : ctx_after); + }As per coding guidelines, follow the Safe C rulebook defined in
docs/SAFETY_RULESET.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zr_test.c` around lines 117 - 120, The computation for end currently uses end = mismatch_off + ctx_after which can overflow size_t; change the logic in tests/zr_test.c around the variables end, mismatch_off, ctx_after and len to use saturating arithmetic: first detect overflow (e.g. if (mismatch_off > SIZE_MAX - ctx_after) set end to SIZE_MAX or directly to len), otherwise compute end = mismatch_off + ctx_after, then clamp end = (end > len) ? len : end so end can never wrap and will be bounded by len; ensure you include <stdint.h> or <limits.h> if needed for SIZE_MAX.
🧹 Nitpick comments (9)
examples/zr_example_common.h (1)
34-36: Replace bare alignment literals with a named constant.Line 35 still embeds
3u/~3u. Please name the mask to align with repository style rules.Proposed refactor
static inline uint32_t zr_ex_align4_u32(uint32_t x) { - return (x + 3u) & ~3u; + enum { ZR_EX_ALIGN4_MASK_U32 = 3u }; + return (x + ZR_EX_ALIGN4_MASK_U32) & ~ZR_EX_ALIGN4_MASK_U32; }As per coding guidelines: "Extract magic numbers to named constants with descriptive names instead of using bare literals."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/zr_example_common.h` around lines 34 - 36, The function zr_ex_align4_u32 uses bare literals 3u and ~3u; define a named constant (e.g., static const uint32_t ZR_EX_ALIGN4_MASK = 3u) and replace the literals so the implementation becomes (x + ZR_EX_ALIGN4_MASK) & ~ZR_EX_ALIGN4_MASK; update zr_ex_align4_u32 to reference ZR_EX_ALIGN4_MASK to satisfy the repo style rule against magic numbers.tests/unit/test_blit.c (1)
153-205: Refactor this test to meet function-size and section-marker conventions.
blit_wide_glyph_lead_does_not_write_outside_rectexceeds the 50-line cap and would be clearer with explicit/* --- Arrange/Act/Assert --- */markers plus a small helper extraction.As per coding guidelines, "
**/*.c: Functions should be 20-40 lines, with a maximum of 50 lines" and "Add section markers in tests (e.g.,/* --- Arrange/Act/Assert --- */)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_blit.c` around lines 153 - 205, The test blit_wide_glyph_lead_does_not_write_outside_rect is too long and needs Arrange/Act/Assert markers and a small helper to shrink it; refactor by extracting the setup of the framebuffer and seeded wide glyph into a helper function (e.g., create_seeded_fb_with_wide_glyph or init_fb_with_wide_glyph) and move the painter begin + blit invocation into a short Act section, then add clear comment markers /* --- Arrange --- */, /* --- Act --- */, /* --- Assert --- */ inside blit_wide_glyph_lead_does_not_write_outside_rect and ensure the test body is <50 lines by calling the helper for setup and keeping assertions in the Assert section (keep references to zr_fb_init, zr_fb_cell, zr_fb_painter_begin, zr_fb_blit_rect, and zr_fb_release so reviewers can locate changes).tests/unit/test_limits.c (1)
215-273: Consider splitting this test and adding explicit AAA section markers.This test exceeds the 50-line cap and would be easier to maintain if split into small helpers with
/* --- Arrange/Act/Assert --- */sections.As per coding guidelines, "
**/*.c: Functions should be 20-40 lines, with a maximum of 50 lines" and "Add section markers in tests (e.g.,/* --- Arrange/Act/Assert --- */)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_limits.c` around lines 215 - 273, The test function limits_link_intern_compacts_stale_refs_and_bounds_growth is over the 50-line guideline; split it into smaller tests/helpers (e.g., setup_fb_and_persistent_link(), intern_ephemeral_links_loop(), and verify_peak_bounds_and_lookup()) so each function/test stays within 20–40 lines (max 50), and add explicit section markers /* --- Arrange --- */, /* --- Act --- */, and /* --- Assert --- */ inside each test to clarify phases; keep references to the same symbols (zr_fb_t fb, zr_fb_init, zr_fb_link_intern, zr_fb_cell, fb.links_len, fb.link_bytes_len, zr_fb_link_lookup, zr_fb_release) when moving code into helpers and ensure assertions remain in the Assert sections.tests/CMakeLists.txt (1)
78-78: Minor: Test file ordering inconsistency.The new test file
test_engine_present_damage_resync_hyperlinks.cis placed aftertest_engine_present_single_flush.c. For consistency with othertest_engine_present_*files, it would fit better aftertest_engine_present_backpressure.c(line 75) and beforetest_engine_restore_hooks.c(line 76).This is purely organizational and doesn't affect functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/CMakeLists.txt` at line 78, Move the test entry "unit/test_engine_present_damage_resync_hyperlinks.c" so the ordering of test_engine_present_* files is consistent: place it after "unit/test_engine_present_backpressure.c" and before "unit/test_engine_restore_hooks.c" in the tests/CMakeLists.txt so that all test_engine_present_* entries remain grouped and ordered similarly to the existing sequence.src/core/zr_framebuffer.c (1)
507-625: Function exceeds 50-line guideline; consider splitting.
zr_fb_links_compact_liveis approximately 112 lines. Per coding guidelines, functions should be 20-40 lines with a maximum of 50 lines. Consider extracting logical sections into helpers:
- Mark live refs (lines 552-563) →
zr_fb_links_mark_live_refs- Relocate entries (lines 569-607) →
zr_fb_links_relocate_entries- Remap cell refs (lines 609-619) →
zr_fb_links_remap_cell_refsThe function already has clear logical sections that would map well to helpers.
♻️ Suggested structure
/* Mark which link refs are live in the remap array. */ static void zr_fb_links_mark_live(const zr_fb_t* fb, uint32_t* remap, size_t cell_count); /* Relocate live link entries and update remap with new refs. */ static zr_result_t zr_fb_links_relocate(zr_fb_t* fb, uint32_t* remap, uint32_t old_len, uint32_t* out_new_links_len, uint32_t* out_new_bytes_len); /* Apply remap to all cell link refs. */ static void zr_fb_links_remap_cells(zr_fb_t* fb, const uint32_t* remap, uint32_t old_links_len, size_t cell_count);As per coding guidelines: "Functions should be 20-40 lines, with a maximum of 50 lines."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/zr_framebuffer.c` around lines 507 - 625, zr_fb_links_compact_live is too large; split its logical sections into helpers to meet the 20-40 line guideline by extracting the live-marking loop into zr_fb_links_mark_live_refs(const zr_fb_t*, uint32_t*, size_t), the relocation/compaction loop into zr_fb_links_relocate_entries(zr_fb_t*, uint32_t*, uint32_t old_links_len, uint32_t* out_new_links_len, uint32_t* out_new_link_bytes_len) which returns zr_result_t on errors, and the final remap loop into zr_fb_links_remap_cell_refs(zr_fb_t*, const uint32_t*, uint32_t old_links_len, size_t cell_count); ensure zr_fb_links_compact_live allocates/frees remap and delegates work to these helpers, propagates errors from zr_fb_links_relocate_entries, and preserves existing validations and side-effects (sanitizing out-of-range refs, updating fb->links_len and fb->link_bytes_len).tests/unit/test_diff_hotpath_telemetry.c (2)
16-26: Extractzr_bytes_containsinto a shared test helper.This helper duplicates logic already used in
test_diff_extended_styles.c(Line 112-122 in the provided context). Centralizing it avoids drift between tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_diff_hotpath_telemetry.c` around lines 16 - 26, The function zr_bytes_contains in tests/unit/test_diff_hotpath_telemetry.c duplicates logic used in test_diff_extended_styles.c; extract it into a shared test helper (e.g., tests/helpers/bytes.c or tests/helpers/bytes.h) and replace the local definition in both files with an `#include` or extern reference to that helper. Move the implementation of zr_bytes_contains into the shared helper and update test files to call the same symbol (zr_bytes_contains) so both tests use the centralised implementation and avoid duplication.
296-361: Split or section this test body to meet project test-structure rules.This test is over the 50-line limit and would benefit from
/* --- Arrange/Act/Assert --- */section markers or a small setup helper.As per coding guidelines: Functions should be 20-40 lines (max 50), and tests should use section markers like
/* --- Arrange/Act/Assert --- */.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_diff_hotpath_telemetry.c` around lines 296 - 361, The test function ZR_TEST_UNIT(diff_row_cache_marks_dirty_when_hyperlink_targets_change_with_same_refs) is too long; split it into Arrange/Act/Assert sections or extract setup into a helper to meet the 20-40 line rule—move repeated initialization (zr_fb_init, zr_fb_clear, link interning, zr_limits_default, scratch setup, caps/initial state preparation, and buffer declarations) into a static helper (e.g., test_prepare_fb_and_scratch or test_setup_diff_inputs) and call it from a shorter test body that contains only the action (zr_diff_render_ex) and assertions on stats/out_len/zr_bytes_contains; add /* --- Arrange/Act/Assert --- */ markers in the test body to explicitly separate sections.tests/unit/test_engine_present_damage_resync_hyperlinks.c (1)
72-149: Replace builder wire-format literals with named constants.Values like
64u,60u, and repeated8uencode protocol structure and should be centralized to avoid subtle drift.As per coding guidelines: Replace magic thresholds/protocol constants with named constants in local enums/macros, and extract magic numbers to descriptive names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_engine_present_damage_resync_hyperlinks.c` around lines 72 - 149, The code uses hard-coded wire-format literals (e.g. 64u, 60u, 8u) across zr_builder_init, zr_builder_finish, zr_builder_cmd_def_string and zr_builder_cmd_draw_text which encode protocol structure; introduce descriptive constants (e.g. ZR_DL_HEADER_SIZE, ZR_DL_CMD_HEADER_SIZE, ZR_DL_DRAW_TEXT_SIZE or similar) in a local enum or macros and replace every literal occurrence (including the initial b->at default, the header field values written in zr_builder_finish, the reserved sizes passed to zr_builder_reserve, and the computed size in zr_builder_cmd_def_string) with those named constants so the protocol layout is centralized and consistent.src/core/zr_engine.c (1)
1538-1549: Factor duplicated rollback flow into a single helper.The rollback source selection +
zr_engine_fb_copy_noalloc+ trace/error path is duplicated in both failure branches.Refactor sketch
+static zr_result_t zr_engine_rollback_fb_next(zr_engine_t* e, const zr_fb_t* rollback_src) { + if (!e || !rollback_src) { + return ZR_ERR_INVALID_ARGUMENT; + } + return zr_engine_fb_copy_noalloc(rollback_src, &e->fb_next); +} ... - const zr_fb_t* rollback_src = have_fb_next_snapshot ? &e->fb_stage : &e->fb_prev; - const zr_result_t rollback_rc = zr_engine_fb_copy_noalloc(rollback_src, &e->fb_next); + const zr_fb_t* rollback_src = have_fb_next_snapshot ? &e->fb_stage : &e->fb_prev; + const zr_result_t rollback_rc = zr_engine_rollback_fb_next(e, rollback_src); ... - const zr_fb_t* rollback_src = have_fb_next_snapshot ? &e->fb_stage : &e->fb_prev; - const zr_result_t rollback_rc = zr_engine_fb_copy_noalloc(rollback_src, &e->fb_next); + const zr_fb_t* rollback_src = have_fb_next_snapshot ? &e->fb_stage : &e->fb_prev; + const zr_result_t rollback_rc = zr_engine_rollback_fb_next(e, rollback_src);Also applies to: 1557-1568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/zr_engine.c` around lines 1538 - 1549, Extract the duplicated rollback logic (selecting rollback_src, calling zr_engine_fb_copy_noalloc into e->fb_next, calling zr_image_frame_reset(&e->image_frame_stage) and zr_dl_resources_release(&e->dl_resources_stage), tracing with zr_engine_trace_drawlist and returning the rollback result) into a small helper, e.g. zr_engine_perform_rollback_or_trace(...) that takes the engine pointer e, the chosen source (or a flag to pick between e->fb_stage/e->fb_prev), the bytes/bytes_len/v and the post-failure rc to use in the trace; replace both duplicated branches with a call to that helper, ensure it returns and traces using rollback_rc (and returns rollback_rc on failure) and that the final successful path still calls the trace with rc as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/zr_engine_present.inc`:
- Around line 337-341: The multiplication computing links_bytes is vulnerable to
overflow; before using links_bytes in memcmp check within the block that reads
prefix->links_len and uses zr_fb_link_t, perform a checked multiplication (e.g.,
ensure prefix->links_len <= SIZE_MAX / sizeof(zr_fb_link_t) or use an existing
safe_mul helper) and if the check fails return false (or handle as an error) so
memcmp is never called with a wrapped/incorrect size; update the logic around
links_bytes, prefix->links_len, zr_fb_link_t, and the memcmp(prefix->links,
full->links, links_bytes) call accordingly.
In `@src/core/zr_engine.c`:
- Around line 81-84: The struct zr_engine_t contains small uint8_t fields
(fb_next_synced_to_prev and _pad_fb_sync0) that trigger clang-tidy "Excessive
padding" rules; fix by reordering or regrouping the small flag/pad bytes so they
pack naturally (e.g., place fb_next_synced_to_prev adjacent to other uint8_t
flags or replace separate padding fields with a single appropriately-sized
flag/pad member) to eliminate the unnecessary 3-byte pad; update the definition
of zr_engine_t to move or combine fb_next_synced_to_prev/_pad_fb_sync0 with
nearby uint8_t flags so the struct layout satisfies the linter.
In `@src/core/zr_framebuffer.c`:
- Around line 546-550: The per-frame hot path performs a malloc for the remap
buffer inside zr_fb_links_compact_live (called via zr_fb_link_intern →
zr_dl_preflight_resources → engine_submit_drawlist), which can allocate every
frame; replace this per-call heap allocation with a persistent buffer or
deferred compaction: add a remap buffer field to the framebuffer/context struct,
allocate or grow it once during init or capacity changes (e.g., in framebuffer
init/resize), and have zr_fb_links_compact_live use that buffer instead of
calling malloc; alternatively, if the persistent buffer is too small, return a
signal to defer compaction to an off-frame worker rather than allocating on the
hot path. Ensure you update allocation/cleanup code and sizing logic to avoid
per-frame mallocs and to keep thread-safety when deferring work.
In `@tests/unit/test_engine_present_damage_resync_hyperlinks.c`:
- Around line 80-101: Add defensive checks at the start of zr_builder_finish:
verify b and b->out are non-NULL, verify b->at is >= 64 (since the header
assumes 64 bytes of prior layout), and verify there is enough remaining space in
the output buffer to write the 64-byte header and any subsequent data (use the
builder's capacity/size field present in the struct — e.g., check whatever field
represents buffer capacity such as b->cap or b->size — and compare (capacity -
b->at) >= 64). If any check fails, return an error/zero or assert instead of
proceeding to call zr_w32 repeatedly; keep existing writes via zr_w32 and
b->cmd_count unchanged once checks pass.
In `@tests/unit/test_limits.c`:
- Around line 253-254: tests/unit/test_limits.c uses the ZR_MAX macro but does
not include its header; add an `#include` for util/zr_macros.h near the other
includes in this file so calls like ZR_MAX(peak_links_len, fb.links_len) and
ZR_MAX(peak_link_bytes_len, fb.link_bytes_len) compile, or alternatively replace
those ZR_MAX usages with equivalent ternary expressions (e.g., (a) > (b) ? (a) :
(b)).
---
Outside diff comments:
In `@tests/zr_test.c`:
- Around line 117-120: The computation for end currently uses end = mismatch_off
+ ctx_after which can overflow size_t; change the logic in tests/zr_test.c
around the variables end, mismatch_off, ctx_after and len to use saturating
arithmetic: first detect overflow (e.g. if (mismatch_off > SIZE_MAX - ctx_after)
set end to SIZE_MAX or directly to len), otherwise compute end = mismatch_off +
ctx_after, then clamp end = (end > len) ? len : end so end can never wrap and
will be bounded by len; ensure you include <stdint.h> or <limits.h> if needed
for SIZE_MAX.
---
Nitpick comments:
In `@examples/zr_example_common.h`:
- Around line 34-36: The function zr_ex_align4_u32 uses bare literals 3u and
~3u; define a named constant (e.g., static const uint32_t ZR_EX_ALIGN4_MASK =
3u) and replace the literals so the implementation becomes (x +
ZR_EX_ALIGN4_MASK) & ~ZR_EX_ALIGN4_MASK; update zr_ex_align4_u32 to reference
ZR_EX_ALIGN4_MASK to satisfy the repo style rule against magic numbers.
In `@src/core/zr_engine.c`:
- Around line 1538-1549: Extract the duplicated rollback logic (selecting
rollback_src, calling zr_engine_fb_copy_noalloc into e->fb_next, calling
zr_image_frame_reset(&e->image_frame_stage) and
zr_dl_resources_release(&e->dl_resources_stage), tracing with
zr_engine_trace_drawlist and returning the rollback result) into a small helper,
e.g. zr_engine_perform_rollback_or_trace(...) that takes the engine pointer e,
the chosen source (or a flag to pick between e->fb_stage/e->fb_prev), the
bytes/bytes_len/v and the post-failure rc to use in the trace; replace both
duplicated branches with a call to that helper, ensure it returns and traces
using rollback_rc (and returns rollback_rc on failure) and that the final
successful path still calls the trace with rc as before.
In `@src/core/zr_framebuffer.c`:
- Around line 507-625: zr_fb_links_compact_live is too large; split its logical
sections into helpers to meet the 20-40 line guideline by extracting the
live-marking loop into zr_fb_links_mark_live_refs(const zr_fb_t*, uint32_t*,
size_t), the relocation/compaction loop into
zr_fb_links_relocate_entries(zr_fb_t*, uint32_t*, uint32_t old_links_len,
uint32_t* out_new_links_len, uint32_t* out_new_link_bytes_len) which returns
zr_result_t on errors, and the final remap loop into
zr_fb_links_remap_cell_refs(zr_fb_t*, const uint32_t*, uint32_t old_links_len,
size_t cell_count); ensure zr_fb_links_compact_live allocates/frees remap and
delegates work to these helpers, propagates errors from
zr_fb_links_relocate_entries, and preserves existing validations and
side-effects (sanitizing out-of-range refs, updating fb->links_len and
fb->link_bytes_len).
In `@tests/CMakeLists.txt`:
- Line 78: Move the test entry
"unit/test_engine_present_damage_resync_hyperlinks.c" so the ordering of
test_engine_present_* files is consistent: place it after
"unit/test_engine_present_backpressure.c" and before
"unit/test_engine_restore_hooks.c" in the tests/CMakeLists.txt so that all
test_engine_present_* entries remain grouped and ordered similarly to the
existing sequence.
In `@tests/unit/test_blit.c`:
- Around line 153-205: The test blit_wide_glyph_lead_does_not_write_outside_rect
is too long and needs Arrange/Act/Assert markers and a small helper to shrink
it; refactor by extracting the setup of the framebuffer and seeded wide glyph
into a helper function (e.g., create_seeded_fb_with_wide_glyph or
init_fb_with_wide_glyph) and move the painter begin + blit invocation into a
short Act section, then add clear comment markers /* --- Arrange --- */, /* ---
Act --- */, /* --- Assert --- */ inside
blit_wide_glyph_lead_does_not_write_outside_rect and ensure the test body is <50
lines by calling the helper for setup and keeping assertions in the Assert
section (keep references to zr_fb_init, zr_fb_cell, zr_fb_painter_begin,
zr_fb_blit_rect, and zr_fb_release so reviewers can locate changes).
In `@tests/unit/test_diff_hotpath_telemetry.c`:
- Around line 16-26: The function zr_bytes_contains in
tests/unit/test_diff_hotpath_telemetry.c duplicates logic used in
test_diff_extended_styles.c; extract it into a shared test helper (e.g.,
tests/helpers/bytes.c or tests/helpers/bytes.h) and replace the local definition
in both files with an `#include` or extern reference to that helper. Move the
implementation of zr_bytes_contains into the shared helper and update test files
to call the same symbol (zr_bytes_contains) so both tests use the centralised
implementation and avoid duplication.
- Around line 296-361: The test function
ZR_TEST_UNIT(diff_row_cache_marks_dirty_when_hyperlink_targets_change_with_same_refs)
is too long; split it into Arrange/Act/Assert sections or extract setup into a
helper to meet the 20-40 line rule—move repeated initialization (zr_fb_init,
zr_fb_clear, link interning, zr_limits_default, scratch setup, caps/initial
state preparation, and buffer declarations) into a static helper (e.g.,
test_prepare_fb_and_scratch or test_setup_diff_inputs) and call it from a
shorter test body that contains only the action (zr_diff_render_ex) and
assertions on stats/out_len/zr_bytes_contains; add /* --- Arrange/Act/Assert ---
*/ markers in the test body to explicitly separate sections.
In `@tests/unit/test_engine_present_damage_resync_hyperlinks.c`:
- Around line 72-149: The code uses hard-coded wire-format literals (e.g. 64u,
60u, 8u) across zr_builder_init, zr_builder_finish, zr_builder_cmd_def_string
and zr_builder_cmd_draw_text which encode protocol structure; introduce
descriptive constants (e.g. ZR_DL_HEADER_SIZE, ZR_DL_CMD_HEADER_SIZE,
ZR_DL_DRAW_TEXT_SIZE or similar) in a local enum or macros and replace every
literal occurrence (including the initial b->at default, the header field values
written in zr_builder_finish, the reserved sizes passed to zr_builder_reserve,
and the computed size in zr_builder_cmd_def_string) with those named constants
so the protocol layout is centralized and consistent.
In `@tests/unit/test_limits.c`:
- Around line 215-273: The test function
limits_link_intern_compacts_stale_refs_and_bounds_growth is over the 50-line
guideline; split it into smaller tests/helpers (e.g.,
setup_fb_and_persistent_link(), intern_ephemeral_links_loop(), and
verify_peak_bounds_and_lookup()) so each function/test stays within 20–40 lines
(max 50), and add explicit section markers /* --- Arrange --- */, /* --- Act ---
*/, and /* --- Assert --- */ inside each test to clarify phases; keep references
to the same symbols (zr_fb_t fb, zr_fb_init, zr_fb_link_intern, zr_fb_cell,
fb.links_len, fb.link_bytes_len, zr_fb_link_lookup, zr_fb_release) when moving
code into helpers and ensure assertions remain in the Assert sections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b039e883-94d4-4d81-9966-53175e630ec9
📒 Files selected for processing (62)
examples/zr_example_common.hinclude/zr/zr_caps.hinclude/zr/zr_config.hinclude/zr/zr_debug.hinclude/zr/zr_drawlist.hinclude/zr/zr_engine.hinclude/zr/zr_event.hinclude/zr/zr_metrics.hinclude/zr/zr_platform_types.hinclude/zr/zr_result.hinclude/zr/zr_terminal_caps.hinclude/zr/zr_version.hsrc/core/zr_cursor.hsrc/core/zr_damage.csrc/core/zr_damage.hsrc/core/zr_detect.csrc/core/zr_diff.csrc/core/zr_drawlist.csrc/core/zr_engine.csrc/core/zr_engine_present.incsrc/core/zr_event_pack.csrc/core/zr_event_pack.hsrc/core/zr_framebuffer.csrc/core/zr_metrics.csrc/core/zr_placeholder.csrc/platform/win32/zr_win32_conpty_test.csrc/platform/win32/zr_win32_conpty_test.hsrc/unicode/zr_grapheme.hsrc/unicode/zr_unicode_data.hsrc/unicode/zr_unicode_pins.hsrc/unicode/zr_utf8.hsrc/unicode/zr_width.hsrc/unicode/zr_wrap.hsrc/util/zr_arena.hsrc/util/zr_ring.hsrc/util/zr_string_builder.hsrc/util/zr_string_view.hsrc/util/zr_vec.htests/CMakeLists.txttests/golden/zr_golden.ctests/golden/zr_golden.htests/integration/win32_conpty_rawmode.ctests/integration/win32_conpty_wake.ctests/integration/zr_integration_smoke.ctests/unit/test_arena.ctests/unit/test_blit.ctests/unit/test_checked.ctests/unit/test_damage_rects.ctests/unit/test_detect_profile.ctests/unit/test_diff_hotpath_telemetry.ctests/unit/test_drawlist_execute.ctests/unit/test_drawlist_image.ctests/unit/test_drawlist_validate.ctests/unit/test_engine_present_damage_resync_hyperlinks.ctests/unit/test_engine_present_single_flush.ctests/unit/test_engine_submit_drawlist_no_partial_effects.ctests/unit/test_engine_tick_events.ctests/unit/test_framebuffer_init_resize.ctests/unit/test_limits.ctests/unit/test_metrics_struct.ctests/zr_test.ctests/zr_tests_main.c
💤 Files with no reviewable changes (9)
- src/unicode/zr_utf8.h
- src/util/zr_string_view.h
- tests/unit/test_checked.c
- tests/golden/zr_golden.h
- tests/unit/test_damage_rects.c
- src/unicode/zr_unicode_pins.h
- tests/integration/zr_integration_smoke.c
- tests/unit/test_arena.c
- tests/zr_tests_main.c
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f692697da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
RtlZeroMemory
force-pushed
the
fix/177-da1-probe-drain
branch
from
78a2efd to
2b2e62c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_limits.c (1)
216-274: Refactor this test into smaller phases/helpers to meet test-function limits.The new test is over the 50-line cap and is missing explicit test phase markers. Please split the ephemeral-loop logic into a helper and add
/* --- Arrange/Act/Assert --- */sections.♻️ Minimal refactor shape
+static void zr_limits_intern_ephemeral_links(zr_fb_t* fb, zr_cell_t* left, zr_cell_t* right, + uint32_t* peak_links_len, uint32_t* peak_link_bytes_len) { + /* loop body moved from test */ +} + ZR_TEST_UNIT(limits_link_intern_compacts_stale_refs_and_bounds_growth) { + /* --- Arrange --- */ ... - for (uint32_t i = 0u; i < ZR_TEST_EPHEMERAL_LINK_ITERS; i++) { - ... - } + /* --- Act --- */ + zr_limits_intern_ephemeral_links(&fb, left, right, &peak_links_len, &peak_link_bytes_len); + /* --- Assert --- */ ... }As per coding guidelines: "
**/*.c: Functions should be 20-40 lines, with a maximum of 50 lines" and "Add section markers in tests (/* --- Arrange/Act/Assert --- */)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_limits.c` around lines 216 - 274, Split the oversized test ZR_TEST_UNIT(limits_link_intern_compacts_stale_refs_and_bounds_growth) into clear Arrange/Act/Assert sections and move the ephemeral-link loop into a small helper function; specifically, add comments /* --- Arrange --- */ before setup (zr_fb_init, persistent link intern, left/right cells), /* --- Act --- */ to call a new static helper (e.g., run_ephemeral_link_iters or collect_link_peak_stats) that encapsulates the for-loop creating ephemeral URIs and updating right->style.link_ref and returns peak_links_len and peak_link_bytes_len, and /* --- Assert --- */ to perform the final bounds and lookup assertions (ZR_ASSERT_TRUE checks, zr_fb_link_lookup and zr_fb_release). Ensure the helper references the zr_fb_t* fb, left and right cells and returns or outputs the peak values so the test body stays under the line limit and each phase is clearly marked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/test_limits.c`:
- Around line 216-274: Split the oversized test
ZR_TEST_UNIT(limits_link_intern_compacts_stale_refs_and_bounds_growth) into
clear Arrange/Act/Assert sections and move the ephemeral-link loop into a small
helper function; specifically, add comments /* --- Arrange --- */ before setup
(zr_fb_init, persistent link intern, left/right cells), /* --- Act --- */ to
call a new static helper (e.g., run_ephemeral_link_iters or
collect_link_peak_stats) that encapsulates the for-loop creating ephemeral URIs
and updating right->style.link_ref and returns peak_links_len and
peak_link_bytes_len, and /* --- Assert --- */ to perform the final bounds and
lookup assertions (ZR_ASSERT_TRUE checks, zr_fb_link_lookup and zr_fb_release).
Ensure the helper references the zr_fb_t* fb, left and right cells and returns
or outputs the peak values so the test body stays under the line limit and each
phase is clearly marked.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/test_detect_profile.c (1)
257-275: Add explicit AAA section markers and name the expected read-count constant.This regression test is solid, but it currently misses required test section markers and uses a bare literal for a key expectation.
♻️ Suggested cleanup
ZR_TEST_UNIT(detect_profile_da1_reply_limits_final_drain_wait) { + /* --- Arrange --- */ mock_plat_reset(); plat_t* plat = NULL; plat_caps_t baseline; ZR_ASSERT_EQ_U32(zr_test_open_mock_platform(&plat, &baseline), ZR_OK); static const uint8_t kResponses[] = "\x1b[?1;2c"; + enum { ZR_EXPECTED_DA1_DRAIN_READS = 2u }; ZR_ASSERT_EQ_U32(mock_plat_push_input(kResponses, sizeof(kResponses) - 1u), ZR_OK); + /* --- Act --- */ zr_terminal_profile_t profile; plat_caps_t out_caps; ZR_ASSERT_EQ_U32(zr_detect_probe_terminal(plat, &baseline, &profile, &out_caps, NULL, 0u, NULL), ZR_OK); + /* --- Assert --- */ ZR_ASSERT_EQ_U32(profile.da1_responded, 1u); - ZR_ASSERT_EQ_U32(mock_plat_timed_read_call_count(), 2u); + ZR_ASSERT_EQ_U32(mock_plat_timed_read_call_count(), ZR_EXPECTED_DA1_DRAIN_READS); zr_test_close_mock_platform(plat); }As per coding guidelines, "Add section markers in tests (/* --- Arrange/Act/Assert --- /, / --- Validate/Compute/Emit --- */)" and "Extract magic numbers to named constants with descriptive names instead of using bare literals".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_detect_profile.c` around lines 257 - 275, The test function detect_profile_da1_reply_limits_final_drain_wait is missing the AAA section comments and uses a bare literal for the expected timed-read count; add the standard test section markers (/* --- Arrange --- */, /* --- Act --- */, /* --- Assert --- */) to separate setup, stimulus, and verification blocks around the mock_plat_reset()/zr_test_open_mock_platform()/mock_plat_push_input, the zr_detect_probe_terminal call, and the assertions respectively, and replace the magic literal 2u used in ZR_ASSERT_EQ_U32(mock_plat_timed_read_call_count(), 2u) with a named constant (e.g., const uint32_t kExpectedTimedReadCalls = 2u) declared near the Arrange section to make the expectation self-documenting.src/core/zr_detect.c (1)
868-921: Refactor the DA1-enhanced probe loop into sectioned helpers to meet function-size guidance.The DA1 sentinel logic is correct, but this adds more branching to an already large function. Please split this path into helper(s) and add section markers in the remaining loop body for maintainability.
As per coding guidelines, "Functions should be 20-40 lines, with a maximum of 50 lines" and "Add section markers in long functions using comments like /* --- Section Name --- */ to break up logic".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/zr_detect.c` around lines 868 - 921, Refactor the DA1-specific branch of the big probe loop into one or two helpers: extract the timeout/budget clamping and time bookkeeping into a helper named e.g. zr_detect_update_da1_budget(da1_responded, da1_seen_ms, da1_drain_spent_ms, start_ms, timeout_spent_ms, &timeout_ms) that calls zr_detect_read_timeout_slice, computes da1_elapsed_ms, updates da1_drain_spent_ms, calls zr_detect_remaining_da1_drain_budget and zr_detect_clamp_timeout_budget; extract the post-read parsing that sets da1_responded/da1_seen_ms into a helper e.g. zr_detect_check_da1_response(collected, collected_len, &da1_responded, &da1_seen_ms) which invokes zr_detect_parse_responses; then replace the in-loop blocks that manipulate da1_responded/da1_seen_ms/da1_drain_spent_ms and parsing with those helpers and add clear section markers in the remaining loop (e.g. /* --- Read timeout & DA1 budget --- */, /* --- Read/copy data --- */, /* --- Parse responses / DA1 detection --- */) to keep the main function within size guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/zr_detect.c`:
- Around line 868-921: Refactor the DA1-specific branch of the big probe loop
into one or two helpers: extract the timeout/budget clamping and time
bookkeeping into a helper named e.g. zr_detect_update_da1_budget(da1_responded,
da1_seen_ms, da1_drain_spent_ms, start_ms, timeout_spent_ms, &timeout_ms) that
calls zr_detect_read_timeout_slice, computes da1_elapsed_ms, updates
da1_drain_spent_ms, calls zr_detect_remaining_da1_drain_budget and
zr_detect_clamp_timeout_budget; extract the post-read parsing that sets
da1_responded/da1_seen_ms into a helper e.g.
zr_detect_check_da1_response(collected, collected_len, &da1_responded,
&da1_seen_ms) which invokes zr_detect_parse_responses; then replace the in-loop
blocks that manipulate da1_responded/da1_seen_ms/da1_drain_spent_ms and parsing
with those helpers and add clear section markers in the remaining loop (e.g. /*
--- Read timeout & DA1 budget --- */, /* --- Read/copy data --- */, /* --- Parse
responses / DA1 detection --- */) to keep the main function within size
guidance.
In `@tests/unit/test_detect_profile.c`:
- Around line 257-275: The test function
detect_profile_da1_reply_limits_final_drain_wait is missing the AAA section
comments and uses a bare literal for the expected timed-read count; add the
standard test section markers (/* --- Arrange --- */, /* --- Act --- */, /* ---
Assert --- */) to separate setup, stimulus, and verification blocks around the
mock_plat_reset()/zr_test_open_mock_platform()/mock_plat_push_input, the
zr_detect_probe_terminal call, and the assertions respectively, and replace the
magic literal 2u used in ZR_ASSERT_EQ_U32(mock_plat_timed_read_call_count(), 2u)
with a named constant (e.g., const uint32_t kExpectedTimedReadCalls = 2u)
declared near the Arrange section to make the expectation self-documenting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bef252cf-38bb-441a-8505-f9ef848c7d96
📒 Files selected for processing (3)
src/core/zr_detect.ctests/unit/test_detect_profile.ctests/unit/test_limits.c
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_limits.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix startup probe latency after DA1 by bounding post-DA1 drain on elapsed wall-clock time.
This PR is intentionally minimal and contains only the DA1 probe fix plus one test build fix.
Context
RtlZeroMemory/Rezi#177Changes
fix(detect): stop probing shortly after DA1 replyfix(detect): bound DA1 drain by elapsed timefix(test): include zr_macros for ZR_MAX in limits testtests/unit/test_limits.cso CI/build compiles consistently.Validation
cmake --preset posix-clang-debugcmake --build --preset posix-clang-debug -j2Summary by CodeRabbit
Improvements
Tests
Chores