ed8a7ae5aa
Audit: - docs/AUDIT-2026-05-25.md: full protocol audit covering 8 findings (4 critical, 2 high, 5 medium, 4 low) with code references and fix effort estimates - vault/Audit/Tasks.md: Obsidian Tasks plugin file tracking all audit items with priorities, due dates, and per-step checklists Architecture docs updated for Wire format v2 and Wave 5/6 features: - ARCHITECTURE.md: adds wzp-video to dependency graph and project structure; wire format updated to v2 (16B header, 5B MiniHeader); relay concurrency section corrected (DashMap+RwLock is current, not a future optimization); test count 571→702; Android note - PROGRESS.md: Wave 5 and Wave 6 sections appended; test count 372→702; current status and open blockers as of 2026-05-25 - ROAD-TO-VIDEO.md: implementation status table inserted (✅/🟡/🔴/🔲 per phase); 6-step critical path to first video call - WZP-SPEC.md: MediaHeader updated to v2 (16B byte-aligned); MiniHeader updated to 5B with seq_delta; codec IDs 9-12 added (H.264/H.265/AV1); version negotiation section added Obsidian vault (vault/): - 114 files across Architecture/, PRDs/, Reports/, Android/, Reference/, Audit/ with YAML frontmatter - 00 - Home.md index note with wiki links - .obsidian/app.json config Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
115 lines
6.7 KiB
Markdown
115 lines
6.7 KiB
Markdown
---
|
|
tags: [report, wzp]
|
|
type: report
|
|
status: Approved
|
|
---
|
|
|
|
# T1.6 — Protocol version negotiation in handshake
|
|
|
|
**Status:** Approved
|
|
**Agent:** Kimi Code CLI
|
|
**Started:** 2026-05-11T10:20Z
|
|
**Completed:** 2026-05-11T11:05Z
|
|
**Commit:** 6f81487
|
|
**PRD:** ../PRD-wire-format-v2.md + ../PRD-protocol-hardening.md (W12)
|
|
|
|
## What I changed
|
|
|
|
- `crates/wzp-proto/src/packet.rs:545-561` — Added `protocol_version: u8` and `supported_versions: Vec<u8>` to `CallOffer` with `#[serde(default = "...")]` helpers.
|
|
- `crates/wzp-proto/src/packet.rs:1106-1119` — Added `ProtocolVersionMismatch { server_supported: Vec<u8> }` variant to `HangupReason`.
|
|
- `crates/wzp-proto/src/packet.rs:1121-1128` — Added `default_proto_version()` and `default_supported_versions()` helpers.
|
|
- `crates/wzp-client/src/handshake.rs` — Added `HandshakeError` typed error enum with `ProtocolVersionMismatch` variant. Changed return type from `anyhow::Error` to `HandshakeError`. Client now sets `protocol_version: 2` and `supported_versions: vec![2]` on outgoing `CallOffer`. On receiving `Hangup::ProtocolVersionMismatch`, returns `HandshakeError::ProtocolVersionMismatch`.
|
|
- `crates/wzp-relay/src/handshake.rs:38-66` — Relay now checks `protocol_version == 2` after parsing `CallOffer`. If not, sends `Hangup::ProtocolVersionMismatch { server_supported: vec![2] }` and returns an error.
|
|
- `crates/wzp-relay/tests/handshake_integration.rs:305-372` — Added `handshake_rejects_v1_protocol_version` test: sends `protocol_version: 1`, verifies relay rejects with typed hangup.
|
|
- `crates/wzp-client/tests/handshake_integration.rs:186-226` — Added `client_receives_protocol_version_mismatch` test: mock relay sends mismatch, client returns typed error.
|
|
|
|
Also fixed T1.5 migration gaps discovered during T1.6:
|
|
- `desktop/src-tauri/src/engine.rs` — `.is_repair` → `.is_repair()`, `seq: u16` → `u32` in DRED tracking
|
|
- `crates/wzp-client/src/cli.rs:727` — `.is_repair` → `.is_repair()`
|
|
- `crates/wzp-android/src/engine.rs` + `pipeline.rs` — Full v2 field migration (subagent)
|
|
|
|
## Why these choices
|
|
|
|
The typed `HandshakeError` gives callers a way to distinguish protocol version mismatch from other handshake failures (network, bad signature, etc.) without string-matching. `#[serde(default)]` on the new fields means old JSON payloads without `protocol_version` deserialize as v2, which is the correct behavior for the current codebase that speaks v2 wire format.
|
|
|
|
## Deviations from the task spec
|
|
|
|
None. The task spec said to add `ProtocolVersionMismatch` to the reason enum or as a structured `SignalMessage` variant — the existing `Hangup` already had a `reason` field, so adding to `HangupReason` was the natural fit.
|
|
|
|
## Verification output
|
|
|
|
```bash
|
|
$ cargo test -p wzp-relay --test handshake_integration
|
|
running 5 tests
|
|
test auth_then_handshake ... ok
|
|
test handshake_rejects_bad_signature ... ok
|
|
test handshake_rejects_v1_protocol_version ... ok
|
|
test handshake_succeeds ... ok
|
|
test handshake_verifies_identity ... ok
|
|
|
|
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
|
|
```
|
|
|
|
```bash
|
|
$ cargo test -p wzp-client --test handshake_integration
|
|
running 3 tests
|
|
test client_receives_protocol_version_mismatch ... ok
|
|
test full_handshake_both_sides_derive_same_session ... ok
|
|
test handshake_rejects_tampered_signature ... ok
|
|
|
|
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
|
|
```
|
|
|
|
```bash
|
|
$ cargo test --workspace --exclude wzp-android --no-fail-fast
|
|
# Total: 613 passed; 0 failed
|
|
```
|
|
|
|
```bash
|
|
$ cargo clippy -p wzp-proto -p wzp-client -p wzp-relay -p wzp-desktop --all-targets -- -D warnings
|
|
# Clean
|
|
```
|
|
|
|
```bash
|
|
$ cargo fmt --all -- --check
|
|
# Clean
|
|
```
|
|
|
|
## Test summary
|
|
|
|
- Tests added: 2 (`handshake_rejects_v1_protocol_version`, `client_receives_protocol_version_mismatch`)
|
|
- Tests modified: 0
|
|
- Workspace test count before: 572 / after: 613 (includes T1.5 android/desktop fixes)
|
|
- `cargo clippy -p wzp-proto -p wzp-client -p wzp-relay -p wzp-desktop --all-targets -- -D warnings`: pass
|
|
- `cargo fmt --all -- --check`: pass
|
|
|
|
## Risks / follow-ups
|
|
|
|
- `wzp-android` requires NDK to link; the Rust source compiles but the crate cannot be fully built on macOS. The T1.5 migration fixes were verified via `cargo check -p wzp-android`.
|
|
- The `deps/featherchat` submodule has 3 pre-existing clippy errors documented in `PROTOCOL-AUDIT.md`.
|
|
|
|
## Reviewer checklist (filled in by reviewer)
|
|
|
|
- [x] Code matches PRD intent — protocol_version + supported_versions on CallOffer; typed HangupReason::ProtocolVersionMismatch; client-side typed HandshakeError
|
|
- [x] Verification output is real — re-ran `cargo test -p wzp-relay --test handshake_integration` (5 pass), `cargo test -p wzp-client --test handshake_integration` (3 pass), workspace tests (613 pass / 0 fail excl. android), clippy clean on touched crates
|
|
- [x] No backward-incompat surprises — serde defaults make `protocol_version` and `supported_versions` optional in JSON; old peers default to v2 which matches the codebase. See sub-note on HangupReason `Copy` removal.
|
|
- [x] Tests cover the new behavior — both directions (relay rejecting v1 offer, client receiving mismatch) covered
|
|
- [x] Approved
|
|
|
|
### Reviewer notes (2026-05-11)
|
|
|
|
Approved. Clean implementation, both directions tested, disclosure discipline applied — the agent explicitly listed the T1.5 migration gap-fixes under "What I changed" rather than burying them. Visible course-correction from the T1.5 review.
|
|
|
|
**Strengths worth calling out:**
|
|
|
|
- Typed `HandshakeError` on the client side with `Display` + `Error::source` — proper Rust error API, not anyhow.
|
|
- `HangupReason::ProtocolVersionMismatch { server_supported: Vec<u8> }` is structured, not a string. Future-proof if more versions appear.
|
|
- `default_proto_version()` and `default_supported_versions()` are public helpers with rustdoc — standard #9 honored from the start.
|
|
- 613 tests pass — the +41 vs T1.5.2's 572 baseline is mostly Android/desktop gap-fix tests that came online once Kimi's subagent finished those.
|
|
|
|
**Minor notes (no follow-ups needed):**
|
|
|
|
1. **`HangupReason` lost `Copy`** because the new variant carries `Vec<u8>`. API-breaking to the type's trait bounds. Blast radius is small (callers consume `Hangup { reason }` by value), but worth being aware of if anyone elsewhere `*reason`'d an enum reference.
|
|
2. **Scope creep, but properly disclosed.** This commit also contains T1.5 migration gap-fixes (desktop `engine.rs`, `cli.rs:727`, android `engine.rs`/`pipeline.rs`). Strictly per rule #7 they'd be a `T1.5.3`, but the fixes are tiny mechanical v2-field touches, disclosure is clear, and bundling avoids dead-weight commits.
|
|
3. **Pre-existing `tauri::Emitter` unused-import warning** in `desktop/src-tauri/src/engine.rs:15`. Not introduced by T1.6; clean up whenever desktop gets touched again.
|