You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
spaced: split pplx-space-spaced-common into spaced-protocol and spaced-client - #1703
Splits pplx-space-spaced-common into its two halves, so neither needs a
baseline exception:
pplx-space-spaced-protocol (crates/l20_domain/spaced/protocol): the
host, guest, bridge-host, host-control and activity protos, the
guest-compatibility rules (SUPPORTED_SPACED_GUEST_VERSIONS), and the guest
operation allowlist.
pplx-space-spaced-client (crates/l40_api/spaced/client): the UDS, vsock
and Firecracker connectors, the UDS and vsock listeners, LazyClient, the
retry policy and spaced_client_config!. All of its users are l40_api
crates or tools, so it is a client they may all depend on.
The old crate re-exported items from pplx-space-spaced-guest-protocol
(proto::bridge_guest, proto::operations, the validators, the wire helpers,
the vsock ports); callers now import them from that crate. Only moves and
import rewrites otherwise. Generated code and proto packages are unchanged:
sbx-mgr-protocol's extern_path now names spaced-guest-protocol's operations module, which the old path re-exported.
Removes the [layers.domain_dependencies] and [layers.model_protocol]
baseline entries for spaced-common, and its [crate_names] entry.
The retry loop's two debug events change log target from pplx_space_spaced_common::retry_policy to pplx_space_spaced_client::retry_policy.
ci/kind-pull-paths.txt selected the transport for the Kind smoke through crates/l20_domain/spaced/**. It now
also lists crates/l40_api/spaced/client/**, and ci/scripts/test-kind-pull-needed.sh checks that a change in either
new crate selects Kind. scripts/move_crates.toml drops the deleted crate's row, as it did for earlier deleted crates.
Validation
ci/scripts/test-kind-pull-needed.sh passes. Its new client case fails without the new path entry.
cargo fmt --check, cargo check --workspace --all-targets, clippy on
the touched crates, ci/scripts/run-rust-checks.sh clippy and isolated-crates, and ci/scripts/run-archcheck.sh against the base
(no required findings).
run-rust-checks.sh tests-unprivileged: every failure is a privileged
guest-process or node path test that also fails without this change on a
host without those privileges.
The generated code for the moved protos is byte-identical to the base's;
sbx-mgr-protocol's differs only in the extern path's crate prefix.
Not run: a Kind smoke.
Severity and risk
Severity: does not apply (restructuring). Risk: build-only. Code that imports pplx_space_spaced_common must switch to the new crates; nothing changes at
run time beyond the two log targets above. Roll back by reverting.
AI assistance: Claude Code made the moves, rewrote the imports, and drafted
this description.
Automatic approval is disabled by the configured approval policy for this pull request. Review comments and requests for changes continue normally.
Commit: 56f3b805038e.
No actionable comments were generated in the recent review.
Walkthrough
The pull request removes spaced-common and distributes its protocol, guest-protocol, transport, and retry APIs across dedicated crates. It adds public APIs to spaced-protocol and spaced-client, then updates workspace declarations, dependencies, consumers, tests, CI paths, and documentation. The summaries report that existing request flows, validation, connection behavior, and test assertions remain unchanged in the migrated call sites.
Merge Risk: Minimal · up to 56f3b
This is a mechanical crate split with updated imports and CI paths. No concrete defect was found, and runtime behavior is reported unchanged apart from two log targets.
Security architecture risk: Low · up to 56f3b
The split preserves the inspected capability checks, transport controls and retry behavior. No introduced security weakness was substantiated. Remaining risk is the integration of renamed dependencies across several consumers; builds and deployment behavior were not independently validated.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — A regression in the relocated shared transport could affect multiple sandbox-facing consumers, including host/guest communication and gateway host-control access. The inspected changes do not substantiate increased tenant reachability or privilege; they preserve existing calls to the moved implementations.
Trust Boundaries and Controls
observed — The candidate path from guest-controlled platform metadata to privileged guest operations retains its existing admission controls. Parsing checks the expected operating system and descriptor bound; host filtering rejects unknown or image-disallowed operation claims before authorization. Reading and comparing the complete allowlist resolved the supplied proof gap without substantiating a new bypass.
Resilience and Maintainability Implications
observed — Connection recovery retains generation-fenced invalidation, and newly constructed operation clients receive fresh capability-observation cells tied to their channels. The unchanged retry loop invokes failure invalidation before exhausting its retry allowance. The host wrapper still distinguishes replay-safe calls from single-dispatch operations, preserving existing failure-containment behavior rather than adding replay authority.
Pre-merge checks: 6
Check name
Status
Explanation
Title check
The title names the crate split, which is the main change. It follows the area-prefix pattern with a lowercase area and colon ('spaced:'). The summary starts with a lowercase verb and includes no trai…
Description Follows The Pr Template
The PR description contains all four required items. Each has real content. 1. ## Summary: Present. It says the change splits pplx-space-spaced-common into pplx-space-spaced-protocol and `pplx…
Bug Fixes Name A Regression Test
The check applies only when the PR fixes a defect in application behavior under crates/, shim/, or ui/. PR #1703 is a crate split, not a bug fix. The PR description says "Severity: does not apply (res…
Weakened Safeguards Are Justified
The PR does not remove, disable, narrow, or weaken a test, assertion, lint, CI guard, or repository guard without an equivalent replacement. Evidence from the base..head diff: - Retry tests: The t…
Agent Tooling Runs No New Commands
The check fails only if this PR adds or changes agent-executed content: plugin hooks, MCP/LSP entries, SKILL.md keys other than name/description, inline shell substitutions (!cmd), or other content …
Public-Safe Content
Checked the PR diff range (128 changed files, 862 added lines) and the PR title and description for public-safety issues. Scans for private key blocks, AWS key IDs, ARNs, GitHub/Slack token formats, p…
Full details: Title check
Explanation
The title names the crate split, which is the main change. It follows the area-prefix pattern with a lowercase area and colon ('spaced:'). The summary starts with a lowercase verb and includes no trailing period, ticket ID, or link. The title does not name the new crate paths or the Kind CI change, which are secondary details.
Full details: Description Follows The Pr Template
Explanation
The PR description contains all four required items. Each has real content. 1. ## Summary: Present. It says the change splits pplx-space-spaced-common into pplx-space-spaced-protocol and pplx-space-spaced-client. It lists what each new crate holds and what callers must change. 2. ## Validation: Present. It names the checks run and their results. These include ci/scripts/test-kind-pull-needed.sh (passes), cargo fmt --check, cargo check, clippy, the isolated-crate and archcheck scripts, and run-rust-checks.sh tests-unprivileged. It also states that a Kind smoke was not run. 3. ## Severity and risk: Present. It gives "Severity: does not apply (restructuring)" and "Risk: build-only", with a rollback note. 4. AI assistance line: Present. The line begins "AI assistance:" and names the tool and its work: "Claude Code made the moves, rewrote the imports, and drafted this description." The check judges presence only. The description meets the template requirements, so the check passes. The description content was assessed as given. No code evidence was needed for this check.
Full details: Bug Fixes Name A Regression Test
Explanation
The check applies only when the PR fixes a defect in application behavior under crates/, shim/, or ui/. PR #1703 is a crate split, not a bug fix. The PR description says "Severity: does not apply (restructuring). Risk: build-only." The changed-file inventory shows moves (R100/R094 renames) and import rewrites across many crates. The retry-policy and status-classification code is moved with identical bodies: status_is_retryable appears as a removal in crates/l20_domain/spaced/common/src/lib.rs and an addition with the same logic in crates/l40_api/spaced/client/src/lib.rs. The only behavior-adjacent edit the author lists is the change of two debug-event log targets, which is not a defect fix. The new tests in crates/l40_api/spaced/client/src/tests.rs cover moved logic, and the Kind-selection regression case is in CI scripts, not application code. Because the PR is not an application bug fix, the check does not apply.
Full details: Weakened Safeguards Are Justified
Explanation
The PR does not remove, disable, narrow, or weaken a test, assertion, lint, CI guard, or repository guard without an equivalent replacement. Evidence from the base..head diff: - Retry tests: The two #[test] functions in the base spaced-common crate (src/tests.rs) are replaced by crates/l40_api/spaced/client/src/tests.rs. That file keeps the same status_is_retryable assertions for UNAVAILABLE, UNKNOWN, DEADLINE_EXCEEDED, and INVALID_ARGUMENT. It also keeps the REQUEST_REFUSED case and the unmarked-UNAVAILABLE case. - Port assertion:assert_eq!(port, ...BRIDGE_GUEST_VSOCK_PORT) in host/server/src/run/tests.rs is kept. Only the constant's crate path changes, to pplx_space_spaced_guest_protocol. - .expect("test version is valid SemVer"): Kept in tests/support/mock_bridge_guest.rs. It moved to the new crate path and is otherwise unchanged. - ci/kind-pull-paths.txt: Only one path is added (crates/l40_api/spaced/client/**). No path is removed. - ci/scripts/test-kind-pull-needed.sh: Two assert_needed true lines are added. No assertion is removed. - ARCHITECTURE.toml and architecture-baseline.toml:spaced-common is removed from the mixed-crate list, the [layers.domain_dependencies] and [layers.model_protocol] exception entries, and [crate_names]. No entry is added. These edits tighten the baseline. The comment edits do not mention reviews, merge risk, or approvals. - AGENTS.md and CLAUDE.md: Only the crate name and path references change. No rule is loosened, and no review-guidance text is added. - No changes to .github/workflows, deny.toml, clippy.toml, .config/nextest.toml, [lints] tables, continue-on-error, or SPACE_TEST_DEPS_OPTIONAL. One residual item: #![allow(clippy::large_enum_variant)] on the bridge_host proto module has no reason attribute. The base has the identical line in spaced/common/src/lib.rs (line 28). The head has it in spaced/protocol/src/lib.rs (line 25). Git shows this as a delete and add only because the file was split. It is a verbatim relocation, not a new or broader suppression, so it does not weaken a safeguard. A strict reading of "adds #[allow] to non-test Rust code" could still flag it. The check's own wording does not require a reason for this case.
Full details: Agent Tooling Runs No New Commands
Explanation
The check fails only if this PR adds or changes agent-executed content: plugin hooks, MCP/LSP entries, SKILL.md keys other than name/description, inline shell substitutions (!cmd), or other content agent tools run. Evidence: - The changed-file list (git diff --name-status between the review base and head) has no plugin manifests (plugin.json, plugin.yaml), no SKILL.md files, no command files, no hooks, and no MCP or LSP configs. The existing .agents/plugins/pplx-space/** and .claude/skills/** files are not in the diff. - The only agent-guideline files touched are crates/l20_domain/spaced/protocol/AGENTS.md (heading rename, 1 line), crates/l20_domain/spaced/protocol/CLAUDE.md (pure rename, 100% similarity, no content change), and crates/l40_api/spaced/AGENTS.md (two path references updated). All changed lines are plain prose. Nothing adds an executable substitution, hook, or server entry. - A search of all added diff lines for ! followed by a backtick, allowed-tools, hooks:, mcpServers, lspServers, or a "command" key found no matches. The backtick and $( hits were Rust macro code (spaced_client_config!) and prose, which the check excludes. Other changes (Rust crates, Cargo manifests, CI scripts, TOML baselines, docs) are not agent plugin or skill content in the sense the check defines. The check does not fail on them.
Full details: Public-Safe Content
Explanation
Checked the PR diff range (128 changed files, 862 added lines) and the PR title and description for public-safety issues. Scans for private key blocks, AWS key IDs, ARNs, GitHub/Slack token formats, password/secret/token assignments, absolute personal paths (/Users/, /home/<name>), IP addresses, internal hostnames, internal tracker or chat links, and 12-digit account-ID-like numbers found no matches. The only "credential" hits are the existing crate name pplx-space-credential-injection, which is a package name and not a secret. The PR description contains no URLs and no production data, incident details, or security vulnerability text. The change is a crate split, with a CI path-list and test-script update.
Recent review info
Automatic approval policy
Commit: 56f3b805038e.
Setting
Effective value
Source
Automatic approvals
Disabled
Override 3
Merge risk
Moderate
Global configuration
Security risk
Not set
Global configuration
Matching overrides (numbered as in the resolved configuration):
Any matching disabled override withholds approval, regardless of risk. Otherwise, the strictest explicitly set limit wins for each score. Limits omitted by all matching overrides inherit the global configuration.
Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 2 remain after this review.
Verified against 97a8f9d91406478674c3302d28d1b07fa68a49ed, diffed against this PR's base branch ac/cc-host (119 files). I checked that the protocol types, guest-compatibility rules and operation allowlist moved to pplx-space-spaced-protocol unchanged, that the connectors, listeners, LazyClient and retry policy moved to pplx-space-spaced-client unchanged, and that consumers were updated consistently. CI is green: Rust tests, clippy, format, isolated-crate builds, and the Kind Firecracker smokes (12 shards plus Direct) all pass. I didn't rerun builds or tests locally.
pplx-space-spaced-common held both the compiled host/control protos and the
UDS/vsock/retry transport, so as one l20_domain crate it counted as a model
that depended on a protocol and on tokio, hyper-util and tokio-vsock.
The protos, the guest-compatibility rules and the guest operation allowlist
move to pplx-space-spaced-protocol (l20_domain/spaced/protocol). The
connectors, listeners, lazy client, retry policy and spaced_client_config!
move to pplx-space-spaced-client (l40_api/spaced/client), where every
client and server that uses them may depend on it. The re-exports of
spaced-guest-protocol items go too: callers import them from that crate.
Generated code and wire packages are unchanged. sbx-mgr-protocol's
extern_path names spaced-guest-protocol's operations module, which the old
path re-exported. The retry loop's two debug events change target from
pplx_space_spaced_common::retry_policy to pplx_space_spaced_client::retry_policy.
Removes the [layers.domain_dependencies] and [layers.model_protocol]
baseline entries for spaced-common, and its [crate_names] entry.
ci/kind-pull-paths.txt selected the transport for the Kind smoke through
crates/l20_domain/spaced/**; it now also lists crates/l40_api/spaced/client/**,
and the selector test checks both new crates.
scripts/move_crates.toml drops the deleted crate's row, as it did for the
earlier deleted crates.
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
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.
the-gigi-pplx
left a comment
Summary
Splits
pplx-space-spaced-commoninto its two halves, so neither needs abaseline exception:
pplx-space-spaced-protocol(crates/l20_domain/spaced/protocol): thehost, guest, bridge-host, host-control and activity protos, the
guest-compatibility rules (
SUPPORTED_SPACED_GUEST_VERSIONS), and the guestoperation allowlist.
pplx-space-spaced-client(crates/l40_api/spaced/client): the UDS, vsockand Firecracker connectors, the UDS and vsock listeners,
LazyClient, theretry policy and
spaced_client_config!. All of its users are l40_apicrates or tools, so it is a client they may all depend on.
The old crate re-exported items from
pplx-space-spaced-guest-protocol(
proto::bridge_guest,proto::operations, the validators, the wire helpers,the vsock ports); callers now import them from that crate. Only moves and
import rewrites otherwise. Generated code and proto packages are unchanged:
sbx-mgr-protocol's
extern_pathnow names spaced-guest-protocol'soperationsmodule, which the old path re-exported.Removes the
[layers.domain_dependencies]and[layers.model_protocol]baseline entries for spaced-common, and its
[crate_names]entry.The retry loop's two debug events change log target from
pplx_space_spaced_common::retry_policytopplx_space_spaced_client::retry_policy.ci/kind-pull-paths.txtselected the transport for the Kind smoke throughcrates/l20_domain/spaced/**. It nowalso lists
crates/l40_api/spaced/client/**, andci/scripts/test-kind-pull-needed.shchecks that a change in eithernew crate selects Kind.
scripts/move_crates.tomldrops the deleted crate's row, as it did for earlier deleted crates.Validation
ci/scripts/test-kind-pull-needed.shpasses. Its new client case fails without the new path entry.cargo fmt --check,cargo check --workspace --all-targets, clippy onthe touched crates,
ci/scripts/run-rust-checks.sh clippyandisolated-crates, andci/scripts/run-archcheck.shagainst the base(no required findings).
run-rust-checks.sh tests-unprivileged: every failure is a privilegedguest-process or node path test that also fails without this change on a
host without those privileges.
The generated code for the moved protos is byte-identical to the base's;
sbx-mgr-protocol's differs only in the extern path's crate prefix.
Not run: a Kind smoke.
Severity and risk
Severity: does not apply (restructuring). Risk: build-only. Code that imports
pplx_space_spaced_commonmust switch to the new crates; nothing changes atrun time beyond the two log targets above. Roll back by reverting.
AI assistance: Claude Code made the moves, rewrote the imports, and drafted
this description.