Skip to content

Use the shared protocol#26

Merged
InftyAI-Agent merged 3 commits into
InftyAI:mainfrom
kerthcet:cleanup/share-protocol
Jul 1, 2026
Merged

Use the shared protocol#26
InftyAI-Agent merged 3 commits into
InftyAI:mainfrom
kerthcet:cleanup/share-protocol

Conversation

@kerthcet

@kerthcet kerthcet commented Jul 1, 2026

Copy link
Copy Markdown
Member

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?


Signed-off-by: kerthcet <kerthcet@gmail.com>
Copilot AI review requested due to automatic review settings July 1, 2026 05:52
@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 shared Message, DaemonMetadata, and SnapshotInfo types.
  • Updated server and sandd crates to depend on and use sandd_protocol::* imports; removed the old sandd local protocol module.
  • Strengthened snapshot-related message payloads to use typed SnapshotInfo instead of untyped serde_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_at is documented as milliseconds, but snapshot creation currently uses SystemTime::now().duration_since(UNIX_EPOCH).as_secs() (seconds) in sandd/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 in protocol::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.

Comment thread sandd/src/snapshot/types.rs Outdated
@kerthcet

kerthcet commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

/lgtm
/kind feature

@InftyAI-Agent InftyAI-Agent added lgtm Looks good to me, indicates that a PR is ready to be merged. feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Jul 1, 2026
Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet kerthcet force-pushed the cleanup/share-protocol branch from 5ad9111 to 781752d Compare July 1, 2026 10:40
@InftyAI-Agent InftyAI-Agent removed the lgtm Looks good to me, indicates that a PR is ready to be merged. label Jul 1, 2026
Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet kerthcet force-pushed the cleanup/share-protocol branch from 94ebecb to ec846d1 Compare July 1, 2026 17:05
@kerthcet

kerthcet commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

/lgtm

@InftyAI-Agent InftyAI-Agent added the lgtm Looks good to me, indicates that a PR is ready to be merged. label Jul 1, 2026

@InftyAI-Agent InftyAI-Agent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved: PR has both lgtm and approved labels

@InftyAI-Agent InftyAI-Agent merged commit 45ad881 into InftyAI:main Jul 1, 2026
13 checks passed
@kerthcet kerthcet deleted the cleanup/share-protocol branch July 1, 2026 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. feature Categorizes issue or PR as related to a new feature. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support snapshot in daemon

3 participants