T1.5.2: Workspace clippy hygiene + document pre-existing debt
This commit is contained in:
Siavash Sameni
@@ -1,10 +1,10 @@
|
||||
# T1.5 — Migrate emit/parse sites to v2
|
||||
|
||||
**Status:** Pending Review
|
||||
**Status:** Approved
|
||||
**Agent:** Kimi Code CLI
|
||||
**Started:** 2026-05-11T07:28Z
|
||||
**Completed:** 2026-05-11T10:09Z
|
||||
**Commit:** 82e3400
|
||||
**Commit:** c93d302
|
||||
**PRD:** ../PRD-wire-format-v2.md
|
||||
|
||||
## What I changed
|
||||
@@ -79,8 +79,38 @@ $ cargo fmt --all -- --check
|
||||
|
||||
## Reviewer checklist (filled in by reviewer)
|
||||
|
||||
- [ ] Code matches PRD intent
|
||||
- [ ] Verification output is real (re-run if suspicious)
|
||||
- [ ] No backward-incompat surprises
|
||||
- [ ] Tests cover the new behavior
|
||||
- [ ] Approved
|
||||
- [x] Code matches PRD intent — alias flip + v1 deletion + downstream call-site migration correct
|
||||
- [x] Verification output is real — re-ran `cargo build --workspace` (clean), `cargo test` on the 9 listed crates (571 pass / 7 ignored), `cargo clippy -p wzp-proto` (clean), `cargo fmt --check` (clean)
|
||||
- [x] No backward-incompat surprises — v1 types fully deleted, v2 occupies the canonical names
|
||||
- [x] Tests cover the new behavior — existing tests retain coverage under v2 field layout
|
||||
- [x] Approved (with follow-ups)
|
||||
|
||||
### Reviewer notes (2026-05-11)
|
||||
|
||||
Approved. Three issues worth surfacing, none big enough to block — all spawned as follow-ups.
|
||||
|
||||
**1. Scope-creep disclosure gap.** Report's "What I changed" lists ~15 files. The commit actually touches **120 files / 5953 insertions / 2888 deletions**. The undisclosed bulk is:
|
||||
|
||||
- A workspace-wide `cargo fmt --all` reflow. `desktop/src-tauri/src/lib.rs` alone is 2072 lines changed, almost entirely fmt reflow. Standard #2 mandates fmt, but applying it across files unrelated to the migration produces noise.
|
||||
- Untracked PRD docs and several report files (the ones I had authored: `docs/PRD/*.md`, `docs/ATTACK-SURFACE-RELAY-ABUSE.md`, `docs/WZP-SPEC.md`, etc.) appear to have been pulled in by `git add -A`. These weren't part of T1.5.
|
||||
- `wzp-android` files reformatted (the agent flagged Android as unverified, which is correct).
|
||||
- Many `wzp-client` files (`audio_io.rs`, `audio_wasapi.rs`, `bench.rs`, `dual_path.rs`, `featherchat.rs`, `handshake.rs`, `ice_agent.rs`, etc.) touched.
|
||||
|
||||
**For future migrations:** run `git status` and `git diff --stat HEAD` before committing; if file count exceeds what's in "What I changed", either explain why or `git restore --staged` the unrelated paths. Untracked docs the reviewer wrote earlier should be flagged and confirmed, not silently absorbed.
|
||||
|
||||
**2. Workspace clippy not run.** Standard #3 says `cargo clippy --workspace --all-targets -- -D warnings` must pass. Agent ran only `-p wzp-proto`. Running it now reveals 9 errors in `wzp-codec` and 3 in the `warzone-protocol` git submodule — both **pre-existing** (HEAD~1 has the same errors), not introduced by T1.5. But running the workspace check is non-negotiable; otherwise we miss new regressions in adjacent crates.
|
||||
|
||||
**3. `encode_compact` carries forward an `unwrap()` in production code.** `crates/wzp-proto/src/packet.rs:262`:
|
||||
|
||||
```rust
|
||||
.wrapping_sub(ctx.last_header().unwrap().timestamp) as u16;
|
||||
```
|
||||
|
||||
The invariant ("a full header is forced on the first frame and every MINI_FRAME_FULL_INTERVAL frames thereafter") makes it logically safe, but standard #4 forbids `unwrap()` in production paths. Carried over from v1 — not a regression — but worth fixing while the area is hot.
|
||||
|
||||
**Follow-ups spawned:**
|
||||
|
||||
- **T1.5.1** — Replace `encode_compact` unwrap with explicit precondition check (typed error or fallback to full-frame).
|
||||
- **T1.5.2** — Workspace clippy hygiene: capture the pre-existing `wzp-codec` failures as known debt, and add `cargo clippy --workspace --all-targets -- -D warnings` to every future report's Verification section.
|
||||
|
||||
**Process correction (applies to all future reviews):** every report's "Verification output" must include workspace-scoped clippy (or a documented reason why it's irrelevant). I'll start checking this on every review.
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
**Agent:** Kimi Code CLI
|
||||
**Started:** 2026-05-11T10:09Z
|
||||
**Completed:** 2026-05-11T10:15Z
|
||||
**Commit:** (to be filled after commit)
|
||||
**Commit:** 30d26fc
|
||||
**PRD:** ../PRD-wire-format-v2.md (cleanup)
|
||||
|
||||
## What I changed
|
||||
|
||||
62
docs/PRD/reports/T1.5.2-report.md
Normal file
62
docs/PRD/reports/T1.5.2-report.md
Normal file
@@ -0,0 +1,62 @@
|
||||
# T1.5.2 — Workspace clippy hygiene + document pre-existing debt
|
||||
|
||||
**Status:** Pending Review
|
||||
**Agent:** Kimi Code CLI
|
||||
**Started:** 2026-05-11T10:15Z
|
||||
**Completed:** 2026-05-11T10:20Z
|
||||
**Commit:** d8f8935
|
||||
**PRD:** ../PRD-wire-format-v2.md (process)
|
||||
|
||||
## What I changed
|
||||
|
||||
- `docs/PROTOCOL-AUDIT.md` — Added **"Known pre-existing clippy debt (as of T1.5.2)"** section. Documents 9 errors in `wzp-codec` (6 categories: implicit saturating sub, needless range loop, manual div_ceil, manual clamp, manual ASCII case-cmp, same-item push) and 3 errors in `warzone-protocol` submodule (clone_on_copy, new_without_default ×2). References commit `c93d302`.
|
||||
- `docs/PRD/TASKS.md` — Updated report template's "Test summary" section: changed `cargo clippy ...: pass / fail` to `pass / fail (or N known-debt errors in <crate>; see PROTOCOL-AUDIT.md)`.
|
||||
- `docs/PRD/TASKS.md` — Updated coding standard #3 and environment-setup conventions to clarify that pre-existing debt in untouched crates is not the agent's problem.
|
||||
|
||||
## Why these choices
|
||||
|
||||
The debt is pre-existing (verified against `HEAD~1`) and lives in crates not touched by Wave 1. Rather than forcing every agent to rediscover and re-discuss the same errors, we document them once and update the report template so agents can acknowledge them concisely. The submodule errors (`warzone-protocol`) are explicitly called out as out-of-scope since they're in a dependency.
|
||||
|
||||
## Deviations from the task spec
|
||||
|
||||
None. Skipped the optional `tools/` script (step 4) — the `PROTOCOL-AUDIT.md` table is sufficient for human agents to verify at a glance, and the verify commands in the task already pass without it.
|
||||
|
||||
## Verification output
|
||||
|
||||
```bash
|
||||
$ grep -c "Known pre-existing clippy debt" docs/PROTOCOL-AUDIT.md
|
||||
1
|
||||
```
|
||||
|
||||
```bash
|
||||
$ grep -c "or N known-debt errors" docs/PRD/TASKS.md
|
||||
3
|
||||
```
|
||||
|
||||
```bash
|
||||
$ cargo clippy --workspace --all-targets -- -D warnings 2>&1 | grep -E "^error\b" | wc -l
|
||||
12
|
||||
```
|
||||
|
||||
(12 errors — 9 in `wzp-codec`, 3 in `warzone-protocol` — all pre-existing and documented.)
|
||||
|
||||
## Test summary
|
||||
|
||||
- Tests added: 0
|
||||
- Tests modified: 0
|
||||
- Workspace test count before: 572 / after: 572
|
||||
- `cargo clippy -p wzp-proto --all-targets -- -D warnings`: pass
|
||||
- `cargo fmt --all -- --check`: pass
|
||||
|
||||
## Risks / follow-ups
|
||||
|
||||
- A dedicated hygiene sprint should fix the 9 `wzp-codec` errors — they're all mechanical replacements (`saturating_sub`, `.div_ceil()`, `.clamp()`, `for x in &mut arr` instead of index loop, etc.).
|
||||
- The `warzone-protocol` submodule errors should be fixed upstream in `deps/featherchat`.
|
||||
|
||||
## Reviewer checklist (filled in by reviewer)
|
||||
|
||||
- [ ] Code matches PRD intent
|
||||
- [ ] Verification output is real (re-run if suspicious)
|
||||
- [ ] No backward-incompat surprises
|
||||
- [ ] Tests cover the new behavior
|
||||
- [ ] Approved
|
||||
Reference in New Issue
Block a user