chronogrid/docs/ARCHITECTURE/05-architecture-review-asse...

177 lines
11 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# ChronoGrid P2P — Senior Architecture Review & Assessment
**Review Date:** 2026-08-22
**Reviewers:** Architecture Review Board (AI-Assisted)
**Scope:** Full codebase, documentation, task structure, and development plan
**Status:** Accepted — Drives Master Development Plan v2
---
## 1. Executive Summary
ChronoGrid P2P is a well-conceived, architecturally sound local-first productivity system. The foundational design decisions — Rust core, SQLCipher, Automerge CRDT, libp2p, and Flutter FFI — are appropriate, modern, and consistent with the data sovereignty mandate. Phase 1 (Storage & CRDT Foundation) is **complete and production-quality**, with 80+ unit tests across `chronogrid_core`, `chronogrid_crypto`, and `chronogrid_ffi`.
The project is at a **critical inflection point**: the data layer is solid, but Phases 2–5 (Networking, UI, AI/Automation, Headless Daemon) are entirely unimplemented stubs. The documentation is partially inconsistent, the task structure covers only Phase 1, and the verification script is incomplete. This review establishes the corrective actions and the full master development plan.
---
## 2. Strengths — What Is Working Well
### 2.1 Architecture & Design
- **Local-first mandate is rigorously enforced.** No cloud endpoints exist anywhere in the codebase. All data paths terminate at SQLCipher on-device.
- **Layered separation of concerns is clean.** `chronogrid_crypto` → `chronogrid_core` → `chronogrid_ffi` → Flutter is a correct dependency graph with no circular references.
- **Schema is the single source of truth.** `db.rs` uses `include_str!` to embed `schema.sql` at compile time — an excellent pattern that prevents schema drift.
- **CRDT design is correct.** Automerge LWW registers with HLC timestamps are the right primitive for the task/note domain. Fork/merge semantics are properly tested.
- **Security implementation is spec-compliant.** Argon2id parameters (64 MiB, 3 iter, 4 threads), SQLCipher 4.x raw-hex key bypass, and OS keychain abstraction all match `02-security-and-crypto.md` exactly.
- **FFI bridge architecture is sound.** The global `Engine` singleton behind a `Mutex` with `with_engine()` / `with_engine_mut()` helpers is thread-safe and compatible with FRB's isolate pool.
- **ADR discipline is established.** ADRs 0002–0005 are present and well-written.
### 2.2 Code Quality
- Rust code is idiomatic, well-documented with `///` doc comments, and follows consistent module patterns.
- Error types use `thiserror` with meaningful variants (`CoreError::InvalidKeyLength`, `CoreError::SchemaInit`, etc.).
- `DerivedKey` implements `Zeroize + Drop` — correct handling of sensitive memory.
- Test coverage for Phase 1 is comprehensive: 11 DB tests, 45 CRDT tests, 25 FFI tests, plus crypto tests.
---
## 3. Gaps, Risks & Recommendations
### 3.1 CRITICAL — Unimplemented Crates (Blocking Phases 2–5)
| Crate | Status | Risk |
|---|---|---|
| `chronogrid_net` | Stub — only a placeholder `it_works` test | **CRITICAL** — P2P sync is the core differentiator |
| `chronogrid_ai` | Stub — only a placeholder `it_works` test | HIGH — Automation pipeline depends on this |
| `chronogrid_daemon` | No `main.rs`, no `Cargo.toml` | HIGH — Headless node is required for always-on sync |
| Flutter UI | Only `main.dart` with a placeholder text widget | HIGH — No user-facing functionality exists |
**Recommendation:** Phases 2–5 must be sequenced carefully. `chronogrid_net` is the highest priority after Phase 1 because it enables multi-device sync — the core value proposition.
### 3.2 HIGH — Missing Schema Entities
The current `schema.sql` is missing several entities required by the master user story:
| Missing Entity | Required By |
|---|---|
| `time_blocks` table | Calendar time-blocking grid (UI_UX/02) |
| `attachments` table | IMAP ingestion saves email attachments |
| `calendar_feeds` table | CalDAV bridge external feed configuration |
| `node_peers` table | P2P peer registry and swarm key storage |
| `automation_rules` table | Local automation/rules engine |
| `ingestion_configs` table | IMAP account configuration storage |
**Recommendation:** Extend `schema.sql` in Phase 2 before any networking or UI work begins. All schema changes must be additive (new tables/columns only) to preserve CRDT document compatibility.
### 3.3 HIGH — FFI Bridge Not Code-Generated
The `frb_generated.rs` file does not exist (noted in `lib.rs` as a comment: "Uncomment after running codegen"). The Dart bindings in `apps/chronogrid_flutter/lib/src/rust/` do not exist. The Flutter app cannot call any Rust functions until codegen is run.
**Recommendation:** Running `flutter_rust_bridge_codegen generate` must be the first step of Phase 3 and must be added to the CI/verification pipeline.
### 3.4 HIGH — Verification Script Is Incomplete
`scripts/verify_agent_work.ps1` only checks `chronogrid_core` and `chronogrid_ffi` compilation plus Flutter analyze. It does not:
- Run any unit tests (`cargo test`)
- Check `chronogrid_crypto`, `chronogrid_net`, or `chronogrid_ai`
- Verify schema alignment
- Check for the daemon binary
**Recommendation:** Expand the verification script to cover all crates and run the full test suite.
### 3.5 MEDIUM — Documentation Inconsistencies
| Issue | Location | Impact |
|---|---|---|
| `docs/WORKFLOWS/01-master-user-story.md` is a stub that just references the ARCHITECTURE doc | WORKFLOWS/ | Redundant file, creates confusion |
| `docs/UI_UX/03-kanban-gantt-views.md` has typos: "acklog", "eview", "amespace_id" | UI_UX/ | Spec is unreadable in places |
| `docs/UI_UX/02-calendar-timeblocking.md` has typo: "stimated_duration" | UI_UX/ | Spec error |
| OSAD Section 7 repo structure shows `build_android_libs.sh` and `build_desktop_ffi.py` — neither exists | ARCHITECTURE/01 | Scripts referenced but missing |
| `ffi_bridge_spec.json` is minimal (4 functions) vs. 38 actual FFI functions implemented | API_CONTRACTS/ | Contract is severely out of date |
| `DECISION_LOG.md` entries are out of chronological order (ADL-002 appears after ADL-005) | Root | Minor but unprofessional |
| Phase 1 tasks file marks all tasks `[x]` complete but has no Phase 2–5 task files | TASKS/ | No automated execution path beyond Phase 1 |
### 3.6 MEDIUM — Missing `chronogrid_daemon` Cargo.toml
The `apps/chronogrid_daemon/` directory has only a `.gitkeep` and an empty `src/` directory. There is no `Cargo.toml`, no `main.rs`, and the workspace `Cargo.toml` does not include it as a member.
**Recommendation:** Add `apps/chronogrid_daemon` to the workspace and scaffold the binary crate in Phase 2.
### 3.7 MEDIUM — No Integration Tests or Cross-Crate Tests
All tests are unit tests within individual crates. There are no integration tests that exercise the full stack (crypto → core → ffi → dart).
**Recommendation:** Add an `integration_tests/` directory in Phase 3 with end-to-end tests covering the full data path.
### 3.8 LOW — `FfiTaskData.due_date` Type Mismatch
In `types.rs`, `due_date: Option<i64>` but in `schema.sql` and `task.rs` the field is `u64`. This is a subtle type inconsistency that will cause issues when large epoch values exceed `i64::MAX`.
**Recommendation:** Standardize on `u64` for all epoch millisecond fields in the FFI layer, or document the intentional narrowing.
### 3.9 LOW — No Build Scripts for Android/Linux Cross-Compilation
The OSAD references `build_android_libs.sh` and `build_desktop_ffi.py` but neither exists. The Android NDK cross-compilation pipeline is entirely undocumented in executable form.
**Recommendation:** Create `scripts/build_android_libs.sh` and `scripts/build_desktop_libs.ps1` in Phase 3.
---
## 4. Architecture Decisions Confirmed
The following architectural decisions from the OSAD are **confirmed as correct** and should not be revisited:
1. **Rust + Flutter FFI** — Correct choice. Rust provides memory safety, performance, and cross-platform compilation. Flutter provides a single UI codebase for Windows/Linux/Android.
2. **SQLCipher over plain SQLite** — Correct. AES-256-CBC with HMAC-SHA512 page auth is the right encryption model for local-first data.
3. **Automerge over custom CRDT** — Correct. Automerge is battle-tested, has a Rust crate, and handles the LWW + sequence CRDT requirements.
4. **libp2p over custom P2P** — Correct. libp2p provides mDNS, QUIC, Noise, Gossipsub, and Kademlia DHT out of the box.
5. **Argon2id for KDF** — Correct. Argon2id is the OWASP-recommended KDF for password-based key derivation.
6. **flutter_rust_bridge v2** — Correct. FRB v2 provides zero-copy async streams, which are essential for real-time CRDT event propagation to the UI.
---
## 5. Recommended Architecture Additions
### 5.1 Event Stream Architecture (New)
The current FFI layer is purely request/response. For real-time UI updates (CRDT sync events, ingestion notifications), a **Dart Stream** pattern must be added:
- Rust side: `flutter_rust_bridge` `StreamSink<T>` for pushing events to Dart.
- Events: `SyncEvent`, `IngestionEvent`, `TaskUpdatedEvent`, `PeerConnectedEvent`.
### 5.2 Repository Pattern for SQL Queries (New)
The `Database` struct currently only manages CRDT blob persistence. A `Repository` layer is needed for structured SQL queries (list tasks by namespace, filter by Eisenhower quadrant, etc.) to support the UI views.
### 5.3 Eisenhower Score Engine (New — in `chronogrid_core`)
The priority score formula `S = (W_i * P_user) + (W_u * f(Δt)) + (W_a * Age)` must be implemented as a pure Rust function in `chronogrid_core` and called on every task mutation and on a scheduled timer.
### 5.4 CalDAV Embedded Server (New — in `chronogrid_core` or separate crate)
A lightweight CalDAV server on `127.0.0.1:4242` using `hyper` or `axum` for local calendar interoperability.
### 5.5 IMAP Polling Engine (New — in `chronogrid_core`)
An async IMAP client using `async-imap` + `tokio` for background email polling. Must run in a dedicated Tokio runtime separate from the synchronous SQLite connection.
---
## 6. Overall Assessment
| Dimension | Score | Notes |
|---|---|---|
| Architecture Design | 9/10 | Excellent local-first design, correct technology choices |
| Phase 1 Implementation | 9/10 | Solid, well-tested, spec-compliant |
| Documentation Quality | 6/10 | Good architecture docs, but inconsistencies and gaps |
| Task Structure | 4/10 | Only Phase 1 tasks exist; no automated execution path for Phases 2–5 |
| Test Coverage | 7/10 | Phase 1 well-tested; Phases 2–5 have zero tests |
| Build & CI Pipeline | 4/10 | Verification script is incomplete; no cross-compilation scripts |
| **Overall Readiness** | **6.5/10** | Strong foundation; execution plan needed for Phases 2–5 |
---
## 7. Immediate Action Items (Pre-Phase 2)
1. Fix documentation typos in `UI_UX/03-kanban-gantt-views.md` and `UI_UX/02-calendar-timeblocking.md`
2. Update `ffi_bridge_spec.json` to reflect all 38 implemented FFI functions
3. Expand `scripts/verify_agent_work.ps1` to run full test suite across all crates
4. Create Phase 2–5 task files in `docs/TASKS/`
5. Scaffold `apps/chronogrid_daemon/` as a proper Cargo binary crate
6. Extend `schema.sql` with missing tables (`time_blocks`, `attachments`, `node_peers`, etc.)
7. Fix `FfiTaskData.due_date` type from `i64` to `u64`
8. Update `DECISION_LOG.md` to chronological order