Code Review: PR #26 — Wave B (Per-Action Throttling)

[mrtwebdesign]

Code Review for PR #26 (Wave B)

Reviewer: Claude Code (2026-05-07)
Scope: hypercart-query-guard.php (throttling additions), tests/ActionThrottleTest.php, tests/bootstrap.php (Wave B stubs)

Fix Now

• H3. Unsanitized $_SERVER['REQUEST_URI'] in log payloads — hypercart-query-guard.php:391,747,944 — Raw user input flows into log payloads. wp_json_encode escapes for JSON, but if logs render in an HTML admin UI this is an XSS vector. Fix: apply esc_url_raw() or sanitize_text_field() before storing. (Fixed in e4dc882)
• M1. CRC32 collision risk in defer_count_key() — hypercart-query-guard.php:706 — CRC32 is 32-bit; birthday paradox gives meaningful collisions with thousands of action signatures. Two different (hook, args, group) tuples sharing a counter causes premature "max_defers_reached." Fix: swap hash('crc32', ...) to md5(...). (Fixed in e4dc882)

Fix Before Production

• H2. defer_action() cancel/unclaim ordering inverted — hypercart-query-guard.php:641-642 — cancel_action() then unclaim_action() risks an orphaned claim. If AS's cancel_action() already clears the claim (version-dependent), the unclaim call is redundant. Verify AS behavior; either reverse the order or remove the redundant call. (Fixed in 146e0ed — removed redundant unclaim_action)
• M3. get_throttle_policy() doesn't validate filter output structure — hypercart-query-guard.php:474-479 — Only checks is_array(). A malformed filter return like ['elevated' => 'oops'] causes a type error downstream when apply_throttle_value() casts. Fix: apply the same normalization pattern used in get_action_delay_matrix(). (Fixed in 146e0ed)
• M2. Non-atomic increment_defer_count() — hypercart-query-guard.php:714-718 — Read-modify-write race; two concurrent AS runners can lose an increment. Code acknowledges "best-effort." Fix: use wp_cache_incr() + wp_cache_add() fallback for atomicity on persistent caches. (Fixed in 146e0ed)

Fix Later (Test Infrastructure)

• H6. Zero test coverage for integration-level deferral — get_action_throttle_decision(), maybe_defer_action_before_execute(), defer_action(), build_deferred_action_clone() are all untested. Requires stubbing ActionScheduler_Store, ActionScheduler_Action, and ActionScheduler_SimpleSchedule.
• M9. Memoized $throttle_runtime not cleared between tests — hypercart-query-guard.php:160 — Static array not reset in setUp(). Not an active bug yet (no tests exercise these functions), but will cause test state leakage when integration tests are added.

Noted (No Action Required)

• L1. $_REQUEST['action'] used without nonce verification at line 792 (context detection only). Add a phpcs:ignore comment.
• L2. as_get_datetime_object() called without function_exists() guard at line 675. Upstream try/catch catches the fatal, but the pattern is fragile.
• L12. Missing test edge cases: empty/edge-case inputs for defer_count_key, observe-mode deduplication, max-defer safety valve.

Cross-Wave Note

No issues from PR #25 (Wave A) were resolved by this PR. The only cross-wave change was removing a "this is inert until Wave B" comment from the priority registry docblock. All waves are cleanly additive.