Revise PR #246: nan and inf walk straight through the new since guard by jaylfc · Pull Request #269 · jaylfc/taosmd

jaylfc · 2026-08-14T06:23:38Z

CARD TITLE (intent, not commit subject): Revise PR #246: nan and inf walk straight through the new since guard

Autonomous build of board card tsk-dx6fok.

Files:
taosmd/http_server.py | 14 ++++++++--
tests/test_a2a.py | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 2 deletions(-)

Summary by Gitar

API Validation Updates:
- Reject since timestamp values below 1_000_000_000 with a 400 error in /a2a/messages and /a2a/stream endpoints
Testing:
- Added comprehensive unit tests in tests/test_a2a.py for rejected and accepted since parameter values

_{This will update automatically on new commits.}

qodo-code-review · 2026-08-14T06:23:48Z

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

coderabbitai · 2026-08-14T06:23:50Z

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 56 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0cdd248b-0a75-4483-ac01-57dfc41e8d4f

📥 Commits

Reviewing files that changed from the base of the PR and between 6de8798 and 6d5c37b.

📒 Files selected for processing (2)

taosmd/http_server.py
tests/test_a2a.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

gitar-bot · 2026-08-14T06:24:00Z

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

kilo-code-bot · 2026-08-14T06:30:00Z

                since = float(since_raw) if since_raw is not None else None
            except (TypeError, ValueError) as exc:
                raise _BadRequest("'since' must be a float timestamp") from exc
+            if since is not None and since < 1_000_000_000:


CRITICAL: nan and inf bypass this guard — PR title claims to fix them but the check doesn't catch them

float('nan') < 1_000_000_000 is False in Python (NaN comparisons always return False), and float('inf') < 1_000_000_000 is also False. So since=nan and since=inf both pass through to archive.query(), where they are truthy and get injected into the SQL as timestamp >= nan/inf. The client receives a 200 with an empty (or wrong) result set instead of the expected 400.

Since math is already imported (line 167), use math.isnan(since) or math.isinf(since) to catch these before the range check.

This affects both this handler and the SSE handler at line 1566.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

kilo-code-bot · 2026-08-14T06:30:00Z

                last_ts = float(since_raw) if since_raw is not None else time.time()
            except (TypeError, ValueError):
                last_ts = time.time()
+            if since_raw is not None and last_ts < 1_000_000_000:


CRITICAL: Same nan/inf bypass as in _handle_a2a_messages

float('inf') < 1_000_000_000 is False, so since=inf reaches service.a2a_feed(since=inf, ...) and the SSE loop runs forever sending keepalives. since=nan causes m["ts"] > last_ts to always be False (NaN comparison), so every poll returns empty and the stream hangs.

Add math.isnan(last_ts) or math.isinf(last_ts) before or alongside the range check.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

kilo-code-bot · 2026-08-14T06:30:00Z

+    assert status == 200
+    msgs = body["messages"]
+    assert len(msgs) == 1
+    assert msgs[0]["body"] == "after pivot"


WARNING: No test coverage for nan/inf rejection despite the PR title claiming to fix them

The PR title says "nan and inf walk straight through the new since guard" but no tests were added for since=nan or since=inf on either endpoint. Without these tests the claimed fix cannot be verified and may regress.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

kilo-code-bot · 2026-08-14T06:30:11Z

Code Review Summary

Status: 3 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	2
WARNING	1

Issue Details (click to expand)

CRITICAL

File	Line	Issue
`taosmd/http_server.py`	1514	`nan` and `inf` bypass the `since < 1_000_000_000` guard because NaN comparisons always return False and inf is not less than 1 billion. These values reach `archive.query()` where they are truthy and get injected into SQL, returning empty/wrong results instead of a 400. The PR title claims to fix this but the check doesn't catch them.
`taosmd/http_server.py`	1566	Same `nan`/`inf` bypass in the SSE stream handler. `since=inf` causes the poll loop to run forever sending keepalives; `since=nan` causes every poll to return empty and the stream hangs.

WARNING

File	Line	Issue
`tests/test_a2a.py`	381	No test coverage for `nan`/`inf` rejection despite the PR title claiming to fix them. Without these tests the claimed fix cannot be verified and may regress.

Files Reviewed (2 files)

taosmd/http_server.py - 2 issues
tests/test_a2a.py - 1 issue

Fix these issues in Kilo Cloud

_{Reviewed by step-3.7-flash · Input: 109.4K · Output: 16.2K · Cached: 1.2M}

jaylfc · 2026-08-14T07:41:41Z

Closing this as superseded, and the reason is not "master moved" - it is that this PR does not fix the defect in its own title, and master already does. Everything below was run, not read.

1. `nan` and `inf` still walk straight through

The title is "nan and inf walk straight through the new since guard". The change adds an epoch-floor comparison at the two handler sites:

if since is not None and since < 1_000_000_000:
    raise _BadRequest(...)

float("nan") < 1e9 is False and float("inf") < 1e9 is False, so neither is caught. Live probe against a real server on this branch, controls in the same run:

CODE UNDER TEST: .../wt269/taosmd/http_server.py     (has _parse_since helper: False)
       since |    /a2a/messages |                /a2a/stream | note
         nan |              200 |              ACCEPTED(200) | <-- the defect this PR is named after
         inf |              200 |              ACCEPTED(200) | <-- the defect this PR is named after
        -inf |    REJECTED(400) |              REJECTED(400) |
        1444 |    REJECTED(400) |              REJECTED(400) | CONTROL msg id      -> must REJECT
           0 |    REJECTED(400) |              REJECTED(400) | CONTROL below floor -> must REJECT
         abc |    REJECTED(400) |              ACCEPTED(200) | CONTROL unparseable -> must REJECT
  1786000000 |              200 |              ACCEPTED(200) | CONTROL valid epoch -> must ACCEPT

The -inf row is what proves the mechanism rather than just the symptom: -inf < 1e9 IS True, so it is rejected incidentally by the comparison, while nan and +inf are not. A comparison-based guard cannot catch non-finite values; only a finite check can.

And the tests here do not cover it. The five new tests exercise 1444, 0, -1, omitted, and a valid epoch. Not one passes nan or inf. That is why CI is green on a PR whose stated defect is unfixed - same shape as the vacuous-test problem on #264.

2. A second defect this introduces on `/a2a/stream`

Row abc above: /a2a/stream?since=abc returns 200 on this branch and 400 on master. The stream handler keeps its except (TypeError, ValueError): last_ts = time.time(), so an unparseable since silently becomes "now" instead of erroring - a client with a malformed cursor gets a stream that looks fine and silently skips history.

3. Master already does all of it, via #253 (`e3ea7c3`)

Same probe, same controls, against master:

CODE UNDER TEST: .../wtmaster/taosmd/http_server.py  (has _parse_since helper: True)
         nan |    REJECTED(400) |              REJECTED(400)
         inf |    REJECTED(400) |              REJECTED(400)
        -inf |    REJECTED(400) |              REJECTED(400)
        1444 |    REJECTED(400) |              REJECTED(400)
           0 |    REJECTED(400) |              REJECTED(400)
         abc |    REJECTED(400) |              REJECTED(400)
  1786000000 |              200 |              ACCEPTED(200)

_parse_since (http_server.py:267) does math.isfinite and the 1e9 floor, and it is wired into both call sites (:1617 messages, :1661 stream) - the exact two sites this PR patches inline.

4. The check that settles it

I lifted this PR own test file onto master unchanged and ran it there:

8 passed, 28 deselected

Every acceptance test this PR ships is already green on master without this PR code. That is the cleanest possible statement that the code change is redundant.

What is NOT redundant, and is being kept

test_http_a2a_stream_since_message_id_rejected covers /a2a/stream, and master has no since-rejection test on the stream endpoint at all - master two tests (test_a2a_since_nan_rejected, test_a2a_since_inf_rejected) only hit /a2a/messages. Master stream behaviour is correct today but unguarded, so a regression there would be silent. Carded, with the note that these tests pass on master as-is.

This PR conflicts with master and is not mergeable as it stands. No work is being thrown away that is not being carried into that card. Branch untouched.

jaylfc · 2026-08-14T07:41:42Z

Closed as superseded by #253 (e3ea7c3). Full measurement in the comment above: this branch's own acceptance tests pass on master without this branch's code, and its guard does not catch nan/inf. Stream-endpoint test coverage salvaged to a card.

Master already rejects a bad since on /a2a/stream via _parse_since, but no test covered it. Lift the stream rejection tests from PR #269 and extend to nan, inf, a message id (1444) and an unparseable value (abc), each asserting HTTP 400. Add a valid-epoch accept case (1786000000) that reads only the SSE status line over a raw socket, so the never-closing accept stream cannot hang the test. Covers the PR #269 regression where /a2a/stream?since=abc returned 200.

carry forward the tsk-jpioif work

6d5c37b

kilo-code-bot Bot reviewed Aug 14, 2026

View reviewed changes

jaylfc closed this Aug 14, 2026

This was referenced Aug 14, 2026

A2A: reject implausible since values on stream/messages (id-vs-timestamp footgun, taOS-dev 1447) #246

Closed

Add since-rejection tests for /a2a/stream (master validates it correctly but nothing covers it) #278

Merged

Navigation Menu

Uh oh!

Revise PR #246: nan and inf walk straight through the new since guard - #269

Revise PR #246: nan and inf walk straight through the new since guard#269
jaylfc wants to merge 1 commit into
masterfrom
exec/tsk-dx6fok

jaylfc commented Aug 14, 2026 •

edited by gitar-bot Bot

Loading

Uh oh!

qodo-code-review Bot commented Aug 14, 2026

Uh oh!

coderabbitai Bot commented Aug 14, 2026

Review limit reached

Uh oh!

gitar-bot Bot commented Aug 14, 2026

Uh oh!

kilo-code-bot Bot Aug 14, 2026

Uh oh!

kilo-code-bot Bot Aug 14, 2026

Uh oh!

kilo-code-bot Bot Aug 14, 2026

Uh oh!

kilo-code-bot Bot commented Aug 14, 2026 •

edited

Loading

CRITICAL

WARNING

Uh oh!

jaylfc commented Aug 14, 2026

Uh oh!

jaylfc commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jaylfc commented Aug 14, 2026 • edited by gitar-bot Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Gitar

Uh oh!

qodo-code-review Bot commented Aug 14, 2026

Uh oh!

coderabbitai Bot commented Aug 14, 2026

Review limit reached

Uh oh!

gitar-bot Bot commented Aug 14, 2026

Uh oh!

kilo-code-bot Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

kilo-code-bot Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

kilo-code-bot Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

kilo-code-bot Bot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Summary

Overview

CRITICAL

WARNING

Uh oh!

jaylfc commented Aug 14, 2026

1. nan and inf still walk straight through

2. A second defect this introduces on /a2a/stream

3. Master already does all of it, via #253 (e3ea7c3)

4. The check that settles it

What is NOT redundant, and is being kept

Uh oh!

jaylfc commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jaylfc commented Aug 14, 2026 •

edited by gitar-bot Bot

Loading

kilo-code-bot Bot commented Aug 14, 2026 •

edited

Loading

1. `nan` and `inf` still walk straight through

2. A second defect this introduces on `/a2a/stream`

3. Master already does all of it, via #253 (`e3ea7c3`)