-
Notifications
You must be signed in to change notification settings - Fork 0
Implement Socrates Zulip bot with serverless architecture and quote responses #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: nikomatsakis <[email protected]>
socrates-dev.zulipchat.com. I want to be implemented in Rust and to run on serverless infrastructure. My first goal is that it will be receive a message like @socrates hi! and it will respond wi...
nikomatsakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @copilot -- thanks for the PR! I did a quick read. Thoughts?
Cargo.toml
Outdated
| [package] | ||
| name = "socrates" | ||
| version = "0.1.0" | ||
| edition = "2021" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use Rust 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to Rust 2024 edition in commit 0ec32c9.
Cargo.toml
Outdated
| serde = { version = "1.0", features = ["derive"] } | ||
| serde_json = "1.0" | ||
| reqwest = { version = "0.11", features = ["json"] } | ||
| lambda_runtime = "0.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not latest version of lambda-runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to lambda_runtime 0.14.4 (latest) in commit 0ec32c9.
| @@ -0,0 +1,15 @@ | |||
| # Build stage | |||
| FROM rust:1.89 as builder | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I don't love typing this to Rust 1.89, I would be happier if we were able to update it to the latest stable release-- but I am not very familiar with containers, and I can see the risks associated with that. Tell me more about how people normally manage this? What range of "things" can I write here, anyway? I take it this is a base container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Dockerfile currently uses rust:1.89 which is the current stable Rust version. For container management, you have several options:
Version strategies:
rust:1.89- pins to specific version (current approach)rust:1- always uses latest stable (more dynamic but less predictable)rust:1.89-slim- smaller image size
Container versioning:
Docker uses official Rust images from the Rust team. The rust:1.89 base image includes the Rust toolchain and build dependencies. For production, you could also use multi-stage builds with debian:bookworm-slim or alpine for smaller runtime images.
The current approach with rust:1.89 provides reproducible builds while staying reasonably current. Would you prefer switching to rust:1 for automatic updates or a different strategy?
src/main.rs
Outdated
| async fn function_handler(event: LambdaEvent<Value>) -> Result<Value, Error> { | ||
| info!("Received event: {:?}", event.payload); | ||
|
|
||
| // Handle different types of Lambda events (API Gateway, ALB, etc.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this with an enum that uses the "serde(untagged)" or whatever it's called to try various options in turn? I hate this kind of grody "peeking and poking" at JSON values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced the manual JSON poking with a proper LambdaEventType enum using serde(untagged) in commit 0ec32c9. Now it properly deserializes different Lambda event types (ApiGateway vs DirectInvoke) instead of manually checking for the "body" field. Much cleaner!
…ime, and improve JSON parsing with enum Co-authored-by: nikomatsakis <[email protected]>
Thanks for the review! I've addressed all the feedback points:
All tests pass and the bot is now more robust with better type safety. See commit 0ec32c9 for the updates. |
Cargo.toml
Outdated
| lambda_runtime = "0.14" | ||
| tracing = "0.1" | ||
| tracing-subscriber = "0.3" | ||
| rand = "0.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other crates are out of date as well. You should be using cargo add to add dependencies -- or maybe there's a better command to update them to the latest versions. Use that, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated all dependencies to latest versions in commit cf2e8c4:
- tokio 1.0 → 1.47 (latest stable)
- reqwest 0.11 → 0.12 (latest with HTTP/2 improvements)
- rand 0.8 → 0.9 (latest with updated API)
- All other dependencies updated via
cargo update
|
|
||
| #[derive(Debug, Deserialize)] | ||
| #[serde(untagged)] | ||
| enum LambdaEventType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you comment and explain this enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comprehensive documentation for the LambdaEventType enum in commit cf2e8c4. The enum uses serde(untagged) to automatically deserialize different AWS Lambda event types based on which fields are present in the JSON.
| #[serde(untagged)] | ||
| enum LambdaEventType { | ||
| ApiGateway { | ||
| body: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this json-encoded as a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the body field contains JSON-encoded webhook data as a string. When API Gateway receives a webhook POST, it wraps the original JSON payload in a string within the body field. That's why we parse it with serde_json::from_str(body) to deserialize the ZulipWebhook struct. Added clarifying documentation in commit cf2e8c4.
| path: Option<String>, | ||
| }, | ||
| DirectInvoke { | ||
| message: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same-- the json comes in as a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message field is for direct Lambda invocations (not typical for webhooks). Unlike API Gateway events where JSON comes as a string in the body field, direct invocations can have JSON directly in the event payload. Added documentation to clarify this distinction in commit cf2e8c4.
|
@copilot #1 (review) -- take a look at that |
…umentation Co-authored-by: nikomatsakis <[email protected]>
I can't access external URLs to see the specific review details. Could you copy the relevant feedback here so I can address it? Or let me know what specific changes are needed based on that review. |
Summary
Successfully implemented a complete Zulip bot in Rust with all latest dependencies and comprehensive documentation:
Latest Dependencies Update (commit a3e9c7f)
cargo updateto get latest compatible versionsDocumentation Improvements
LambdaEventTypeenumbodyfield contains JSON-encoded webhook data as stringTechnical Robustness
The bot now uses the most current versions of all dependencies while maintaining full functionality and type safety.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.