fix(notify): bound hook input and provider payload sizes (#618) - #666
Conversation
Cap stdin/hook reads at 256 KiB and truncate or refuse Discord, Telegram, and Signal payloads against documented ceilings so oversized events fail with a bounded payload_limit diagnostic instead of silent non-delivery. Closes #618 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAgent Notify now bounds hook and positional input to 256 KiB. Discord, Telegram, and Signal payloads are fitted to provider limits. Oversized content returns sanitized ChangesNotification size enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
solomonneas
added
the
coderabbit-review
ChatGPT Codex Connector
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stations/notify/cmd/agent-notify/main_test.go`:
- Around line 869-873: Update adapterMaxInputBytes in the CLI test helper to
return the exported adapter.MaxInputBytes constant instead of duplicating the
numeric limit, using the existing adapter package import and preserving
TestRun_OversizedStdinRejected’s behavior.
In `@stations/notify/internal/channels/limits_test.go`:
- Around line 64-101: Replace the non-executing fixture and direct
payloadLimitError call in TestDiscord_Send_PayloadLimitErrorHasNoSecrets with a
genuinely oversized message, such as a large Body combined with maximum title,
footer, and tags, that drives fitDiscordEmbed beyond DiscordEmbedTotalMax.
Invoke d.Send with a context and msg using the secret webhook URL, assert it
returns the payload-limit error, then run SafeError on that returned error and
verify it contains payload_limit without exposing the webhook URL or HTTP
scheme. Remove the unconditional skip and unused-variable workarounds.
In `@stations/notify/internal/channels/telegram.go`:
- Around line 100-134: Fix the binary-search direction in fitTelegramText: when
truncateRunes returns !ok for mid, increase the lower bound with lo = mid + 1
instead of decreasing hi, since larger budgets may succeed. Add a regression
test using a short body and near-limit title/tag overhead where truncation is
required but feasible, verifying fitTelegramText returns a valid payload rather
than payloadLimitError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df501fc9-08c9-48a0-877d-0f006a655e3a
📒 Files selected for processing (14)
stations/notify/cmd/agent-notify/main.gostations/notify/cmd/agent-notify/main_test.gostations/notify/internal/adapter/adapter.gostations/notify/internal/adapter/bound.gostations/notify/internal/adapter/bound_test.gostations/notify/internal/adapter/claude_code_notification.gostations/notify/internal/adapter/claude_code_stop.gostations/notify/internal/adapter/codex_notify.gostations/notify/internal/channels/discord.gostations/notify/internal/channels/errors.gostations/notify/internal/channels/limits.gostations/notify/internal/channels/limits_test.gostations/notify/internal/channels/signal.gostations/notify/internal/channels/telegram.go
Co-Authored-By: Codex <codex@openai.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
stations/notify/cmd/agent-notify/main.go (1)
306-308: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winValidate positional size before joining arguments.
strings.Joinand[]byte(joined)allocate the complete custom payload beforeCheckSizerejects it. Sum argument byte lengths and separators beforestrings.Join. Returnadapter.ErrInputTooLargewhen the total exceedsadapter.MaxInputBytes.Proposed change
- joined := strings.Join(posArgs, " ") - if err := adapter.CheckSize([]byte(joined)); err != nil { - return canonical.Message{}, err + size := len(posArgs) - 1 + for _, arg := range posArgs { + if len(arg) > adapter.MaxInputBytes-size { + return canonical.Message{}, adapter.ErrInputTooLarge + } + size += len(arg) } + joined := strings.Join(posArgs, " ") return adapter.FromString(joined), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stations/notify/cmd/agent-notify/main.go` around lines 306 - 308, Update the positional-argument handling around adapter.CheckSize to calculate the total UTF-8 byte length of posArgs plus one space separator between adjacent arguments before calling strings.Join. If the total exceeds adapter.MaxInputBytes, return adapter.ErrInputTooLarge without constructing the joined payload; otherwise preserve the existing join and CheckSize flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@stations/notify/cmd/agent-notify/main.go`:
- Around line 306-308: Update the positional-argument handling around
adapter.CheckSize to calculate the total UTF-8 byte length of posArgs plus one
space separator between adjacent arguments before calling strings.Join. If the
total exceeds adapter.MaxInputBytes, return adapter.ErrInputTooLarge without
constructing the joined payload; otherwise preserve the existing join and
CheckSize flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bfba3f46-c894-4847-a030-e6be789d3563
📒 Files selected for processing (14)
stations/notify/cmd/agent-notify/main.gostations/notify/cmd/agent-notify/main_test.gostations/notify/internal/adapter/adapter.gostations/notify/internal/adapter/bound.gostations/notify/internal/adapter/bound_test.gostations/notify/internal/adapter/claude_code_notification.gostations/notify/internal/adapter/claude_code_stop.gostations/notify/internal/adapter/codex_notify.gostations/notify/internal/channels/discord.gostations/notify/internal/channels/errors.gostations/notify/internal/channels/limits.gostations/notify/internal/channels/limits_test.gostations/notify/internal/channels/signal.gostations/notify/internal/channels/telegram.go
1 participant
Summary
adapter.ReadBounded(256 KiB) so allocation cannot grow without bound.DeliveryErrorcausepayload_limit; stderr stays onSafeError(no credentials or raw provider bodies).Acceptance criteria → tests
TestReadBounded_BelowLimit,TestReadBounded_AtLimit,TestReadBounded_AboveLimit,TestAutoDetect_AboveLimit,TestRun_OversizedStdinRejectedTestDiscord_Send_TruncatesDescriptionToLimit,TestTelegram_Send_TruncatesToTextMax,TestTelegram_Send_PayloadLimitWhenTitleAloneExceeds,TestSignal_Send_TruncatesToMessageMaxTestRun_PartialFailurePayloadLimitVisibleTestRun_PartialFailurePayloadLimitVisible,TestTelegram_Send_PayloadLimitWhenTitleAloneExceeds,TestDiscord_Send_PayloadLimitErrorHasNoSecretsTest plan
brigade work verify run --target . --command "./scripts/verify" --capture brigade-work20260801-215514-work-verify-2a81e8brigade work verify run --target ./stations/notify --command "go test ./..." --capture brigade-work20260801-215512-work-verify-0d3674Closes #618
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes