-
Notifications
You must be signed in to change notification settings - Fork 0
v0.9.0: Generic payload support via PayloadCollector #10
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
Conversation
Introduces a generic type parameter P to CommandContext, allowing users to pass custom typed data from client to daemon handler. Payload is auto-collected via PayloadCollector::collect() before each command. - Add PayloadCollector trait with async fn collect() -> Self - Make CommandContext<P>, SocketMessage<P>, CommandHandler<P> generic - Make DaemonServer<H, P> and DaemonClient<P> generic - Default P = () maintains full backward compatibility - Add to prelude: PayloadCollector, Serialize, Deserialize 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add 7 unit tests for PayloadCollector and CommandContext<P> serialization - Add 2 integration tests for end-to-end payload flow through sockets - Total test count: 73 (was 64) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Users who need env vars should include them directly in their payload struct via PayloadCollector::collect(). This simplifies the API by unifying two mechanisms into one. Removed: - EnvVarFilter struct and methods - env_vars field from CommandContext - with_env_filter() from DaemonClient - 5 EnvVarFilter unit tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request replaces static environment-variable filtering with a generic payload mechanism: adds Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
59-62: Verify type inference works forDaemonClient::connectwithout explicit payload type.Line 60 uses
DaemonClient::connect(root_path)without a turbofish type parameter, while the struct is now generic overP. This relies on type inference from usage context. If the compiler cannot inferP, users may see confusing error messages.Consider adding explicit type annotation for clarity in the basic example:
-let mut client = DaemonClient::connect(root_path).await?; +let mut client = DaemonClient::<()>::connect(root_path).await?;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.claude/CLAUDE.md.gitignoreCHANGELOG.mdCargo.tomlPRD.mdREADME.mdexamples/cli.rsexamples/concurrent.rssrc/client.rssrc/lib.rssrc/server.rssrc/tests.rssrc/transport.rstests/integration_tests.rstests/version_tests.rs
💤 Files with no reviewable changes (2)
- PRD.md
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (6)
examples/cli.rs (2)
src/client.rs (1)
connect(65-77)src/transport.rs (1)
connect(164-181)
tests/integration_tests.rs (4)
src/client.rs (1)
connect_with_name_and_timestamp(87-214)src/lib.rs (3)
collect(153-153)collect(159-159)handle(414-420)src/tests.rs (5)
collect(194-199)handle(19-28)handle(142-158)handle(287-298)handle(312-324)src/server.rs (1)
new_with_name_and_timestamp(154-179)
examples/concurrent.rs (3)
tests/version_tests.rs (12)
client(62-62)client(110-110)client(160-160)client(209-209)client(231-231)client(281-281)client(327-327)client(391-391)client(440-440)client(462-462)client(469-469)client(488-488)src/client.rs (1)
connect(65-77)src/transport.rs (1)
connect(164-181)
src/client.rs (3)
src/lib.rs (3)
collect(153-153)collect(159-159)with_payload(197-203)src/tests.rs (1)
collect(194-199)tests/integration_tests.rs (1)
collect(1181-1186)
src/lib.rs (1)
src/tests.rs (1)
collect(194-199)
src/tests.rs (1)
src/lib.rs (4)
new(186-192)collect(153-153)collect(159-159)with_payload(197-203)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest)
🔇 Additional comments (29)
Cargo.toml (1)
3-3: LGTM!Version bump to 0.9.0 is appropriate for the breaking changes introduced in this PR.
examples/cli.rs (1)
59-59: LGTM!Correct usage of the new generic
DaemonClient::<()>API with the unit type for cases where no custom payload is needed.Also applies to: 137-137
examples/concurrent.rs (1)
326-326: LGTM!Consistent with the new generic API, using unit type
()as the payload parameter.tests/version_tests.rs (1)
58-62: LGTM!All
SocketMessageusages correctly updated toSocketMessage::<()>for the unit payload type. The test logic remains sound and properly exercises the version handshake flow.src/transport.rs (2)
237-240: LGTM!The
Commandvariant correctly propagates the generic payload typePthroughCommandContext<P>, enabling payload-based communication between client and server.
232-233: TheP: Defaultbound is necessary and correctly included.The serde bound at line 232 is correct. The
CommandContext<P>struct (lib.rs:173) has apayload: Pfield with the#[serde(default)]attribute. During deserialization, serde callsP::default()when the field is missing or during partial deserialization, which requires theDefaultbound. This is separate from the runtimePayloadCollector::collect()logic on the server side.tests/integration_tests.rs (2)
177-177: LGTM!All existing integration tests correctly updated to use
DaemonClient::<()>with the unit payload type.
1168-1316: Excellent payload test coverage!The new payload integration tests provide comprehensive coverage:
- Custom
TestPayloadtype withPayloadCollectorimplementationPayloadEchoHandlerto verify payload transmission- End-to-end tests for single and multiple commands with payloads
The tests successfully verify that:
- Payloads are collected via
PayloadCollector::collect()- Payloads are serialized and sent over the socket
- Server deserializes and passes payloads to handlers via
CommandContext<P>The limitation noted at lines 1254-1261 (inability to capture stdout for full verification) is acceptable given the test environment constraints. The tests passing confirms the full payload flow is working.
src/server.rs (3)
66-79: LGTM!Correct addition of generic parameter
Pwith default value()toDaemonServer:
PhantomData<P>properly maintains the type parameter in the type system without runtime overhead- Default parameter ensures backward compatibility
- Public fields remain accessible
99-102: LGTM!Trait bounds are correctly specified:
H: CommandHandler<P>ensures handler can process the payload typeP: PayloadCollectorensures payload can be collected- Combined with
Clone + 'staticfor proper async task spawning
278-393: LGTM!All
SocketMessageusages correctly updated toSocketMessage::<P>throughout the message handling flow:
- Version handshake (lines 278-284)
- Command reception (line 310)
- Error/completion messages (lines 370, 376)
- Output chunks (line 393)
The generic payload is properly threaded through the entire server communication pipeline.
.claude/CLAUDE.md (2)
94-106: LGTM! Documentation accurately reflects the updated handler trait.The documented
CommandHandler<P>trait withPayloadCollectorbound andCommandContext<P>parameter aligns correctly with the implementation insrc/lib.rs.
135-136: LGTM! Server construction example updated correctly.The example now includes
StartupReason::default()as the third parameter toDaemonServer::new, consistent with the updated API.README.md (1)
65-107: LGTM! Comprehensive custom payload example.The
PayloadCollectorimplementation example clearly demonstrates the pattern for collecting client-side data (cwd, user) and accessing it viactx.payloadin the handler.src/tests.rs (5)
17-29: LGTM! Basic handler implementation is correct.The
TestHandlercorrectly implementsCommandHandler(defaulting toP = ()) and usesCommandContextwithout explicit type parameter.
42-46: LGTM! Explicit type annotations ensure correct generic handling.Using
SocketMessage<()>explicitly ensures the serialization tests work correctly with the generic type system.
185-200: LGTM! Test payload and collector implementation.The
TestPayloadstruct withPayloadCollectorimplementation provides good test coverage for the generic payload mechanism.
209-214: LGTM! Unit type payload collector test.Testing the
()implementation ofPayloadCollectorensures backward compatibility with code that doesn't use custom payloads.
305-347: LGTM! End-to-end handler test with payload.The
test_handler_receives_payloadtest validates the complete flow from payload creation through handler execution, verifying that payload data is correctly accessible in the handler.src/client.rs (4)
55-58: LGTM! Generic impl block with correct bounds.The
PayloadCollectorbound onPensures payload collection is available for command execution.
143-152: LGTM! Version handshake correctly uses generic message type.The explicit
SocketMessage::<P>::VersionCheckturbofish syntax andreceive_message::<SocketMessage<P>>()ensure proper type resolution during handshake, even thoughVersionCheckdoesn't carry payload data.
618-632: LGTM! Payload collection happens at the right time.Collecting the payload immediately before command execution (
P::collect().await) ensures fresh data is captured for each command, which is the expected behavior per the trait documentation.
646-650: LGTM! Generic message receiving in command loop.The receive loop correctly uses the generic
SocketMessage<P>type, ensuring the command response handling is payload-aware.src/lib.rs (6)
148-154: LGTM! PayloadCollector trait has appropriate bounds.The trait bounds (
Serialize + DeserializeOwned + Send + Sync + Clone + Default + 'static) are necessary for:
- Serialization across the socket (Serialize, DeserializeOwned)
- Thread-safety in async context (Send, Sync)
- Handler cloning (Clone)
- Serde default initialization (Default)
- Static lifetime for trait objects ('static)
156-160: LGTM! Unit type implementation provides backward compatibility.The empty implementation for
()allows existing code without custom payloads to continue working unchanged.
171-182: Serde bound may fail deserialization when payload has non-default-constructible fields.The
#[serde(bound(deserialize = "P: Default + serde::de::DeserializeOwned"))]combined with#[serde(default)]on thepayloadfield means if the JSON lacks thepayloadkey, serde will useP::default(). This is correct behavior.However, the
_phantom: PhantomData<P>field is interesting—it has#[serde(skip)]which is correct since PhantomData shouldn't be serialized.
184-203: LGTM! Constructor separation is clean.Having
CommandContext::new()only forCommandContext<()>andwith_payload()for the generic case provides a clear API that guides users toward the correct pattern.
395-420: LGTM! CommandHandler trait correctly propagates the generic payload type.The trait accepts
CommandContext<P>whereP: PayloadCollector, ensuring handlers receive properly typed payloads. The documentation update at lines 405-407 accurately describes the payload access pattern.
262-270: LGTM! Prelude exports are comprehensive.Exporting
PayloadCollectorand serde traits (Serialize,Deserialize) in the prelude enables users to implement custom payloads without additional imports.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration_tests.rs (1)
84-84: Consider explicit type parameters for consistency.The handler implementations rely on implicit default type parameters (likely
CommandHandlerdefaults toCommandHandler<()>), while client usage throughout the file explicitly specifies types (DaemonClient::<()>). For consistency and clarity, consider using explicit type parameters:-impl CommandHandler for EchoHandler { +impl CommandHandler<()> for EchoHandler {This pattern would match the explicit
CommandHandler<TestPayload>usage in line 1194 and align with the client-side style.Also applies to: 104-104, 127-127, 152-152
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdtests/integration_tests.rs
🔇 Additional comments (4)
CHANGELOG.md (1)
18-36: Previous review issue has been resolved.The "Before (0.8.0)" example now correctly shows
DaemonClient::connect(path)without turbofish notation (line 20), which accurately reflects the pre-generic 0.8.0 API. The migration guide clearly demonstrates the transition to the payload-based design.tests/integration_tests.rs (3)
1141-1149: LGTM: Defensive connection rejection handling.The match logic appropriately handles race conditions in the connection limit test. By checking both connection failures and command execution failures, it correctly counts rejections without over-asserting, which is important given the concurrent nature and timing variations in the test.
1168-1316: Excellent payload test coverage.The new payload tests comprehensively verify the end-to-end payload flow:
TestPayloadandPayloadCollectorimplementation provide deterministic test dataPayloadEchoHandlervalidates payload transmission by echoing received valuestest_custom_payload_end_to_endverifies the complete flow: collection → serialization → transmission → deserialization → handler receptiontest_multiple_commands_with_payloadconfirms fresh payload collection per commandThe explicit use of
DaemonClient::<TestPayload>andCommandHandler<TestPayload>demonstrates the proper API usage patterns for the new generic payload feature.
177-177: Consistent explicit type parameter usage.All existing tests correctly updated to use
DaemonClient::<()>::connect_with_name_and_timestamp(...), maintaining consistency with the new generic payload API. The explicit unit type specification clearly documents that these tests use no custom payload.Also applies to: 211-211, 241-241, 275-275, 308-308, 410-410, 468-468, 539-539, 628-628, 670-670, 709-709, 745-745, 815-815, 874-874, 919-919, 942-942, 987-987, 1022-1022, 1060-1060, 1115-1115, 1133-1133
Summary
CommandContext<P>with custom payload support viaPayloadCollectortraitEnvVarFilter- users include env vars directly in their payload structBreaking Changes
EnvVarFilterremoved - usePayloadCollectorinsteadDaemonClient::with_env_filter()removedTest plan
🤖 Generated with Claude Code