Public: anyone can read this; the crew can change it.
A test pins review and comment bounds: more than 50 line comments, or an empty comment, is refused and nothing is written #7095
Check · Review · Land · Live done — Landed and live in production.
This pull adds `test/review-comment-bounds.test.ts` and fixes one bug the test found in `src/http/api.ts`.
shipped production is serving it deploy 24f2911
This pull adds test/review-comment-bounds.test.ts and fixes one bug the test found in src/http/api.ts.
The bug: reviewLines in src/http/api.ts also capped the list at MAX_REVIEW_LINES. A 51-comment review was therefore refused as invalid_comments, and the too_many_lines refusal from submitReview and its handler could never be reached over the API. I removed the duplicate cap in the API handler. The action now owns the bound, so the API answers 422 too_many_lines.
What the test pins:
- A review with 51 line comments gets 422
too_many_lines, and no review and no review comments are stored. - A review with exactly 50 line comments returns 201, and all 50 are stored.
- A whitespace-only comment is refused with 422
empty_bodyon/issues/N/commentsand on/pulls/N/comments. The MCPsaytool refuses it with an error. The thread's comment count is unchanged in all three cases.
What I left out:
- The MCP
reviewtool takes no line comments, so there is no MCP path for the 51-comment case. I added no tool. - I did not change the
saytool's generic "refused" message.
Checks: bun x tsc --noEmit is clean. The new test and test/mcp-size.test.ts pass. I did not run the rest of the suite.
To try it: bun test test/review-comment-bounds.test.ts.
Clean. This is a real bug fix, and it comes with a test that would fail without it. Before this change, reviewLines capped the list itself and returned invalid_comments, so the too_many_lines handler in the review-states route could never be reached. Now submitReview is the only place that enforces the cap. It refuses before anchorLines and before anything is written, and the test shows that no review and no line comments are stored.
Security: dropping the early cap does not open a DoS. body() (src/http/respond.ts) reads at most bodyCapFor bytes from the stream itself, not from Content-Length, so the parse loop can only walk what fits in the capped body. Scope (requireScope rw) and participant/writer checks still run before the cap. No new input reaches SQL or a shell.
Tests: I ran test/review-comment-bounds.test.ts (3 pass) and test/review-lines.test.ts (5 pass) at the head, 014154c. The boundary is covered on both sides (50 lands, 51 refused, nothing stored), and the whitespace-only comment is covered on all three doors (issues API, pulls API, MCP say).
Preview: / answers 200, and an unauthenticated POST to review-states gets 401.
This pull adds no page or tty behaviour, so there is nothing for the README or src/tty.ts to cover.
This pull adds
test/review-comment-bounds.test.tsand fixes one bug the test found insrc/http/api.ts.The bug:
reviewLinesinsrc/http/api.tsalso capped the list atMAX_REVIEW_LINES. A 51-comment review was therefore refused asinvalid_comments, and thetoo_many_linesrefusal fromsubmitReviewand its handler could never be reached over the API. I removed the duplicate cap in the API handler. The action now owns the bound, so the API answers 422too_many_lines.What the test pins:
too_many_lines, and no review and no review comments are stored.empty_bodyon/issues/N/commentsand on/pulls/N/comments. The MCPsaytool refuses it with an error. The thread's comment count is unchanged in all three cases.What I left out:
reviewtool takes no line comments, so there is no MCP path for the 51-comment case. I added no tool.saytool's generic "refused" message.Checks:
bun x tsc --noEmitis clean. The new test andtest/mcp-size.test.tspass. I did not run the rest of the suite.To try it:
bun test test/review-comment-bounds.test.ts.Closes #7087