Skip to content

_stream_rejection discards bytes already read, so it works only by luck of buffer sizes - #346

Merged
jaylfc merged 1 commit into
masterfrom
exec/tsk-ocjut3
Aug 18, 2026
Merged

_stream_rejection discards bytes already read, so it works only by luck of buffer sizes#346
jaylfc merged 1 commit into
masterfrom
exec/tsk-ocjut3

Conversation

@jaylfc

@jaylfc jaylfc commented Aug 18, 2026

Copy link
Copy Markdown
Owner

CARD TITLE (intent, not commit subject): _stream_rejection discards bytes already read, so it works only by luck of buffer sizes

Autonomous build of board card tsk-ocjut3.

Files:
changelog.d/tsk-ocjut3-stream-rejection-buffer.md | 3 +++
tests/test_a2a.py | 32 ++++++++---------------
2 files changed, 14 insertions(+), 21 deletions(-)

@qodo-code-review

Copy link
Copy Markdown

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

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

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

Next review available in: 50 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

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: 5a6411f2-749f-4220-aabc-ffed7eb15c9f

📥 Commits

Reviewing files that changed from the base of the PR and between 12d7710 and 092a57a.

📒 Files selected for processing (2)
  • changelog.d/tsk-ocjut3-stream-rejection-buffer.md
  • 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

gitar-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

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

kilo-code-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • changelog.d/tsk-ocjut3-stream-rejection-buffer.md
  • tests/test_a2a.py - 1 issue fixed, no new issues

Reviewed by step-3.7-flash · Input: 60.5K · Output: 12.4K · Cached: 331.4K

@jaylfc

jaylfc commented Aug 18, 2026

Copy link
Copy Markdown
Owner Author

MERGE — the fix is real, and I proved it rather than taking the rewrite on faith.

The card's acceptance asked for the evidence to be pasted in the PR and none of it was (the body is
boilerplate). So I ran every required probe myself. All of it passes, including the control that
distinguishes a genuine fix from a tidy-up, so the risk that requirement guards against is retired and I am
merging rather than bouncing it for paperwork. The gap is recorded at the bottom.

The direct test of the bug — chunk-size sensitivity, with master as the control

This is the probe the card called "the part that distinguishes a real fix from a rewrite". Patched by line
number inside _stream_rejection only, and I printed each patched line before running, because my first
attempt at this silently applied nothing and still printed five green passes:

helper                     recv(128)   recv(4096)   recv(65536)
master  (old, 2 sites)     5 passed        —        4 failed, 1 passed   ← the latent bug, REPRODUCED
PR #346 (new, 1 site)      5 passed    5 passed     5 passed             ← insensitive

Master genuinely breaks when the whole response lands in one read — the empty-headers_raw path the card
predicted. The new helper does not care. That is the defect fixed, not merely re-expressed.

FAST-red property preserved (the reason #280 exists)

last_ts = _parse_since(since_raw)last_ts = None at taosmd/http_server.py:1678:

RED    4 failed on assert 200 == 400, 1 passed, in 3.11s   (wall clock 3.652s real; no hang, exit ≠ 124)
GREEN  restored → 5 passed in 2.54s                        (wall clock 3.069s real)

The "do not" list is respected

test names / assertions changed      none  (no +/- line matches '^(def test|assert)')
_stream_status_code, _read_sse_frames  absent from the diff entirely
body-substring checks                 'finite' 5, 'timestamp' 3, '1444' 3, 'float timestamp' 1 — all intact

Trial merge onto master (12d77108)

conflict markers          clean
deleted-symbols-guard     clean   (output read, not exit code)
normalise-handle-gate     clean
witness-gate              clean
full suite                1552 passed, 12 skipped   ← exactly baseline

Baseline is the correct number here: this rewrites a helper, adds no tests and deletes none. The five
stream_since tests exercising the helper pass in every configuration above.

Recorded, not blocking

  1. The PR body pasted none of the required proofs. The card asked in three places — "Paste the
    wall-clock", "Paste both", "run it at both 128 and 65536 and paste both" — and explicitly warned "a green
    alone proves nothing here". A future card should not assume the reviewer will re-run the harness.
  2. changelog.d/tsk-ocjut3-stream-rejection-buffer.md has no trailing newline.
  3. Pre-existing and untouched by this PR, so not carded: if the peer closes before any \r\n\r\n arrives,
    data is empty and header_text.decode(…).splitlines()[0] raises IndexError rather than the intended
    AssertionError. Master has the same hole; the rewrite neither adds nor removes it.

Merging by squash; the measurements above go into the squash message.

@jaylfc
jaylfc merged commit de6252e into master Aug 18, 2026
7 checks passed
jaylfc added a commit that referenced this pull request Aug 18, 2026
…of/review_by coercion guards forward with tests (#347)

Revision of the closed #312. All three blockers from tsk-agi5ph resolved, both non-blocking items
addressed, and the tests verified red-first rather than taken on the body's word.

RED FIRST, this PR's test files run UNCHANGED against #307 (swap proven by md5 first:
test_api.py d2736eae and test_collections_ingest.py c5787bc1 identical on both sides;
api.py ce0ac0e0 (#307) vs ccc68390 (#347) genuinely differs):

    the card's five-case table, on #307:
      FAILED  coerce_iso_string_timestamp_returns_hits    ValueError
      FAILED  coerce_int_review_by_returns_hits           TypeError '<' int/str
      FAILED  coerce_list_review_by_returns_hits          TypeError '<' list/str
              coerce_str_review_by_overdue_is_past_review  passed
              coerce_plain_metadata_returns_hits           passed
      3 failed, 2 passed          -- failure-for-failure identical to the card's prediction
    all six coercion tests on #307: 4 failed, 2 passed  -- exactly the body's "4 of 6" claim

BLOCKER 2, the dropped producer, restored rather than rebuilt: taosmd/collections.py is
byte-identical to #307 (md5 744af0bb on both). The producer is genuinely absent on master --
the end-to-end test cannot even import there (ImportError: cannot import name
'_parse_front_matter'), so it discriminates. indexed_at is written at collections.py:909.
The PR body states the chosen direction explicitly, as acceptance required.

BLOCKER 3: the false UnboundLocalError changelog entry is gone; replacement fragment ends 0x0a.
Guards are now symmetric (api.py:486 as_of, api.py:494 review_by).

Gates clean (deleted-symbols, normalise-handle, witness), no conflict markers, no
Removes-Intentionally trailer. Suite 1617 passed, 12 skipped -- reconciles as the post-#351
baseline 1591 + the 26 tests added. The card's bar was >1435.

STATED LIMITATIONS.
- test_search_hit_metadata_always_has_doc_currency_fields PASSES on #307 too. It is a valid
  output-shape contract guard but is NOT evidence anything was fixed; the discriminating tests
  are the four coercion cases.
- The red-first baseline is #307, not master: against master these tests cannot import at all,
  so master is useless as a red baseline for the producer half.
- The PR body reports no suite count, which the card's acceptance asked for in as many words.
  Ran it here rather than bounce correct work. This is the sixth PR in that pattern
  (#340, #341, #345, #346, #348, #347) and is worth fixing at the source.
- api.py:498 builds is_past_review in a 110-character expression. No line-length rule exists
  in the repo, so style only; no change requested.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant