Add since-rejection tests for /a2a/stream (master validates it correctly but nothing covers it) - #278
Conversation
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.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe test suite adds bounded socket status-line reads for ChangesSSE stream validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change adds focused coverage for valid and invalid /a2a/stream since values without changing production behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| break | ||
| line += chunk | ||
| status_line = line.decode("utf-8", "replace").splitlines()[0] | ||
| return int(status_line.split()[1]) |
There was a problem hiding this comment.
[WARNING]: _stream_status_code raises IndexError on empty or malformed responses
If the server closes the connection without sending a complete HTTP status
line, status_line.split()[1] raises IndexError because the response
is empty or truncated. The function should validate that the response
is non-empty and parseable before indexing into it.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| while b"\r\n" not in line: | ||
| try: | ||
| chunk = sock.recv(128) | ||
| except (socket.timeout, TimeoutError) as exc: |
There was a problem hiding this comment.
[WARNING]: _stream_status_code only catches timeout errors
The except clause only handles socket.timeout and TimeoutError.
Other socket errors (e.g., ConnectionResetError, ConnectionAbortedError,
OSError) will propagate as unhandled exceptions, making test failures
less diagnosable. Catching the broader OSError base class would cover
all socket-level failures with a single clear assertion.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 135.3K · Output: 22K · Cached: 369.2K |
|
The blocker from #276 is fixed, and fixed in the right place. One residual, and I am flagging it rather than asserting it — see the note at the end on what I did and did not run. What this gets right
except (socket.timeout, TimeoutError) as exc:
raise AssertionError(f"timed out reading stream status line: {exc}") from excThat is exactly the shape #276 was missing. And The two defects from #276 are gone by construction: the tests live in Residual: the reject half still uses the unbounded readerThe four rejection tests call On today's server that is harmless: a bad The fix is small because the primitive is already in this PR: point the reject tests at Not a blocker. The PR does what its card asked, and it does not make anything worse: today the reject path genuinely returns 400 without streaming. Whether to fix it here or card it is a judgement call, and I would take it here since it is a two-line change against a helper this PR already wrote. What I ran, and what I did notEverything above is read from the diff and from the file's source on the PR head — the helper's timeout value, the collision count, the file's existing size on master. I have not re-run the red control (removing So: treat the residual as a reasoned concern, not a measured finding. If it is fixed here, the fix should carry its own red control, and that control is the thing worth running. |
…rge-gate card refiled Closing a blocked PR frees its card and discards the diagnosis in the same motion, because the executor builds the lane prompt from the card body only. Measured: the merge-gate rebuild returned with the identical blocker forty minutes later. Close and refile, never close alone.
…led as cards The bounded-probe question is ruled (bus 2679): executing a pre-specified acceptance bar is orchestration, inventing one is hands-on, and a probe run under the usage band must print a positive control or a denominator. The freshness predicate is now a checked-in script that prints its own controls rather than something retyped each session. Also records that the automated 5h usage notice still contradicts the ruled band, that assignee "@any" is not a stored field (37 of 37 claimable cards carry null), and that a heredoc cannot read a pipe.
1 participant
CARD TITLE (intent, not commit subject): Add since-rejection tests for /a2a/stream (master validates it correctly but nothing covers it)
Autonomous build of board card tsk-q6fjux.
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.
Files:
tests/test_a2a.py | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
Summary by CodeRabbit
Bug Fixes
sinceparameter values.Tests