fix(slack): typed ingress schemas + streaming/recipient/dedupe/upload correctness + tests - #365
Conversation
… handlers interface-smoke --assert-typed-schemas flagged slack::events and slack::interactions as AnyValue. They are raw-Value HTTP handlers that read the request body off the request_body channel (Slack HMAC requires the exact raw bytes, unlike telegram-bot's header secret-token compare), so the auto-derived schema is empty. Attach typed request/response schemas via RegisterFunction::request_format/ response_format with a schema-only request struct and a null response — the same pattern shell and image-resize use for handlers whose auto-schema is wrong. build, test, fmt, clippy -D warnings all green.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a crate-internal Slack HTTP Trigger Schema Wiring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 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 |
skill-check — worker0 verified, 28 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@slack/src/functions/events.rs`:
- Around line 23-24: The event payload schema is accidentally marking body as
optional because of the serde default on the Events function struct. Remove
#[serde(default)] from the body field in the relevant Events/event wrapper type
so schemars keeps body in required and the published trigger schema still
advertises the payload field downstream diagnostics expect.
In `@slack/src/functions/interactions.rs`:
- Around line 24-25: The published schema is incorrectly marking the
Interactions request `body` field as optional because `#[serde(default)]` is
present on the `body: Value` field in the `Interactions` request type. Remove
the defaulting behavior from that field so schemars keeps `body` required, and
verify the request struct that defines `body` still serializes/deserializes
correctly without implying a missing payload is allowed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 463f5380-1826-49de-bcec-02ad9cf31d1e
📒 Files selected for processing (3)
slack/src/functions/events.rsslack/src/functions/interactions.rsslack/src/functions/mod.rs
| #[serde(default)] | ||
| body: Value, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In schemars 0.8, does marking a struct field with #[serde(default)] remove that field from the generated JSON Schema "required" array?
💡 Result:
Yes, in schemars 0.8, marking a struct field with #[serde(default)] (or the equivalent #[schemars(default)]) will remove that field from the generated JSON Schema's "required" array [1][2][3]. By applying this attribute, you are informing schemars that the field has a default value, which allows the generated schema to treat the field as optional. Additionally, schemars will generally include the default value in the "default" property of the field's schema definition, unless that behavior is suppressed by other attributes like skip_serializing_if [1][2][3].
Citations:
- 1: https://graham.cool/schemars/v0/deriving/attributes/
- 2: https://shadow.github.io/docs/rust/schemars/derive.JsonSchema.html
- 3: https://docs.rs/schemars/latest/schemars/derive.JsonSchema.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' slack/src/functions/events.rs
printf '\n---\n'
sed -n '1,220p' lsp/src/diagnostics.rsRepository: iii-hq/workers
Length of output: 9915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the schema helper and the registration path.
rg -n "schema_value::<|fn schema_value|request_format\\(" slack src lsp -g '!target'Repository: iii-hq/workers
Length of output: 507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' slack/src/functions/mod.rs
printf '\n---\n'
sed -n '1,120p' slack/src/functions/interactions.rsRepository: iii-hq/workers
Length of output: 7671
Keep body required in the published schema Remove #[serde(default)]; schemars will drop body from required, so the trigger schema no longer advertises the payload field downstream diagnostics expect.
🤖 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 `@slack/src/functions/events.rs` around lines 23 - 24, The event payload schema
is accidentally marking body as optional because of the serde default on the
Events function struct. Remove #[serde(default)] from the body field in the
relevant Events/event wrapper type so schemars keeps body in required and the
published trigger schema still advertises the payload field downstream
diagnostics expect.
| #[serde(default)] | ||
| body: Value, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In schemars 0.8, does marking a struct field with #[serde(default)] remove that field from the generated JSON Schema "required" array?
💡 Result:
Yes, in schemars 0.8, marking a struct field with #[serde(default)] (or #[schemars(default)]) causes the field to be excluded from the generated JSON Schema "required" array [1][2][3]. When this attribute is present, the field is treated as optional in the generated schema, and the field's schema object will include a "default" property, unless it is explicitly skipped by a #[serde(skip_serializing_if = "...")] attribute [1][2][3]. This behavior aligns with the library's goal of ensuring the generated schema reflects how serde_json serializes and deserializes the corresponding Rust struct [4][5]. While there have been minor regressions and fixes regarding this logic in various 0.8.x patch releases (such as handling conflicts between default, required, and skip_serializing_if), the standard functionality is that #[serde(default)] signals to the schema generator that the field is not required [6][7].
Citations:
- 1: https://docs.rs/schemars/latest/schemars/derive.JsonSchema.html
- 2: https://graham.cool/schemars/v0/deriving/attributes/
- 3: https://graham.cool/schemars/deriving/attributes/
- 4: https://docs.rs/crate/schemars/0.8.21
- 5: https://github.com/GREsau/schemars
- 6: https://github.com/GREsau/schemars/blob/master/CHANGELOG.md
- 7: Behavior change in 0.8.19 with
default,required, andskip_serializing_ifGREsau/schemars#292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the request struct and schema wiring in the target file.
sed -n '1,120p' slack/src/functions/interactions.rs
# Look for the request schema generation path used by this codebase.
rg -n "schema_value::<InteractionsHttpRequest>|request_format\\(|required\\]" slack/src lsp/srcRepository: iii-hq/workers
Length of output: 3656
Keep body required in the published schema
#[serde(default)] makes schemars treat body as optional, so downstream tooling will not flag a missing payload even though this request shape depends on it.
Possible fix
struct InteractionsHttpRequest {
/// The url-encoded interactivity body (also delivered raw on the request_body channel).
- #[serde(default)]
body: Value,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[serde(default)] | |
| body: Value, | |
| body: Value, |
🤖 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 `@slack/src/functions/interactions.rs` around lines 24 - 25, The published
schema is incorrectly marking the Interactions request `body` field as optional
because `#[serde(default)]` is present on the `body: Value` field in the
`Interactions` request type. Remove the defaulting behavior from that field so
schemars keeps `body` required, and verify the request struct that defines
`body` still serializes/deserializes correctly without implying a missing
payload is allowed.
…+ tests Audit against the 0.20.0 workers (telegram-bot streaming pattern, shell schema pattern) surfaced real defects beyond the typed-schema CI fix: - streaming: serialize per-session (RuntimeState::stream_lock) and gate on message-updated revision (drop stale/out-of-order), mirroring telegram-bot's per-entry lock + revision monotonicity. Fixes lost-update races and double chat.startStream under concurrent revision events. - channel streaming: send recipient_user_id AND recipient_team_id for channels, none for DMs (channel id starts with 'D'). Native channel streaming was rejected by Slack before this. - delta: skip a non-prefix rewrite on the native append path (can't retract), instead of duplicating content. - dedupe: bound seen_events with a time window + eviction cap (was unbounded). - files.upload: POST the bytes as multipart/form-data (Slack's -F file=@ shape), not a raw body. - identity: keep last-known-good on a transient reload auth.test failure (config-status still clears on an explicit check). - strip_mention: tidy the gap left by a removed mention; keep other users' mentions and newlines. Tests: +13 (per-session lock identity/serialization, revision guard, DM detection, delta reset/truncation, socket/http/bridge precedence, mention tidy/preserve, dedupe new/repeat + eviction). 34 unit + manifest + schema, all green; fmt + clippy -D warnings clean. Skipped from the audit (with reason): conversations.open selector validation (thin passthrough by design), self-loop hardening (already guarded by bot_id.is_some() || is_self), pre-verify body size cap (HMAC-gated route).
rohitg00
changed the title
1 participant
Summary
Post-merge fixes for the
slackworker, from a full audit against the 0.20.0 workers (telegram-bot's streaming pattern, shell's schema pattern) and the open dep/convention PRs.CI unblock
slack::events/slack::interactionsare raw-ValueHTTP handlers (must read the raw body off therequest_bodychannel because Slack HMAC needs the exact bytes, unlike telegram-bot's header secret-token compare). A raw-Valuehandler publishes the AnyValue schema and failedinterface-smoke --assert-typed-schemas. Fixed by attaching typed request/response schemas viaRegisterFunction::request_format/response_format+ aschema_valuehelper — the patternshellandimage-resizeuse.Correctness (audit findings, verified against code)
RuntimeState::stream_lock) and gate on themessage-updatedrevision (drop stale/out-of-order), mirroring telegram-bot's per-entry lock + revision monotonicity. Concurrent revision events no longer lose appended text or open two streams.recipient_user_idandrecipient_team_idfor channels, neither for DMs (D…).chat.startStreaminto a channel was rejected before this.seen_eventsnow time-windowed with an eviction cap.multipart/form-data(-F file=@), not a raw body.auth.testfailure (so mention/self detection isn't blinded);slack::config-statusstill clears on an explicit check.Tests
+13 unit tests (per-session lock identity + serialization, revision guard, DM detection, delta reset/truncation, socket/http/bridge precedence, mention tidy/preserve, dedupe new-repeat + eviction). 34 unit + manifest + schema golden tests, all green;
fmt+clippy -D warningsclean.Skipped from the audit (with reason)
conversations.openselector validation — the whole API surface is thin passthrough by design; Slack validates.bot_id.is_some() || is_self.