specs/003-key-trust-allowlist/.analyze-done

Ref: main   Size: 4.1 KiB

# Cross-Artifact Consistency Analysis
## Date: 2026-03-21

## Findings

### MEDIUM: Plan references SignatureVerificationResult in check_trust but tasks describe different signature
- Plan (Architecture section): "For each VerifyStatus::Valid result, checks whether result.pubkey is in the trusted set"
- Task T014: "takes &[SignatureVerificationResult] and &TrustPolicy"
- Actual code: SignatureVerificationResult has `pubkey: Option<String>` -- this matches.
- However, the plan's VerifyStatus enum snippet omits the existing `Untrusted` variant description fields. Minor, no real inconsistency since the struct is separate from the enum.
- **Verdict**: Consistent. No action needed.

### MEDIUM: Task T009 implements remove_trusted_key() but it's labeled as US1
- T009 is in Phase 2 (US1 - Add Trusted Keys) but implements `remove_trusted_key()` which is a US3 concern.
- This is intentional for code locality (both save and remove operate on the same file) but creates a traceability gap: US3 remove tests (T017) depend on implementation done in US1's phase.
- **Verdict**: Acceptable pragmatic grouping but slightly misleading labeling.

### LOW: FR-002a numbering is non-standard
- Spec uses FR-002, FR-002a, FR-002b which breaks sequential numbering. FR-002a and FR-002b are sub-requirements of FR-002 but are really independent requirements about sync behavior (not about `key add`).
- **Verdict**: Cosmetic. No functional impact.

### LOW: Spec says "reject the entire ref" (FR-002a) and tasks confirm whole-ref rejection, but current code already rejects on Invalid/Missing
- The current reconcile_refs filters on `r.status != VerifyStatus::Valid`. Adding `Untrusted` variant means it would automatically be caught by this existing filter.
- Plan says trust check happens AFTER verify_ref in reconcile_refs, but the existing rejection logic only checks VerifyStatus. The plan's approach of modifying the results vector before the existing check would work, but T015 describes adding trust checking "after verify_ref succeeds" -- the integration point needs care.
- **Verdict**: The plan's architecture (post-verification filter that mutates results) is sound but T015 description could be more explicit about WHERE in reconcile_refs the check is inserted (before the existing failure-filter loop).

### LOW: No task for updating existing match arms when adding Untrusted variant (T001)
- T001 says "update any existing match arms that need a wildcard or explicit handling." The current code in sync.rs filters on `r.status != VerifyStatus::Valid` using PartialEq, not pattern matching, so no match arms need updating there. But verify_ref itself has match arms on VerifyStatus (lines 245-264) that would need an `Untrusted` arm or wildcard.
- **Verdict**: T001's description covers this ("update any existing match arms") but implementers should note verify_ref's internal match.

### PASS: All functional requirements have corresponding tasks
- FR-001 (project-local file): T003, T008
- FR-002 (key add + --self): T010, T011
- FR-002a (whole-ref rejection): T014, T015
- FR-002b (unsigned commits): T014 (passthrough of Missing status)
- FR-003 (key list): T018
- FR-004 (key remove): T009, T019
- FR-005 (reject untrusted during sync): T014, T015
- FR-006 (accept trusted during sync): T014, T015
- FR-007 (fallback when no file): T008 (Unconfigured variant), T015
- FR-008 (validate key format): T007
- FR-009 (no duplicates): T009
- FR-010 (optional label): T010, T011

### PASS: Dependency ordering is correct
- Phase 1 (types) -> Phase 2 (add) -> Phase 3 (sync) / Phase 4 (list/remove) -> Phase 5 (polish)
- Phase 3 and 4 can correctly parallelize (different files: sync.rs vs lib.rs)

### PASS: Test-first approach is consistently applied
- Every phase has tests written before implementation
- Tests are in parallel groups, implementations are sequential where needed

## Summary
No CRITICAL or HIGH findings. The artifacts are well-aligned with minor labeling and description clarity issues. All spec requirements have task coverage. Dependency ordering is sound. The plan's architecture integrates cleanly with the existing codebase structure.