specs/001-gpg-event-signing/review.md

Ref: main   Size: 2.3 KiB

# Review Report: Ed25519 Signing for Event Commits

**Branch**: `001-gpg-event-signing` | **Reviewed**: 2026-03-21 | **Model**: Claude Sonnet

## Summary Table

| # | Dimension | Verdict | Notes |
|---|-----------|---------|-------|
| 1 | Spec-Plan Alignment | WARN | `UnknownKey` VerifyStatus not fully resolved — no known-keys registry designed |
| 2 | Plan-Tasks Completeness | WARN | Missing `dirs` crate in deps task; `error.rs` absent from plan file table |
| 3 | Dependency Ordering | PASS | Phase ordering correct; US2→US3 dependency sound |
| 4 | Parallelization Correctness | WARN | T004/T005/T006 all target same file (`signing_test.rs`); T032 must land before T031/T033 |
| 5 | Feasibility & Risk | WARN | Critical: `serde_json::Value` key sorting is version-dependent; `serde(flatten)` + `serde(tag)` is a known serde footgun |
| 6 | Standards Compliance | WARN | `rand` v0.8 wrong dep (should be `rand_core`); base64 variant unspecified |
| 7 | Implementation Readiness | WARN | `UnknownKey` semantics ambiguous; T009 canonical JSON assumption fragile |

## Overall Verdict: READY WITH WARNINGS

## Must Fix (blockers before implementation)

1. **Canonical JSON correctness** (T009): Explicitly verify `serde_json::Map` is `BTreeMap`-backed (no `preserve_order` feature). Add byte-exact test. Do not rely on "Value sorts keys automatically."

2. **`serde(flatten)` + internally-tagged enum compatibility** (T002): Write a prototype test confirming round-trip ser/de works with `Event`'s `#[serde(tag = "type")]` Action. If it fails, pivot to manual JSON construction.

3. **`UnknownKey` semantics** (T011, T025): Decide if in-scope. If out-of-scope, remove variant and reduce `VerifyStatus` to `{Valid, Invalid, Missing}`. If in-scope, add known-keys file design.

## Should Fix (warnings)

4. **`rand` dependency** (T001): Replace `rand = "0.8"` with `rand_core = { version = "0.6", features = ["getrandom"] }` or use ed25519-dalek's re-exported `OsRng`.

5. **Missing `dirs` crate** (T001): Add `dirs = "5"` to Cargo.toml dependencies task.

6. **Base64 encoding variant**: Specify standard base64 with padding. Add to contracts and reference in signing tasks.

7. **Parallel file conflict** (T004/T005/T006): Sequence these or have first task create skeleton.

8. **T032 ordering**: Must land before T031 and T033. Adjust parallelism annotation.