feat(broker): serde mirrors + golden fixtures for fleet node-control wire protocol (Phase 0)#1106
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 5 minutes and 50 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR introduces the ChangesFleet Wire Protocol Implementation & Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/broker/tests/fleet_wire_fixtures.rs`:
- Around line 31-34: The test currently only asserts fixture_paths is non-empty;
instead collect the observed fixture types (e.g., by mapping fixture_paths to
their type identifier or file stem) and compare against a canonical set of
required types (define a CANONICAL_FIXTURE_TYPES constant or vec) and assert
coverage equality (or that canonical ⊆ observed) so any missing fixture type
fails; update the assertions around fixture_paths and the round-trip checks (in
fleet_wire_fixtures.rs where fixture_paths and FIXTURE_DIR are used) to include
this seen-type coverage assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db9aebc0-e845-4a05-abcc-14a6f4354960
📒 Files selected for processing (14)
crates/broker/src/fleet_wire.rscrates/broker/src/lib.rscrates/broker/tests/fixtures/fleet-wire/action.invoke.jsoncrates/broker/tests/fixtures/fleet-wire/action.result.jsoncrates/broker/tests/fixtures/fleet-wire/agent.deregister.jsoncrates/broker/tests/fixtures/fleet-wire/agent.register.jsoncrates/broker/tests/fixtures/fleet-wire/deliver.jsoncrates/broker/tests/fixtures/fleet-wire/delivery.ack.jsoncrates/broker/tests/fixtures/fleet-wire/inventory.sync.jsoncrates/broker/tests/fixtures/fleet-wire/node.deregister.jsoncrates/broker/tests/fixtures/fleet-wire/node.heartbeat.jsoncrates/broker/tests/fixtures/fleet-wire/node.register.jsoncrates/broker/tests/fixtures/fleet-wire/ping.jsoncrates/broker/tests/fleet_wire_fixtures.rs
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
NO-GO. I attempted to submit this as a request-changes review, but GitHub rejected it with: Review Can not request changes on your own pull request. Findings below.
Validation: clean worktree at /tmp/review-Rev0C1; compared crates/broker/tests/fixtures/fleet-wire against the current relaycast#191 head (b3640f5, v2 stable fixtures). Local validation passed: cargo fmt -- --check, cargo build, env -u RELAY_API_KEY cargo test, env -u RELAY_API_KEY cargo clippy -- -D warnings, env -u RELAY_API_KEY cargo test --release, npm run build:rust.
-
[blocker]
crates/broker/src/fleet_wire.rs:87,crates/broker/src/fleet_wire.rs:153,crates/broker/src/fleet_wire.rs:241,crates/broker/src/fleet_wire.rs:278still serializeinvocation_idfields as camelCaseinvocationId, and the mirrored fixtures carry that casing atcrates/broker/tests/fixtures/fleet-wire/action.invoke.json:4,crates/broker/tests/fixtures/fleet-wire/agent.register.json:5,crates/broker/tests/fixtures/fleet-wire/inventory.sync.json:8, andcrates/broker/tests/fixtures/fleet-wire/action.result.json:4. The canonical relaycast v2 schemas/fixtures use strict snake_caseinvocation_id; relaycast will reject these broker-emitted messages as missinginvocation_idplus unknowninvocationId. Change the serderenamevalues toinvocation_id, remove the camelCase alias unless backward compatibility is explicitly intended and tested, and re-mirror the v2 fixtures. -
[major]
crates/broker/src/fleet_wire.rs:110modelsagent.deregisteras only{ v }, andcrates/broker/tests/fixtures/fleet-wire/agent.deregister.json:3mirrors that shape. The canonical v2 fixture and schema requirenameonagent.deregister, so the broker currently cannot emit the required identity for deregistration. Addpub name: StringtoAgentDeregisterand update the fixture to include the deregistered agent name. -
[major]
crates/broker/tests/fleet_wire_fixtures.rs:31only asserts the fixture directory is non-empty and then self-round-trips whatever local files happen to exist. That missed the v2 fixture inventory: canonical relaycast now has 12 fixtures, includingaction.result.output.jsonandaction.result.error.json, while this PR has one staleaction.result.json. Add an explicit expected fixture list matching relaycast#191 and mirror both action-result variants so missing/renamed fixtures fail the Rust test instead of silently passing. -
[minor]
crates/broker/src/fleet_wire.rs:63andcrates/broker/src/fleet_wire.rs:70do not fully mirror relaycast's strict schema constraints.resume_cursoris required-but-nullable in relaycast, but a plainOption<String>accepts the field being omitted;loadisz.number().finite().nonnegative()in relaycast, but Rust accepts negativef64values. If this module is the serde contract check, add presence/value validation tests and custom deserializers/newtypes so Rust rejects the same invalid wire input as the zod schemas.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Verdict: NO-GO. GitHub rejected a formal request-changes review from this authenticated identity because it owns the PR, so this is posted as a blocking review comment instead. The round-1 fixture fixes are present, and the 12 broker fixtures match relaycast feat/fleet-wire-types byte-for-byte, but the Rust mirror still has schema-parity gaps that the happy-path fixtures do not catch.
-
[major]
crates/broker/src/fleet_wire.rs:63makesnode.register.resume_cursoranOption<String>. In serde, an absentOptionfield deserializes asNone, so Rust accepts anode.registermessage that omitsresume_cursor. The zod source contract isresume_cursor: z.string().nullable(), meaning the field is required and only the value may be null. This breaks the Phase 0 mirror contract and lets Rust accept a shape Relaycast rejects. Change this to a required-nullable deserializer/wrapper that distinguishes missing from null, and add a regression test for missingresume_cursor. -
[major]
crates/broker/src/fleet_wire.rs:70modelsnode.heartbeat.loadas a rawf64. The relaycast zod schema isz.number().finite().nonnegative(), but the Rust mirror currently accepts negative loads when deserializing and can construct invalid outgoing heartbeats without any validation. Add a validated nonnegative finite type or manual serde forNodeHeartbeat, and cover negative and non-finite load cases in tests.
Validation run locally: cargo fmt -- --check, cargo build, env -u RELAY_API_KEY cargo test, npm run build:rust, and env -u RELAY_API_KEY cargo clippy -- -D warnings all pass.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Verdict: GO — clean.
Fresh round-3 pass over the full diff found no blocker/major issues. Checked against fleet-implementation-plan.md Phase 0 item 4 and the current relaycast#191 head (8438b47). The Rust fixtures are byte-for-byte identical to packages/types/fixtures/fleet-wire, and the serde mirror matches the current Zod contract.
Prior findings verified fixed:
- r1
invocationIdcasing: fixed; wire fields and fixtures useinvocation_id. - r1
agent.deregister.name: fixed. - r1 stale/vacuous fixture inventory: fixed; the test asserts the exact 12 fixture filenames and observed message types.
- r2
loadfinite+nonnegative: fixed on deserialize and serialize. - r2
resume_cursorexplicit-null serialization: fixed; Rust serializesNoneasnull, and current relaycast schema explicitly accepts absent or null.
Validation run from fresh worktree /tmp/review-Rev0C3:
cargo fmt -- --check: passcargo build: passenv -u RELAY_API_KEY cargo test: one default-parallel run failed in unrelatedsnippets::tests::configure_agent_relay_mcp_public_reads_env_fallback; the failing test passes isolated and the full debug suite passes with-- --test-threads=1env -u RELAY_API_KEY cargo test -- --test-threads=1: passenv -u RELAY_API_KEY cargo clippy -- -D warnings: passenv -u RELAY_API_KEY cargo test --release: passnpm run build:rust: pass
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Round 4 v3 parity review: NO-GO. I attempted --request-changes, but GitHub rejected it because the authenticated account owns this PR.
- [major] crates/broker/src/fleet_wire.rs:403 / crates/broker/tests/fixtures/fleet-wire/reply.json:6:
agent.registersuccess replies are not typed as carryingtoken. The fixture shows the required payload includesdata.token, but the serde mirror stores successful reply payloads as plainserde_json::Value, so a successfulreplyframe forreq_agent_register_001withdata: {"agent_id":"agt_1","name":"codex-builder-1"}deserializes successfully. I confirmed that with a temporary integration probe. That leaves the Phase 0 token-authority path without a contract-enforced token and lets broker-side consumers accept a successful registration reply that cannot seed the spawned harness token. Add a typed serde struct for the agent-register reply data requiring at leastagent_id,name, andtokenwithinvocation_idandsession_refoptional as in the fixture, plus tests that deserializereply.jsondata through that type and reject missing/non-stringtoken. If the top-levelReplyparser is meant to be the contract boundary, add enough response discrimination/correlation to reject agent-register replies whose data lackstoken.
Validation run: diff -ru between the relaycast and relay fixture directories produced no output; npm ci; cargo test fleet_wire; cargo build; cargo test; npm run build; npm run test. Existing hostile checks reject wrong ok, unsupported v, explicit null optionals, and ambiguous action.result; the missing-token reply is the accepted hostile case above.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Round 5 v3.1 parity review: GO. No findings. Validated relay tip 7fd1765 with env -u RELAY_API_KEY cargo build -p agent-relay-broker, env -u RELAY_API_KEY cargo test -p agent-relay-broker fleet_wire, and diff -ru fixture parity against relaycast. reply.agent_register.json is present and byte-identical across repos; Rust typed validation rejects token-less reply.data.
Reconcile the fleet control plane onto main after the #1105 (harness-driver local protocol) and #1106 (broker serde mirrors + golden wire fixtures) foundations merged. The frozen fleet wire contract (fleet_wire.rs serde mirrors and tests/fixtures/fleet-wire/*.json) is taken byte-identical from main; this commit adds only the control-plane delta on top of it. - node_control: client that drives the harness-driver sidecar over the local protocol (node/handler register, deregister, broker handler invocation, handler results, handler-attributed spawns, inventory resync on reconnect). - runtime/fleet: wires fleet control into the broker runtime — registers nodes and handlers, dispatches broker handler invocations, attributes spawns. - Harden node/handler registration timing to close registration races. - protocol/listen_api/runtime wiring and the futures-util + tokio-tungstenite deps the sidecar transport requires. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reconcile the fleet control plane onto main after the #1105 (harness-driver local protocol) and #1106 (broker serde mirrors + golden wire fixtures) foundations merged. The frozen fleet wire contract (fleet_wire.rs serde mirrors and tests/fixtures/fleet-wire/*.json) is taken byte-identical from main; this commit adds only the control-plane delta on top of it. - node_control: client that drives the harness-driver sidecar over the local protocol (node/handler register, deregister, broker handler invocation, handler results, handler-attributed spawns, inventory resync on reconnect). - runtime/fleet: wires fleet control into the broker runtime — registers nodes and handlers, dispatches broker handler invocations, attributes spawns. - Harden node/handler registration timing to close registration races. - protocol/listen_api/runtime wiring and the futures-util + tokio-tungstenite deps the sidecar transport requires. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+spawn Adds the TypeScript fleet node layer on top of the merged broker control plane (#1107), wire serde mirrors (#1106), and harness-driver local protocol (#1105). - @agent-relay/fleet: defineNode/action/spawn/onMessage declare a node's typed capabilities and channel-message triggers. Trigger match regexes must be flag-free — a flagged regex (e.g. /ship/i) is rejected at defineNode rather than silently matched case-sensitively. - agent-relay fleet serve|nodes|status runs a fleet node sidecar and inspects registered nodes; broker MCP surface adds query_nodes and spawn tools (with MCP legacy resources restored) and clean node deregistration on shutdown. - SDK messaging gains nodes (list/get) and triggers (list/create/update/ delete) facades over the relaycast backend; bump @relaycast/sdk to ^3.1.1 for the node/trigger API. Reconciled onto main: the broker, wire fixtures, and harness-driver local protocol are taken canonical from main (#1105/#1106/#1107); this commit is purely the TS SDK/CLI/MCP delta adapted to compile and test against them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/spawn + §5 resource cleanup (Phase 4) (#1108) * feat(fleet): @agent-relay/fleet SDK, fleet serve CLI, MCP query_nodes+spawn Adds the TypeScript fleet node layer on top of the merged broker control plane (#1107), wire serde mirrors (#1106), and harness-driver local protocol (#1105). - @agent-relay/fleet: defineNode/action/spawn/onMessage declare a node's typed capabilities and channel-message triggers. Trigger match regexes must be flag-free — a flagged regex (e.g. /ship/i) is rejected at defineNode rather than silently matched case-sensitively. - agent-relay fleet serve|nodes|status runs a fleet node sidecar and inspects registered nodes; broker MCP surface adds query_nodes and spawn tools (with MCP legacy resources restored) and clean node deregistration on shutdown. - SDK messaging gains nodes (list/get) and triggers (list/create/update/ delete) facades over the relaycast backend; bump @relaycast/sdk to ^3.1.1 for the node/trigger API. Reconciled onto main: the broker, wire fixtures, and harness-driver local protocol are taken canonical from main (#1105/#1106/#1107); this commit is purely the TS SDK/CLI/MCP delta adapted to compile and test against them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(fleet): namespace-import @agent-relay/fleet & harnesses for bun standalone build The standalone bun --compile bundle (Standalone macOS Smoke) failed with "No matching export in <pkg>/dist/index.d.ts" for value imports from @agent-relay/fleet (and transitively @agent-relay/harnesses) — bun's bundler mis-validates named value imports from these workspace packages against their generated .d.ts, even though both the .js and .d.ts export the symbols. sdk and harness-driver are unaffected; this surfaced only now because fleet-sidecar is the first code to pull fleet/harnesses into the standalone bundle. Switch the affected value imports to a namespace import + destructure (call sites unchanged; type-only imports kept as `import type`). Verified the standalone bun build now bundles (1045 modules) and produces the binary. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary:
Context:
Validation: