Use the shared protocol#26
Merged
Merged
Conversation
Signed-off-by: kerthcet <kerthcet@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes the daemon↔server wire format into a new shared Rust crate (sandd-protocol) and updates both server and sandd to import protocol types from that crate instead of maintaining duplicated/embedded protocol definitions.
Changes:
- Added new workspace member crate
protocol(sandd-protocol) containing the sharedMessage,DaemonMetadata, andSnapshotInfotypes. - Updated
serverandsanddcrates to depend on and usesandd_protocol::*imports; removed the oldsanddlocal protocol module. - Strengthened snapshot-related message payloads to use typed
SnapshotInfoinstead of untypedserde_json::Value.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/src/server.rs | Switch protocol Message import to sandd_protocol. |
| server/src/registry.rs | Switch protocol imports to sandd_protocol. |
| server/src/lib.rs | Remove local protocol module usage and import shared Message. |
| server/Cargo.toml | Add path dependency on sandd-protocol. |
| sandd/src/snapshot/types.rs | Re-export SnapshotInfo from shared protocol; remove local definition. |
| sandd/src/session.rs | Switch protocol Message import to sandd_protocol. |
| sandd/src/protocol.rs | Delete duplicated protocol definitions from daemon crate. |
| sandd/src/main.rs | Remove local protocol module usage; switch imports/types to sandd_protocol. |
| sandd/Cargo.toml | Add path dependency on sandd-protocol. |
| protocol/src/lib.rs | Introduce shared protocol definitions; type snapshot payloads as SnapshotInfo. |
| protocol/Cargo.toml | Define new sandd-protocol crate and its dependencies. |
| Cargo.toml | Add protocol to workspace members. |
| Cargo.lock | Record new workspace package sandd-protocol in the lockfile. |
Comments suppressed due to low confidence (1)
protocol/src/lib.rs:9
created_atis documented as milliseconds, but snapshot creation currently usesSystemTime::now().duration_since(UNIX_EPOCH).as_secs()(seconds) insandd/src/snapshot/manager.rs. This makes the shared protocol ambiguous and will cause consumers to misinterpret timestamps if they follow the protocol docs. Either keep seconds (update docs/comments here and inprotocol::SnapshotInfo) or switch the producer to milliseconds and migrate/handle existing on-disk snapshot JSON.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
/lgtm |
Signed-off-by: kerthcet <kerthcet@gmail.com>
5ad9111 to
781752d
Compare
Signed-off-by: kerthcet <kerthcet@gmail.com>
94ebecb to
ec846d1
Compare
Member
Author
|
/lgtm |
InftyAI-Agent
approved these changes
Jul 1, 2026
InftyAI-Agent
left a comment
Member
There was a problem hiding this comment.
Approved: PR has both lgtm and approved labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Which issue(s) this PR fixes
Fixes #6
Special notes for your reviewer
Does this PR introduce a user-facing change?