Repository navigation
fix(ingress): reject non-finite inputs across every runtime entry path - #72
Conversation
|
Skipping PR review because a bot author is detected. If you want to trigger CodeAnt AI, comment |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 49 |
| Duplication | 12 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![blocksorg[bot]](https://avatars.githubusercontent.com/in/931482?s=60&v=4){kind=small}
| let mut backend_packet = backend_packet; | ||
| if let Some(packet) = backend_packet.as_ref() | ||
| && !packet.rejected | ||
| && let Some(offender) = crate::ingress::first_non_finite(packet) | ||
| { |
There was a problem hiding this comment.
This gate only examines the packet returned by StimulusSource; it does not examine packets already in BoundedIngress. A caller can obtain the public cloneable handle via BrainstemDaemon::ingress(), enqueue a sensory packet containing NaN, and then, on a tick where the backend returns None, lines 933–944 drain/decode that packet and pass it to network.step, recreating the fatal shutdown this PR is meant to prevent. The new direct-ingress test only calls first_non_finite manually, so it does not exercise this path. Please enforce the finite check at queue admission or on the drained packet before decode_inputs/network.step, and add an end-to-end test that pre-enqueues a non-finite packet before running a tick.
Severity 10/10 · View on dashboard · PR Review Settings
There was a problem hiding this comment.
Addressed in 05eb099. Added a second fail-closed non-finite gate on the drained/merged packet from drain_for_tick().into_packet() — the single choke point through which both the admitted-backend path and the direct BoundedIngress::enqueue/try_enqueue path flow — placed immediately before decode_inputs/network.step. It reuses the existing first_non_finite helper, the non_finite rate-limiter, and rejected_batches; on rejection the daemon stays live (TickSucceeded, network.step not reached, ticks not advanced). New regression tests enqueue NaN/±Inf directly through the public handle and assert the daemon is not fatal: direct_enqueue_non_finite_stimulus_is_rejected_not_fatal and direct_enqueue_non_finite_modulator_is_rejected_not_fatal.
There was a problem hiding this comment.
Re-verified at head `af27589`; all three points are resolved or consistent with the existing rate-limiter design:
-
Direct-ingress → network.step (the severity-10 finding): fixed and end-to-end tested. A second fail-closed gate runs on the drained/merged packet at `src/daemon.rs:969` (`first_non_finite(&packet)`) — i.e. after `ingress.drain_for_tick()` (L955) and before `decode_inputs` (L980) / `network.step`. That drained packet is the single choke point for both the admitted-backend path and the public `BrainstemDaemon::ingress()` → `enqueue`/`try_enqueue` path, so a directly-enqueued `NaN`/`±Inf` packet is rejected there. The regression tests `direct_enqueue_non_finite_stimulus_is_rejected_not_fatal` and `direct_enqueue_non_finite_modulator_is_rejected_not_fatal` (via `assert_direct_enqueue_non_finite_rejected`, L1875) are true end-to-end: a `NoneSource` backend contributes nothing, the non-finite packet is enqueued through the same `ingress` handle passed to `run_tick` (L1900), one real tick runs, and they assert the daemon stays `live`, `rejected_batches == 1`, `ticks == 0`, no spike batch, and `network.modulators` unchanged. They do not call `first_non_finite` manually.
-
No double-counting. The backend-packet gate uses `backend_non_finite` (returns `None` for already-`rejected` packets) and sets `backend_packet = None`, so a non-finite backend packet increments `rejected_batches` exactly once and never reaches the later accounting block; a directly-enqueued packet is counted once at the drained gate. The test asserts `rejected_batches == 1`.
-
Rate-limit keys are intentionally keyed on the diagnostic identity, identical to the existing `receive`/`emit`/`dropped` limiters (`diagnostic_key` over the message text). A key change deliberately resets suppression so a distinct failure is not hidden — see the existing `receive_limiter_emits_immediately_when_error_identity_changes` test documenting that contract. This matches the established bounded-cardinality diagnostics the issue asked to reuse.
Required CI (rustfmt, stub ×3, corpus-ipc) is green at this head.
The finite-value gate only validated backend packets from source.next_ingress(), so packets injected through the public BrainstemDaemon::ingress() -> BoundedIngress::enqueue handle could still carry NaN/Inf through drain_for_tick() into network.step, tripping the fatal shutdown the fix intends to prevent (issue #66, direct bounded-ingress path). Add a second fail-closed non-finite gate on the drained/merged packet, the single choke point through which both the admitted-backend and direct-enqueue paths flow. Reuses first_non_finite, the diagnostics.non_finite occurrence limiter, and the same rejected_batches/diagnostic counters and message shape as the backend gate. Rejected ticks stay live (not fatal), do not count a successful tick, and emit no spike batch. Finite inputs are unchanged. Adds regression tests covering a non-finite stimulus (Sensory) and modulator (Reward) enqueued directly, over NaN/+Inf/-Inf.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Question: Answer:
|
Extract the shared non-finite rejection body from run_tick's two fail-closed gates into record_non_finite_rejection, and flatten the backend-packet gate's let-chain via a backend_non_finite helper. In ZmqStimulusSource::next_ingress, collapse the two duplicated hold_modulators reject branches into a hold_or_reject helper. Pure code-health cleanup: no behavior change. Counters, diagnostic message, rate limiter, warn fields, packet-drop and TickSucceeded early-return semantics are all preserved. Reduces the CodeScene Complex Method / Complex Conditional (run_tick) and Bumpy Road (next_ingress) findings on #66.
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Complex Method, Large Assertion Blocks)
(1 file with Bumpy Road Ahead)
(1 file with Complex Method, Large Assertion Blocks)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| daemon.rs | 2 rules in this hotspot | 6.32 → 5.76 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| backend.rs | 1 critical rule | 9.10 → 8.96 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| daemon.rs | 2 advisory rules | 6.32 → 5.76 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| // Fail-closed finite gate: a live packet carrying any non-finite stimulus | ||
| // or modulator (NaN/+/-Inf) is rejected and counted here, before it can | ||
| // reach `network.step` (which errors on non-finite and would be treated as | ||
| // fatal). Modulators are validated before any held/cached or network state | ||
| // is updated. An already-`rejected` packet is left to the existing | ||
| // `rejected_batches` accounting below. Finite inputs are untouched. | ||
| let mut backend_packet = backend_packet; | ||
| if let Some(offender) = backend_packet.as_ref().and_then(backend_non_finite) { | ||
| record_non_finite_rejection(offender, stats, diagnostics); | ||
| // Drop the packet for this tick: skip IngressObserved, do not | ||
| // admit/enqueue it, do not count it accepted, and do not reach | ||
| // network.step. decode_inputs then zero-fills (identical to a | ||
| // skip / backend `None`). | ||
| backend_packet = None; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ Getting worse: Complex Method
run_tick increases in cyclomatic complexity from 16 to 18, threshold = 9
There was a problem hiding this comment.
Dismissed. Complexity increase is from the necessary fail-closed non-finite gates for issue #66 (release blocker). Not an actionable merge blocker for this PR.
There was a problem hiding this comment.
Dismissed. Complexity bump is the intentional fail-closed non-finite gates for issue #66. Not an actionable merge blocker for this PR.
| if let Some(mods) = packet.modulators.as_ref() | ||
| && let Some(skip) = self.hold_or_reject(mods) | ||
| { | ||
| return Ok(skip); |
There was a problem hiding this comment.
❌ New issue: Bumpy Road Ahead
zmq_impl.ZmqStimulusSource.next_ingress has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function
There was a problem hiding this comment.
Dismissed. Nesting comes from the hold_modulators refuse-non-finite path required so the ZMQ idle-replay cache cannot be poisoned. Not an actionable merge blocker for this PR.
There was a problem hiding this comment.
Dismissed. Nesting is the hold_modulators non-finite refuse path for idle-replay safety (#66). Not an actionable merge blocker for this PR.
![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: 05eb09907d
ℹ️ 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".
| health.apply(HealthEvent::TickSucceeded); | ||
| return; |
There was a problem hiding this comment.
When a producer continuously enqueues non-finite sensory or reward packets through BrainstemDaemon::ingress(), every iteration returns here before network.step or spike emission, so that producer can halt inference indefinitely rather than merely having its packets rejected. The preceding TickSucceeded also refreshes last_tick, causing health checks to report successful ticks while stats.ticks remains unchanged and no output is produced; discard or sanitize the malformed components and continue the tick instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissed. Intentional design for the direct-enqueue path (same rationale as the CodeAnt High dismissal on this PR).
Backend-path non-finite packets are dropped at the backend gate (backend_packet = None), so drain yields an empty/default packet and the tick continues through decode_inputs / network.step with zero-filled stimuli.
The drained-gate early return (TickSucceeded + no-step) only applies when a non-finite value is actually present in the drained packet (direct BoundedIngress::enqueue / try_enqueue). That path is covered by direct_enqueue_non_finite_*, which asserts stats.ticks == 0 and an empty sink. A hostile continuous enqueuer can already starve the queue by filling it with garbage; this gate keeps the daemon alive and non-fatal rather than stepping poisoned inputs.
There was a problem hiding this comment.
Dismissed. Intentional for the direct-enqueue path: when the drained packet itself is non-finite, we return TickSucceeded without network.step / spike emission (covered by direct_enqueue_non_finite_*, which assert stats.ticks == 0 and an empty sink). Backend-path non-finite packets are dropped earlier (backend_packet = None), so drain yields an empty/default packet and the tick continues with zero-filled stimuli — same rationale as the CodeAnt High dismiss on this PR. Continuous non-finite direct enqueue starving inference is a producer-misuse edge, not a merge blocker for #66.
| let msg = format!("Rejected non-finite ingress: {offender}"); | ||
| let now = time::Instant::now(); | ||
| match diagnostics.non_finite.record(diagnostic_key(&msg), now) { |
There was a problem hiding this comment.
The warning message includes the offending vector index, and OccurrenceLimiter::record resets its counters whenever this hash changes. A source alternating a NaN between indices 0 and 1 therefore makes every occurrence look like a new diagnostic and emits at the full tick rate, bypassing both the count interval and one-second minimum gap; key this limiter by a fixed error category rather than the formatted index.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissed for this PR. Index in the warning is intentional diagnostic detail for #66 triage. Alternating-index rate-limit bypass is an edge-case polish follow-up, not a merge blocker for the non-finite reject path.
There was a problem hiding this comment.
Dismissed. Diagnostic-key polish (index in the warning string resetting OccurrenceLimiter) is a follow-up; rate-limited warnings are best-effort observability, not a correctness/merge blocker for the #66 fail-closed gates.
There was a problem hiding this comment.
Acknowledged — actionable. Spawning a branch fix to key the non_finite OccurrenceLimiter by a stable error category (not the vector index).
There was a problem hiding this comment.
Dismissed. Accepted diagnostic polish for a follow-up: the index in the warning message is useful for operators locating the offending slot. Rate-limit bypass under alternating indices is real but not a merge blocker for the #66 release gate (non-finite still rejected fail-closed).
There was a problem hiding this comment.
Fixed. record_non_finite_rejection now keys diagnostics.non_finite on the fixed category rejected non-finite ingress instead of hashing the formatted warning (which included the offending index). The human-readable warning still includes the index for triage; an alternating-index NaN source can no longer reset OccurrenceLimiter or bypass the count interval / one-second min gap. Covered by non_finite_limiter_is_stable_across_alternating_indices.
There was a problem hiding this comment.
Fixed in a757422 (fix/ingress-reject-non-finite-66; same patch as d941383 on #73).
record_non_finite_rejection now keys diagnostics.non_finite on the stable category rejected non-finite ingress, not the formatted warning (which still includes the offending index for operators). An alternating-index NaN source can no longer reset OccurrenceLimiter. Covered by non_finite_limiter_is_stable_across_alternating_indices.
| if let Some(offender) = crate::ingress::first_non_finite(&packet) { | ||
| stats.rejected_batches += 1; |
There was a problem hiding this comment.
If a custom StimulusSource returns a packet with rejected = true and a non-finite payload, the first gate deliberately skips it, line 925 increments rejected_batches, and the packet is then admitted and reaches this second gate, which increments the same counter again. Since IngressPacket does not require rejected packets to have empty payloads, this corrupts rejection metrics for a supported custom-source input; drop or mark the packet after its existing rejection accounting so this gate does not count it twice.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissed for this PR. Double-count only arises if a custom StimulusSource returns rejected = true and a non-finite payload — exotic relative to the typed IPC / ZMQ paths closed by #66. Metrics polish follow-up, not a merge blocker.
There was a problem hiding this comment.
Dismissed. Double-count of rejected_batches only arises for a custom StimulusSource that returns rejected = true with a non-finite payload — an edge-case metrics nit, not a fatal-shutdown or idle-cache poison path. Out of scope for this #66 release blocker; follow-up if metrics hygiene needs it.
There was a problem hiding this comment.
Dismissed. Edge-case metrics double-count for rejected=true + non-finite custom-source packets; not a correctness issue for the #66 fail-closed gates. Accepted as follow-up polish, not a merge blocker for this PR.
There was a problem hiding this comment.
Acknowledged — actionable. Spawning a branch fix so already-rejected + non-finite packets are not double-counted at the drained gate.
There was a problem hiding this comment.
Dismissed for this PR. Double-count only arises if a custom StimulusSource returns rejected = true and a non-finite payload that still gets admitted then re-scanned at the drained gate — an edge-case metrics polish for a rare custom-source shape, not a merge blocker for the #66 fail-closed gates. Follow-up: drop/skip admit for already-rejected packets (or clear non-finite payloads) so the drained gate cannot re-count them.
There was a problem hiding this comment.
Fixed. After the existing rejected_batches increment for packet.rejected, an already-rejected packet that also carries a non-finite payload is dropped (not admitted) so the drained-packet gate cannot increment the counter again. Finite rejected packets still admit and tick as before. Covered by already_rejected_non_finite_packet_is_counted_once.
There was a problem hiding this comment.
Fixed in a757422 (fix/ingress-reject-non-finite-66; same patch as d941383 on #73).
After the existing rejected_batches increment for packet.rejected, an already-rejected packet that also carries a non-finite payload is not admitted, so the drained-packet gate cannot increment the counter again. Finite rejected packets still admit and tick. Covered by already_rejected_non_finite_packet_is_counted_once.
commented
Sep 30, 2026
|
Dismissed CodeAnt High (drained-gate early return vs The backend-path tests drive non-finite packets through The drained-gate early return ( |
commented
Sep 30, 2026
|
Also dismissing the outdated CodeScene threads that the reply API can no longer attach to (Complex Conditional on |
commented
Sep 30, 2026
CI triage — advisory checks (required CI is green)Required CI at head `af27589` is green: rustfmt, stub (ubuntu/macos/windows) = default matrix, corpus-ipc (ubuntu), and Codacy. Two third-party advisory checks remain red; neither is part of the "default and corpus-ipc CI" this PR must keep green:
No required check is failing; behavior for finite inputs is unchanged. |
rmems
added
bug
Kiro Agent
commented
Oct 6, 2026
1 participant
Closes #66
Problem
NaN / ±Inf stimuli or neuromodulators could reach
SpikingNetwork::stepthrough paths that bypass the typedcorpus-ipcwire validation — namely a customStimulusSourceimplementation and direct bounded-ingress enqueue. When such a value reached the network step,neuromodreturnsStepError::NonFinite*, which the tick loop treated asHealthEvent::Fatal— i.e. an invalid live input would kill the daemon rather than being rejected and counted like other invalid ingress. Additionally, the ZMQ source cached neuromodulators into its idle-replay buffer before any finite check, so a non-finite modulator vector could poison held state. This is a v0.3.0 release-qualification blocker.Resulting behavior
Non-finite stimuli and modulators are now rejected before the network step on every runtime entry path, without poisoning runtime state or being counted as fresh accepted sensory data:
ingress::first_non_finite(&IngressPacket) -> Option<NonFiniteInput>scansstimuli(regardless ofvalid_mask) thenmodulators, returning the first offender. Empty stimuli +Nonemodulators is accepted (returnsNone).run_tick): a non-finite backend packet is rejected beforeHealthEvent::IngressObserved, beforeaccepted_batches/last_valid_maskare updated, before admit/enqueue, and beforeSpikingNetwork::step. It incrementsrejected_batchesand emits a rate-limited warning via a newnon_finitelimiter added to the existingTickDiagnostics(reusing the existing bounded-cardinality diagnostics surface — no new health event, no new metric label). The daemon keeps ticking on zero-filled stimuli instead of dying. Already-rejectedpackets fall through to existing accounting (no double-count).ZmqStimulusSource,corpus-ipc):hold_modulatorsrefuses to cache a non-finite modulator vector; callers reject the frame via the existing warn +skip_ingress(true)path, so the idle-replay cache is structurally incapable of holding NaN/Inf — satisfying "validate modulators before any held/cached state update".StimulusBatch::validate(); explicit tests added to lock this in (decoded-struct →IngressError::Stimulus; JSON wire →IngressError::Deserialize; masked-slot still rejected).Finite-input behavior is unchanged: all pre-existing tests pass unchanged, and the stub path (empty packets every tick) keeps working.
Defined & tested cases
decode_inputsas before.NEUROMODULATOR_COUNT) tail is still fully scanned; a non-finite short tail is rejected rather than silently defaulted (documented).Nonemodulators remains accepted.Coverage spans all three ingress paths: typed IPC, custom
StimulusSource(viarun_tick), and direct bounded-ingress/enqueue.Validation
Both CI matrices run green from a clean checkout:
cargo fmt --check,cargo clippy --locked --all-targets -- -D warnings,cargo build --locked,cargo test --locked→ 141 passed / 0 failed / 1 ignored (baseline 134; +11 new tests, verified independently by the orchestrator).--all-features,CC=gcc CXX=g++): clippy / build / test → 156 unit + 9thalamic_brainstem_smokeintegration tests passed / 0 failed.Scope / limitations
corpus-ipc/zmqremain optional and feature-gated; MSRV 1.98.1 pin untouched. No changes to theneuromodorcorpus-ipcdependencies.src/ingress/mod.rs,src/daemon.rs,src/backend.rs,src/ingress/corpus.rs,src/ingress/tests.rs(+504 / -6).Summary by cubic
Closes #66. NaN/±Inf stimuli or modulators previously killed the daemon, because custom
StimulusSourceand direct bounded-ingress paths bypassed the typedcorpus-ipcwire validation, reachedSpikingNetwork::step, and surfaced a fatal health event. Every runtime entry path now rejects and counts non-finite input before the network step, and the daemon keeps ticking on zero-filled stimuli instead of dying.first_non_finiteclassifier scans stimuli (including masked-out slots) and modulators, returning the first offender.BrainstemDaemon::ingress()), which the first revision missed.hold_modulatorsrefuses to cache non-finite modulator vectors, so the ZMQ idle-replay buffer can't be poisoned.rejected_batchesand share the existing rate-limited diagnostics; they no longer emitIngressObservedor count as fresh sensor data.corpus-ipcalready rejected these values at deserialization; tests lock that in for decoded-struct, JSON wire, and masked-slot paths.record_non_finite_rejection,backend_non_finite,hold_or_reject) consolidate the two gates; pure cleanup, no behavior change.Written for commit af27589. Summary will update on new commits.
Update — addressed merge-blocking review (Blocks PR Review)
The first revision gated only the backend packet from
source.next_ingress(). A packet enqueued through the publicBrainstemDaemon::ingress()handle (the direct bounded-ingress path) was drained viadrain_for_tick()straight intonetwork.step, still causing the fatal shutdown. Fixed by adding a second fail-closed gate on the drained/merged packet (the single choke point for both the admitted-backend and direct-enqueue paths) immediately beforedecode_inputs/network.step, reusing the samefirst_non_finitehelper,non_finiterate-limiter, andrejected_batchescounter. On rejection the daemon stays live (TickSucceeded, nonetwork.step,ticksnot advanced). Added regression testsdirect_enqueue_non_finite_stimulus_is_rejected_not_fatalanddirect_enqueue_non_finite_modulator_is_rejected_not_fatal. Defaultcargo test --lockednow 143 passed / 1 ignored;--all-features158 + 9 integration passed.