Review: PR#6 — Limit webhook request body size to 1MB
Overall: Looks good. Clean, minimal change that addresses the DoS vector.
Positives
Good use of io.LimitReader — idiomatic Go approach, no extra dependencies.
The 1MB constant is well-named and documented.
Test coverage is included and validates the behavior.
Observations / Minor Concerns
Silent truncation vs. rejection: The current approach silently truncates oversized bodies and lets the JSON parser fail downstream. This is acceptable (the webhook service handles invalid JSON gracefully per the test comment), but consider whether returning a 413 Request Entity Too Large would give webhook senders better feedback. Not a blocker — the current behavior is safe.
Test assertion: The test sends 2MB and expects 200 OK because the truncated body fails JSON parse but is handled gracefully. This is a valid integration test, but a comment noting this intentional behavior would help future readers (already partially there).
No Content-Length check: A preliminary Content-Length header check could reject oversized requests before reading any bytes, saving I/O. Minor optimization, not required.
Verdict: Approve-worthy. The fix is correct and sufficient for the stated issue.
## Review: PR#6 — Limit webhook request body size to 1MB
**Overall: Looks good. Clean, minimal change that addresses the DoS vector.**
### Positives
- Good use of `io.LimitReader` — idiomatic Go approach, no extra dependencies.
- The 1MB constant is well-named and documented.
- Test coverage is included and validates the behavior.
### Observations / Minor Concerns
1. **Silent truncation vs. rejection**: The current approach silently truncates oversized bodies and lets the JSON parser fail downstream. This is acceptable (the webhook service handles invalid JSON gracefully per the test comment), but consider whether returning a `413 Request Entity Too Large` would give webhook senders better feedback. Not a blocker — the current behavior is safe.
2. **Test assertion**: The test sends 2MB and expects `200 OK` because the truncated body fails JSON parse but is handled gracefully. This is a valid integration test, but a comment noting this intentional behavior would help future readers (already partially there).
3. **No `Content-Length` check**: A preliminary `Content-Length` header check could reject oversized requests before reading any bytes, saving I/O. Minor optimization, not required.
**Verdict: Approve-worthy.** The fix is correct and sufficient for the stated issue.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Review: PR#6 — Limit webhook request body size to 1MB
Overall: Looks good. Clean, minimal change that addresses the DoS vector.
Positives
io.LimitReader— idiomatic Go approach, no extra dependencies.Observations / Minor Concerns
413 Request Entity Too Largewould give webhook senders better feedback. Not a blocker — the current behavior is safe.200 OKbecause the truncated body fails JSON parse but is handled gracefully. This is a valid integration test, but a comment noting this intentional behavior would help future readers (already partially there).Content-Lengthcheck: A preliminaryContent-Lengthheader check could reject oversized requests before reading any bytes, saving I/O. Minor optimization, not required.Verdict: Approve-worthy. The fix is correct and sufficient for the stated issue.