Kestrel per-antenna stats: CRC-gate window aggregates + pure physts output param (#355 review)

[josephnef]

Addresses the two code-review findings on #355 (both confirmed real):

1. CRC gating — the Kestrel RX loop fed _rxq/_rxpaths for every WIFI frame, while Jaguar1/2/3 fold window aggregates only for CRC-clean frames. A garbled frame's cached physts biases the per-chain means and the active_mask classification under FCS-error conditions. Both feeds now gate on !crc_err, matching the other generations.

2. Pure output param — parse_physts_8852 updated header RSSI but left snr_avg/snr[]/evm[] untouched on an is_valid=0 stub or an absent IE, so a caller reusing the struct could observe a previous blob's values. The parser now clears out on entry; result depends only on the current buffer. (The in-tree caller passes a fresh struct, so no observed misbehavior — this hardens the API contract.) Selftested with a deliberately dirtied struct.

Validation: 48/48 ctest; on-air re-run of tests/rxpath_perantenna_onair.sh (8852C RX ← 8822BU TX, ch36, ~4.2k frames) PASS with the gate in place.

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Kestrel per-antenna stats: CRC-gated window aggregates + pure physts output

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Gate Kestrel RX window aggregates on CRC-clean frames to match Jaguar behavior.
• Make parse_physts_8852 clear its output struct for deterministic results.
• Extend selftest to catch stale-field leakage in reused KestrelPhySts structs.

Diagram

graph TD
  A["RtlKestrelDevice RX loop"] --> B{"CRC clean?"} -->|"yes"| C["Update _rxq"] --> D["Update _rxpaths"]
  B -->|"no"| E["Skip window aggregates"]
  A --> F["parse_physts_8852 (clears out)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Gate earlier (avoid using cached physts on CRC-fail)

• ➕ Reduces risk of any downstream consumer accidentally using stale/corrupted physts on CRC errors
• ➕ Centralizes CRC policy at the earliest point in frame handling
• ➖ May change existing behavior relied on for debugging/telemetry on bad frames
• ➖ Requires auditing all consumers to ensure they still get desired metadata

2. Return an optional/variant from parse_physts_8852 instead of mutating out

• ➕ Makes validity explicit in the type system and prevents stale-field reads by construction
• ➕ Encourages callers to handle invalid/missing IE cases intentionally
• ➖ More invasive API change and churn across call sites
• ➖ Not aligned with current style of parsers using output parameters

Recommendation: Keep the PR’s approach: CRC-gating only the window aggregates is the minimal behavior fix that matches the established Jaguar convention without disrupting other frame processing. Clearing the physts output struct on entry is a low-cost API hardening that prevents latent misuse; the added selftest specifically guards the regression scenario.

Files changed (3) +26 / -12

Bug fix (2) +19 / -10

FrameParserKestrel.hClear KestrelPhySts output on parse to enforce purity +4/-0

Clear KestrelPhySts output on parse to enforce purity

• parse_physts_8852 now zero-initializes the output KestrelPhySts at function entry. This prevents stale snr/evm fields from leaking through when is_valid=0 stubs are parsed or when IEs are absent.

src/kestrel/FrameParserKestrel.h

RtlKestrelDevice.cppGate _rxq/_rxpaths window updates on CRC-clean frames +15/-10

Gate _rxq/_rxpaths window updates on CRC-clean frames

• The RX loop now feeds windowed RX-quality (_rxq) and per-antenna means (_rxpaths) only when the received frame is not CRC-errored. This avoids biased rolling means and incorrect active-chain classification caused by cached physts on garbled frames.

src/kestrel/RtlKestrelDevice.cpp

Tests (1) +7 / -2

kestrel_rxparse_selftest.cppAdd regression coverage for stale-field leakage on invalid physts +7/-2

Add regression coverage for stale-field leakage on invalid physts

• The selftest now dirties a reused KestrelPhySts struct before parsing an is_valid=0 buffer and asserts that fields like snr/evm are cleared to 0. This validates the new deterministic output-parameter contract.

tests/kestrel_rxparse_selftest.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Out uncleared on parse failure 🐞 Bug ≡ Correctness

Description

parse_physts_8852() now documents that out is a “pure output param” and clears it, but the `(p
== nullptr || len < 8) early return happens before the clear, so a reused KestrelPhySts` can still
retain stale values when the function returns false. This contradicts the new contract and can
leak prior SNR/EVM/RSSI values to callers that inspect out after a failed parse.

Code

src/kestrel/FrameParserKestrel.h[R151-154]

+  /* Pure output param: the result depends only on this buffer, never on what
+   * a reused struct held before (an is_valid=0 stub or an absent IE must
+   * read 0, not a previous blob's value). */
+  out = KestrelPhySts{};

Evidence

The function returns false on p == nullptr || len < 8 before executing the newly added `out =
KestrelPhySts{};` clear, even though the added comment states the output should not depend on prior
struct contents.

src/kestrel/FrameParserKestrel.h[147-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`parse_physts_8852()` now claims to be a “pure output param” API by clearing `out`, but it can still return `false` before clearing when `p == nullptr` or `len < 8`, leaving stale values in a reused struct.
## Issue Context
The function currently clears `out` only after validating `p` and `len`. This means the function’s stated “result depends only on this buffer” guarantee is not true for failure returns.
## Fix Focus Areas
- src/kestrel/FrameParserKestrel.h[147-166]
## Suggested fix
Move `out = KestrelPhySts{};` to the top of the function (before the `p/len` guard), or alternatively narrow the comment/contract to apply only on successful (`true`) returns and add an explicit note that `out` is unspecified on `false`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit 6f9c879 ⚖️ Balanced

Results up to commit 2d6199b

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Out uncleared on parse failure 🐞 Bug ≡ Correctness

Description

parse_physts_8852() now documents that out is a “pure output param” and clears it, but the `(p
== nullptr || len < 8) early return happens before the clear, so a reused KestrelPhySts` can still
retain stale values when the function returns false. This contradicts the new contract and can
leak prior SNR/EVM/RSSI values to callers that inspect out after a failed parse.

Code

src/kestrel/FrameParserKestrel.h[R151-154]

+  /* Pure output param: the result depends only on this buffer, never on what
+   * a reused struct held before (an is_valid=0 stub or an absent IE must
+   * read 0, not a previous blob's value). */
+  out = KestrelPhySts{};

Evidence

The function returns false on p == nullptr || len < 8 before executing the newly added `out =
KestrelPhySts{};` clear, even though the added comment states the output should not depend on prior
struct contents.

src/kestrel/FrameParserKestrel.h[147-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`parse_physts_8852()` now claims to be a “pure output param” API by clearing `out`, but it can still return `false` before clearing when `p == nullptr` or `len < 8`, leaving stale values in a reused struct.
## Issue Context
The function currently clears `out` only after validating `p` and `len`. This means the function’s stated “result depends only on this buffer” guarantee is not true for failure returns.
## Fix Focus Areas
- src/kestrel/FrameParserKestrel.h[147-166]
## Suggested fix
Move `out = KestrelPhySts{};` to the top of the function (before the `p/len` guard), or alternatively narrow the comment/contract to apply only on successful (`true`) returns and add an explicit note that `out` is unspecified on `false`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 4ad46a2

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Out uncleared on parse failure 🐞 Bug ≡ Correctness

Description

parse_physts_8852() now documents that out is a “pure output param” and clears it, but the `(p
== nullptr || len < 8) early return happens before the clear, so a reused KestrelPhySts` can still
retain stale values when the function returns false. This contradicts the new contract and can
leak prior SNR/EVM/RSSI values to callers that inspect out after a failed parse.

Code

src/kestrel/FrameParserKestrel.h[R151-154]

+  /* Pure output param: the result depends only on this buffer, never on what
+   * a reused struct held before (an is_valid=0 stub or an absent IE must
+   * read 0, not a previous blob's value). */
+  out = KestrelPhySts{};

Evidence

The function returns false on p == nullptr || len < 8 before executing the newly added `out =
KestrelPhySts{};` clear, even though the added comment states the output should not depend on prior
struct contents.

src/kestrel/FrameParserKestrel.h[147-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`parse_physts_8852()` now claims to be a “pure output param” API by clearing `out`, but it can still return `false` before clearing when `p == nullptr` or `len < 8`, leaving stale values in a reused struct.
## Issue Context
The function currently clears `out` only after validating `p` and `len`. This means the function’s stated “result depends only on this buffer” guarantee is not true for failure returns.
## Fix Focus Areas
- src/kestrel/FrameParserKestrel.h[147-166]
## Suggested fix
Move `out = KestrelPhySts{};` to the top of the function (before the `p/len` guard), or alternatively narrow the comment/contract to apply only on successful (`true`) returns and add an explicit note that `out` is unspecified on `false`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 6e1095a

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Out uncleared on parse failure 🐞 Bug ≡ Correctness

Description

parse_physts_8852() now documents that out is a “pure output param” and clears it, but the `(p
== nullptr || len < 8) early return happens before the clear, so a reused KestrelPhySts` can still
retain stale values when the function returns false. This contradicts the new contract and can
leak prior SNR/EVM/RSSI values to callers that inspect out after a failed parse.

Code

src/kestrel/FrameParserKestrel.h[R151-154]

+  /* Pure output param: the result depends only on this buffer, never on what
+   * a reused struct held before (an is_valid=0 stub or an absent IE must
+   * read 0, not a previous blob's value). */
+  out = KestrelPhySts{};

Evidence

The function returns false on p == nullptr || len < 8 before executing the newly added `out =
KestrelPhySts{};` clear, even though the added comment states the output should not depend on prior
struct contents.

src/kestrel/FrameParserKestrel.h[147-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`parse_physts_8852()` now claims to be a “pure output param” API by clearing `out`, but it can still return `false` before clearing when `p == nullptr` or `len < 8`, leaving stale values in a reused struct.

## Issue Context
The function currently clears `out` only after validating `p` and `len`. This means the function’s stated “result depends only on this buffer” guarantee is not true for failure returns.

## Fix Focus Areas
- src/kestrel/FrameParserKestrel.h[147-166]

## Suggested fix
Move `out = KestrelPhySts{};` to the top of the function (before the `p/len` guard), or alternatively narrow the comment/contract to apply only on successful (`true`) returns and add an explicit note that `out` is unspecified on `false`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[josephnef]

/review

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 4ad46a2

[josephnef]

/review

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 2d6199b

[josephnef]

/review

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 6f9c879