Skip to content

verify-merged-assertions.md and the blocks_without_body docstring quote the HTTP handler's error string as the service layer's - #313

Merged
jaylfc merged 1 commit into
masterfrom
exec/tsk-cwzaab
Aug 17, 2026
Merged

verify-merged-assertions.md and the blocks_without_body docstring quote the HTTP handler's error string as the service layer's#313
jaylfc merged 1 commit into
masterfrom
exec/tsk-cwzaab

Conversation

@jaylfc

@jaylfc jaylfc commented Aug 17, 2026

Copy link
Copy Markdown
Owner

CARD TITLE (intent, not commit subject): verify-merged-assertions.md and the blocks_without_body docstring quote the HTTP handler's error string as the service layer's

Autonomous build of board card tsk-cwzaab.

Files:
changelog.d/tsk-cwzaab-fix-quoted-error-strings.md | 7 +++++++
docs/verify-merged-assertions.md | 3 ++-
tests/test_a2a.py | 2 +-
3 files changed, 10 insertions(+), 2 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 17, 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: 12 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: 46ef8c4c-9809-4346-9e41-6bc4da43e9a1

📥 Commits

Reviewing files that changed from the base of the PR and between 0bf8c9f and a6d3fc6.

📒 Files selected for processing (3)
  • changelog.d/tsk-cwzaab-fix-quoted-error-strings.md
  • docs/verify-merged-assertions.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 17, 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

@@ -0,0 +1,7 @@
### Fixed

- Corrected quoted error strings in `docs/verify-merged-assertions.md` and

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: Changelog wording is slightly imprecise for the docs change

The changelog says "Corrected quoted error strings in docs/verify-merged-assertions.md", but that file did not previously contain a quoted error string — it only referred to "the error text." The PR adds the quoted service-layer error string rather than correcting an existing quote. Consider rewording to "Added the correct service-layer error string quote to docs/verify-merged-assertions.md and corrected the existing quote in tests/test_a2a.py" for accuracy.


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

@kilo-code-bot

kilo-code-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
changelog.d/tsk-cwzaab-fix-quoted-error-strings.md 3 Changelog says "Corrected quoted error strings in docs/verify-merged-assertions.md" but that file did not previously contain a quoted error string — it only referred to "the error text." The PR adds the quoted service-layer error string rather than correcting an existing quote.
Files Reviewed (3 files)
  • changelog.d/tsk-cwzaab-fix-quoted-error-strings.md - 1 issue
  • docs/verify-merged-assertions.md
  • tests/test_a2a.py

Fix these issues in Kilo Cloud


Reviewed by step-3.7-flash · Input: 168K · Output: 20.7K · Cached: 2.4M

@jaylfc

jaylfc commented Aug 17, 2026

Copy link
Copy Markdown
Owner Author

MERGE — the corrected string is right, and I measured it rather than read it

Reviewed at a6d3fc61 on a trial merge with master 8aa62e4 (clean, ort).

The whole subject of this card is which layer emits which string, so reading the diff would have
been the wrong check. I reproduced the card's own experiment on the merged tree: disabled the R8
branch in http_server.py (blocks present => body non-empty), ran the test, restored it.

R8 disabled:
    assert status == 400                                     PASSES  (service layer still 400)
>   assert "blocks" in body["error"]
E   AssertionError: assert 'blocks' in 'body must be a non-empty string'

R8 restored:  1 passed, 34 deselected

body must be a non-empty string is exactly what the branch now quotes, in both places. The two
source references in the changelog are accurate line-for-line:

taosmd/service.py:436       raise ValueError("body must be a non-empty string")
taosmd/http_server.py:1546  raise _BadRequest("'body' (non-empty string) is required")

That second one is the if not blocks branch — the handler's own message, which is what the doc
and the docstring were wrongly attributing to the service layer. The diagnosis on the card holds.

The fix is complete, not just correct. The stale string survives nowhere:

$ grep -rn "'body' (non-empty string) is required" docs/ tests/
(no matches)

Both call sites now carry the measured text, and the R8 handler message
(...is required when 'blocks' is present) is untouched, which is right — it is the message the
test's "blocks" in body["error"] assertion actually depends on.

Checks

conflict markers            clean
deleted-symbols-guard       clean, exit 0
normalise-handle-gate       clean, exit 0
full suite (trial merge)    1435 passed, 12 skipped, exit 0

The suite count matching master's baseline exactly is the correct outcome here, and the
contrast with #312 (blocked an hour ago for the same number) is worth stating so the rule is not
mis-learned: this PR changes prose only — a doc paragraph, a docstring, and a changelog. No
production code path is touched, so there is no behaviour for a new test to pin. #312 changed
_format_hit and reported the same count, which meant something entirely different.

One process note, not a defect

The card asked you to quote the message from a measurement and paste the assertion error you
actually saw. The corrected string is right, so the measurement clearly happened somewhere, but it
is not in the PR — which meant I had to re-run it to confirm rather than check it. Paste the output
next time; it converts a claim a reviewer must verify into evidence a reviewer can read.

Trivial, ignore or fix at will: the changelog file has no trailing newline (so does
tsk-35fsll-eventqa-exit-code.md, so this is pre-existing drift rather than something you
introduced), and the reflow left The test asserts alone on a short line.

Merging. Card tsk-cwzaab closed.

Reviewed by @jaylfc.

@jaylfc
jaylfc merged commit d79ad48 into master Aug 17, 2026
6 checks passed
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