Skip to content

feat: pcp-messages protobuf#1266

Open
karelispanagiotis wants to merge 11 commits into
mainfrom
karelis/pcp-protobuf
Open

feat: pcp-messages protobuf#1266
karelispanagiotis wants to merge 11 commits into
mainfrom
karelis/pcp-protobuf

Conversation

@karelispanagiotis

@karelispanagiotis karelispanagiotis commented Jun 17, 2026

Copy link
Copy Markdown
Member

Introduces protobuf definitions for pcp messages to be included in the pcp-package.

@karelispanagiotis karelispanagiotis requested a review from a team as a code owner June 17, 2026 19:55
@github-actions

Copy link
Copy Markdown

No concrete merge-blocking issues found in the PR diff.

I checked the workspace wiring, proto paths, build.rs generation setup, exported Rust module, and git diff --check passed. I could not run cargo check -p orb-pcp-messages or buf lint in this sandbox because the environment is read-only and lacks buf/protoc, so CI should cover those checks.

@karelispanagiotis karelispanagiotis changed the title (feat) pcp-messages package (feat): pcp-messages protobuf Jun 17, 2026
@karelispanagiotis karelispanagiotis changed the title (feat): pcp-messages protobuf feat: pcp-messages protobuf Jun 17, 2026
@github-actions

Copy link
Copy Markdown

No concrete correctness, security, or regression issues found in the PR diff.

I could not fully verify locally: cargo check was blocked by the read-only sandbox trying to write Cargo/rustup cache state, and buf is not installed in this environment.

Comment thread pcp-messages/proto/pcp/v1/di_iris_embedding_shares.proto Outdated
Comment thread pcp-messages/proto/pcp/v1/di_iris_embeddings.proto Outdated
Comment thread Cargo.toml Outdated
Comment thread pcp-messages/README.md Outdated
@github-actions

Copy link
Copy Markdown

No concrete merge-blocking issues found in the PR diff.

I could not run cargo check -p orb-pcp-messages or buf lint in this sandbox because rustup cannot write to its home directory here, and protoc/buf are not installed. Static review only.

@github-actions

Copy link
Copy Markdown

I found no concrete merge-blocking issues in the PR diff.

I could not run full verification in this environment: cargo check hit a read-only rustup temp path, and buf/protoc are not installed outside the Nix shell here.

@github-actions

Copy link
Copy Markdown

No concrete merge-blocking issues found in the PR diff.

I couldn’t execute the new crate tests here because the read-only environment prevented rustup from creating temp files under /home/runner/.rustup; buf also isn’t installed in this runner. The schema/build wiring looks consistent with the workspace and CI’s nix develop setup.

@github-actions

Copy link
Copy Markdown

I found no concrete merge-blocking issues in the changes introduced by this PR.

Note: I couldn’t run the Rust checks locally because this sandbox is read-only/offline for the required toolchain/dependencies, so this is a static review of the diff.

@github-actions

Copy link
Copy Markdown

No concrete merge-blocking issues found in the PR diff.

I reviewed the new proto definitions, Rust prost crate wiring, workspace inclusion, and Proto CI workflow. I wasn’t able to run the Rust/buf checks locally in this read-only environment, so this is a static review.

@github-actions

Copy link
Copy Markdown

No concrete issues found in the PR changes.

I attempted CARGO_TARGET_DIR=/tmp/orb-pcp-defs-target cargo test -p orb-pcp-defs, but this sandbox’s read-only rustup home prevented running tests locally. Existing Rust CI should still cover the new workspace crate.

Comment thread pcp-defs/go/README.md Outdated
@github-actions

Copy link
Copy Markdown

No concrete issues found in the PR-introduced changes. The new proto definitions, Rust wrapper crate, workspace registration, and Buf CI wiring look consistent with the existing workspace setup. I did not run the Rust tests because the workspace is read-only in this environment.

@github-actions

Copy link
Copy Markdown

No concrete merge-blocking issues found in the PR diff.

I could not run cargo metadata or tests because the environment is read-only and rustup failed creating a temp file under /home/runner/.rustup/tmp. The static review covered the added proto schema, Rust build wrapper, tests, workspace wiring, and proto CI workflow.

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

Adds PCP (Personal Custody Package) protobuf message definitions to the repo, along with a Rust crate (orb-pcp-defs) that compiles and exposes the generated types for use by other workspace crates (and the pcp-package).

Changes:

  • Introduces new pcp.v1 protobuf message schemas for DI iris embeddings and embedding shares.
  • Adds a new Rust workspace member (pcp-defs/rust) that builds the protos via prost-build and includes round-trip tests.
  • Adds Buf configuration + a dedicated GitHub Actions workflow to lint/format the protobuf module, and includes buf in the Nix dev shell.

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
pcp-defs/rust/tests/round_trip.rs Adds prost encode/decode round-trip tests for the new messages.
pcp-defs/rust/src/lib.rs Exposes generated protobuf types under a v1 module and re-exports prost.
pcp-defs/rust/Cargo.toml Defines the new orb-pcp-defs crate and prost/prost-build deps.
pcp-defs/rust/build.rs Builds Rust code from the proto definitions during compilation.
pcp-defs/README.md Documents the purpose of the new pcp-defs component.
pcp-defs/proto/pcp/v1/di_iris_embeddings.proto Defines DiIrisEmbeddings / DiIrisEmbeddingV1 schemas.
pcp-defs/proto/pcp/v1/di_iris_embedding_shares.proto Defines DiIrisEmbeddingShares / DiIrisEmbeddingShareV1 schemas.
pcp-defs/buf.yaml Configures Buf module + lint/breaking rules for protobufs.
nix/shells/development.nix Adds buf to the dev environment for proto workflows.
Cargo.toml Adds pcp-defs/rust to the Rust workspace members list.
Cargo.lock Records the new crate and prost/prost-build dependencies.
.github/workflows/proto-ci.yaml Adds CI to run buf lint and buf format --diff for pcp-defs/**.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pcp-defs/rust/build.rs
Comment on lines +4 to +8
let proto_root = "./proto";
let proto_files = [
"./proto/pcp/v1/di_iris_embeddings.proto",
"./proto/pcp/v1/di_iris_embedding_shares.proto",
];
@github-actions

Copy link
Copy Markdown

No concrete merge-blocking issues found in the PR diff.

Static review covered the new proto schema, Rust prost crate wiring, workspace registration, tests, and the new buf CI workflow. I could not run cargo check locally because rustup attempted to write under read-only /home/runner/.rustup.

@github-actions

Copy link
Copy Markdown

Found one concrete issue:

  • pcp-defs/rust/tests/round_trip.rs:44: DiIrisEmbeddingShareV1 is constructed without the new embedding_version field defined in di_iris_embedding_shares.proto. Since prost generates plain Rust structs with required fields in struct literals, cargo nextest run --workspace --all-features --all-targets should fail to compile this test. Add embedding_version: "...".into(), to the initializer.

I did not find other merge-blocking issues in the PR diff.

@github-actions

Copy link
Copy Markdown

No concrete blocking issues found in the PR diff. The new protobuf definitions, Rust wrapper crate, tests, workspace entry, and proto lint/format CI look consistent with the existing repo patterns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants