nigig-org/nimanyatta/PLAN.md

# nimanyatta — Execution Plan

## Completed Phases

### Phase 0 — Baseline Hygiene (commit `3908dbc`)
- `.gitignore` for target, logs, `.env`, concatenated dumps, `.bak`
- Deleted: `src/database.rs`, `.bak`/`.new` files, 11 `concatenated_code.txt`, `old/`, `instruments_output_*`, runtime logs, stray binaries
- `broadcast_server.rs`: removed test modules, ~470 lines dead code, dead traits
- Wrote real README with API reference, env vars, cargo aliases
- `KNOWN_ISSUES.md` created from prior review findings

### Phase 1 — Bug Fixes (commit `39401c1`)
- **500-vs-409 fix**: `is_duplicate_error()` shared by all three error mappers in `db.rs`
- **Session eviction fix**: `register_token_conn` called after password login + token restore
- **In-memory DB safety**: `DATABASE_PATH` required when `ENVIRONMENT=production`

### Phase 1 completion + Phase 2 (commit `73fa54a`)
- **Device revocation fix**: `is_token_revoked` now checks `device:<device_id>` markers; all 4 callers updated
- **Dead code removal** (-1,513 lines):
  - `chat_state.rs` (orphaned, not a module)
  - `connection_multiplexer.rs` + `connection_pool.rs` (never instantiated)
  - `io_backend/` directory (not a module, references nonexistent `ServerError`)
  - `error.rs`: removed `NigigError` parent, 7 dead sub-sets, 5 dead `From` impls — kept only `IggyError`
  - Dead `pub use` exports from `optimized/mod.rs`

### Phase 3 — Architecture Cleanup (commit `75b2b41`, -1,058 lines)
- **Deleted distributor infrastructure**: `BroadcastDistributor`, `AdaptiveBroadcaster`, `ShardedBroadcast`, `BackpressureConfig` — all vestigial (hot-path `SendGroup` already bypassed via `get_sender()` → `try_send()`)
- Removed all distributor fields, construction, register/unregister calls, broadcast_rx select arm, is_under_pressure gate
- Replaced `log_direct_drop` with local static counter + inline function

### Phase 4 — Request-Path Improvements (commit `32d237f`, -38 lines)
- **Registration rate limiting**: added IP+username rate limit to `POST /auth/register` (was unprotected)
- **Custom roles ownership check**: replaced 4x full-table `get_room_custom_roles().iter().any()` scans with bounded `role_belongs_to_room()` query (LIMIT 1)
- **Dead code**: deleted `get_active_devices_for_user()` + `ActiveDevice`/`ActiveDeviceRead` structs (zero callers)
- **Skipped token validation caching**: in-memory `PasetoManager` cache is only populated at startup; runtime revocations write to DB but don't update the HashMap. Query is fast (in-memory SurrealDB, LIMIT 1).

### Phase 5 — Security Hardening (commit `ee115ae`, +59 lines)
- **X-Forwarded-For IP filtering**: added `PROXY_TRUSTED_IPS` env var; middleware now checks TCP peer IP against trusted set before reading headers
- **WebSocket Origin validation**: added `WS_ALLOWED_ORIGINS` env var; middleware validates Origin header on WS upgrade, rejects with 403 if not in allowed list
- **UsernameOnly auth**: already properly gated behind `#[cfg(debug_assertions)]`, rejected in release builds — no change needed

### Phase 6 — Observability & Deployment (commit `b08ad8c`, +33 lines)
- **OTEL gating**: added `ENABLE_OTEL` env var (default false); when disabled, skip OTLP exporter entirely — prevents startup hang when no collector is available
- **Env var fix**: read `OTEL_EXPORTER_OTLP_ENDPOINT` first, fall back to `OTLP_ENDPOINT`
- **`/ready` endpoint**: Kubernetes readiness probe returning 200 when running, 503 during shutdown; added to PUBLIC_PATHS
- **Cleanup**: removed dead `health()` function and commented-out route from `start_xitca.rs`

### Phase 7 — Request-Path Perf + Shutdown (commit `fcaec35`, `8cc54e8`)
- **Token validation caching** (commit `fcaec35`): removed the per-request `is_token_revoked` DB query from all 4 call sites (auth middleware, WS handshake, refresh endpoint, gateway token restore). Revocation is fully covered by the in-memory `PasetoManager` cache, which is populated at startup and updated by every `revoke_token` / `revoke_all_user_tokens` call (`revoke_tokens_for_device` has zero callers). Deleted dead `Database::is_token_revoked`.
- **Unbounded query caps**: added safety caps + warnings to `get_room_member_ids` (LIMIT 2,000) and `get_user_joined_rooms` (LIMIT 1,000) so huge rooms/users degrade with a logged warning instead of unbounded memory use.
- **Stale bin config**: removed `[[bin]]` entries for deleted sources (`server`, `websocket_gateway_iggy`, `load_tester`, `simple_ws_server`) and their broken cargo aliases — the full `--features b_server` build was failing.
- **Graceful shutdown** (commit `8cc54e8`): disabled xitca's built-in signal handling (SIGINT mapped to an immediate force stop that defeated our sequencing), configured `shutdown_timeout`, drive a graceful `stop(true)` from `broadcast_server`, wait for SIGINT or SIGTERM, and only force-abort if the drain exceeds `SHUTDOWN_TIMEOUT_SECS`.

## Remaining / Future Work

### Test Coverage Plan
1. Install `cargo-llvm-cov` and run with `--features b_server` — ✅ installed (v0.9.0)
2. Added integration tests (commit `b9e5178`):
   - DB write path: `revoke_token`, `revoke_all_user_tokens`, `revoke_tokens_for_device` — each asserts idempotence of the `UPSERT` (single row) plus marker/user fields
   - `cleanup_expired_revocations` removes only expired rows
   - `load_revoked_tokens` excludes expired rows
   - `create_duplicate_user_returns_conflict` exercises the `409` mapping with a real in-memory SurrealDB (`Database::connect_fresh`)
   - Config guard: production without `DATABASE_PATH` and without `PASETO_KEY` rejected; production with both accepted (env-mutating tests serialized under a lock)
   - `is_duplicate_error` edge cases + rate limiter — already present
3. Exclusions — no Makepad/generated code is part of the `b_server` build (it lives in separate crates), so nothing to configure there. Recommended command (excludes the stray `<WORKSPACE>` debuginfo path emitted by the `../xitca-web` path-dependency macros):

   ```
   cargo llvm-cov --features b_server --bin broadcast_server \
     --html --ignore-filename-regex '<WORKSPACE>|/target/'
   ```
   Baseline (46 tests): total 11.82% lines — pure-logic modules run 22–92% (`sms.rs` 92%, `fingerprint.rs` 87%, `session_limit.rs` 43%, `shared.rs` 40%, `db.rs` 23%); HTTP/WS handlers show 0% because they need a live server and are exercised by the `load`-feature stress/instrumentation suites.

#### Phase 7 coverage run (live, instrumented)
Ran against `127.0.0.1:8080` under `cargo llvm-cov run --no-report` so unit tests + live traffic merge into one profile:
- Executed `chat_load_tester_consistency` (5×45s, 1000 users) and `chat_load_tester` (6 profiles, up to 3000 users) plus a REST/curl pass (register, login, refresh, SMS request/verify, auth failures, `/admin/reset`, `/stats`, `/metrics`, `/ready`).
- **Total: 45.86% lines** (functions 43.79%, regions 45.64%) — up from 11.82%. Auth (`auth.rs` 63%), SMS (`sms_auth.rs` 43%), WS/real-time (`realtime.rs` 59%, `hybrid_gateway_optimized.rs` 55%), `db.rs` 32%, `middleware.rs` 65%, monitoring/observability 20–80%, pure logic (sms 97%, fingerprint 90%, session_limit 84%).
- **Validated graceful shutdown** (commit `8cc54e8`) live: SIGTERM on the app → `Graceful stopped xitca-server-worker-N` for every worker, `Shutting down observability`, `Shutdown complete.`, port released, exit in ~2–4s (timeout backstop not hit).
- **Stale load suites — repaired in Phase 8** (see below): all seven suites (`fast_integration_suite`, `chat_load_tester_realistic`, `security_suite`, `regression_suite`, `session_limit_test`, `enhanced_security_tests`, `enhanced_security`) now compile under `cargo check --features load --bins`.
- **Instrumented-debug caveat**: the `-C instrument-coverage` build is ~10–50× slower than release (register/login 2–4 s, REST keep-alive connections hit `EOF while parsing…` timeouts after a burst). On this build the REST success paths for `routes/rooms.rs`, `routes/roles.rs`, `routes/sync.rs` remain at 0% — the two working load testers drive those operations over WS instead and reach their handlers (`create/join/send` messaging paths covered via WS). To close the REST 0% gaps deterministically, add in‑process route tests using the existing `Database::connect_fresh` pattern (build `AppState` + handlers against embedded SurrealDB, bind an ephemeral port) rather than timing windows against a live instrumented binary. Re-run the stress profiles against a **release** build for trustworthy pass/fail numbers.

#### Phase 7.5 — In-process route tests (closes REST 0% gaps)
Added `src/routes/tests.rs` (`#[cfg(test)]`, wired via `mod tests`): a single `rest_routes_in_process` test builds the **exact production service chain** (`App` → `/ws` + auth/room/role/sync/monitoring routes → `nimanyatta_middleware` → `HttpServiceBuilder::h1()`), serves it on an ephemeral port over `xitca_server::Builder` (worker_threads 1), and drives it with `reqwest` + JSON bodies — no fixed port, no timing windows, no external processes.
- Dev-deps: `reqwest` (plain HTTP, `default-features = false`, features `["json"]`), `serde`, `serde_json` (alongside existing xitca-web/tokio/tokio-tungstenite/futures-util). `xitca-test` was tried first but its build.rs needs `protoc` (grpc), so the harness reimplements `test_h1_server`'s ~15 lines locally and forces a graceful stop via `ServerHandle::stop(true)` + `join.await` (dropping `ServerFuture` from async context panics — "Cannot drop a runtime").
- Flow covered: public `/health`,`/ready`; loopback monitoring `/stats`,`/metrics`; unauthenticated `/nowhere`+`/api/v1/sync` → 401; register (weak password → 400, duplicate → 409, wrong pw/unknown user → 401); login token extraction (tokens are nested `{token, expiry}` objects); refresh rotation (asserts token changed) + garbage → 401; SMS request/verify (Console provider); single-device logout (access **and** refresh → 401, device is deactivated); re-login → all-devices logout (both tokens → 401); group + direct room create, join + duplicate 409, send `MessageContent::Text` (externally-tagged), messages pagination, room roles list/create/assign/update/remove/delete, set room role, set server role (owner seeded `ServerRole::ServerAdmin` via `Database::set_server_role` since granting requires an existing admin), sync with full filter.
- **Real bug found & fixed**: `Database::role_belongs_to_room` compared `id = $role_id` (Surreal record id vs bare ULID string) → always false → `assign`/`update`/`remove` custom role 404'd. Now `id = type::record('custom_role', $role_id)`.
- **Test-run coverage now 49.01% lines** (functions 43.54%, regions 46.78%) on 47 tests. The previously-0% REST handlers are now exercised: `routes/roles.rs` 92.86% (lines), `routes/rooms.rs` 50.00%, `routes/sync.rs` 36.09%. Combined with the Phase 7 live-traffic profile, total ≥ the 45.86% live number (test binary excludes the `broadcast_server` main/run path, which the live run covered at 70%).

### Phase 8 — Protocol crate reconstruction, WS hardening, REST coverage, suite repairs

**Context**: the `nigig-common` dependency (`../nigig-lite/crates/common`) was reconstructed as a faithful, fully-tested crate (21 unit tests) mirroring the wire contract defined by `crates/nimanyatta-protocol` and the server's own call sites. The previously-lost shapes were recovered from the consumer code: `UserProfile`, `DeliveryReceipt { message_id, state, timestamp, by_user }` (+ `sent`/`delivered`/`read` constructors), `DeliveryState` five-state enum, `MessageAck` + `message_ack()`/`group_message_with_ack()` constructors, `MessageDeleted { …, deleted_by }`, `AuthMethod::SsoCallback { access_token, user_id }`, `client_msg_id: String` on `SendGroup`/`SendPrivate`, `JoinRoomRequest { reason }`, `Room.created_at: DateTime<Utc>` (non-optional), `AppError::{DatabaseConnection{source}, db_connection(op, err), SmsSendFailed{reason}, OtpMaxAttemptsExceeded (unit), SessionLimitExceeded{limit: usize}}`, `From<Box<dyn Error>>` for `AppError`, and enum helpers `RoomRole/ServerRole/MembershipState::{as_str, from_str}`, `ServerRole: Default`, `PermissionSet::{merge, defaults_for_server_role, defaults_for_room_role}`.

- **WS origin fail-closed (production)**: `Config::from_env` now **rejects an empty `WS_ALLOWED_ORIGINS` when `ENVIRONMENT=production`** (startup fails with a clear message); `*` remains an explicit opt-out. Non-production keeps the permissive default for local dev. Tests: `production_requires_ws_allowed_origins`, `production_with_database_path_and_key_ok` (now also sets origins).
- **PROXY_TRUSTED_IPS documented**: README env table expanded (PROXY_TRUSTED_IPS, WS_ALLOWED_ORIGINS, CORS_ALLOWED_ORIGINS, ENABLE_OTEL, token durations) plus two explainer sections — the proxy trust model (headers only honoured for TCP peers in the trusted set) and the WS origin fail-closed model.
- **Four unwired REST routes registered** (`src/routes/mod.rs`): `rooms/{room_id}/invite` POST, `/typing` PUT, `/read_receipt` POST, `/redact/{event_id}` POST — the handlers existed and were documented in the README but never reachable (root cause of the rooms 50 % coverage ceiling).
- **`POST /rooms/{room_id}/read_receipt` now returns 501** (`AppError::HttpError`) instead of 500 — REST read receipts are not persisted yet; the WebSocket `MarkRead` path *does* record receipts. README documents this explicitly.
- **README API tables corrected** to the real paths/methods (rooms path-param form, roles, SMS).
- **Gateway KNOWN_ISSUES fixes**: #1 centralised + idempotent close-path cleanup via `cleanup_connection`, #2 `LoggedOut` now sent **before** `remove_session`, #5 DRY'd eviction loop onto `cleanup_connection`; #3 verified already enforced (`session_manager.authenticate` evicts prior same-username connections on both handshake and in-band login paths, including token restore).
- **Gateway receipt bug fixed**: `DeliveryReceipt::read/delivered` call sites passed `Option<UserId>` where the (reconstructed) protocol wants `by_user: Option<String>` — now `Some(username.clone())`.
- **Load suites repaired** (`cargo check --features load --bins` → 0 errors):
  - `fast_integration_suite`, `session_limit_test`, `regression_suite` are `[[bin]]` targets whose bodies are `#[tokio::test]` functions — `#[test]` items are `cfg(test)`-gated and vanish in normal builds, so the bins had no `main`. Added small `main`s (manual multi-thread runtime / `#[tokio::main]`) and stripped the test attributes from the orchestrator functions.
  - All other compile breaks (enum shapes, missing `JoinRoomRequest.reason`, removed imports) resolved via the nigig-common reconstruction above; `cargo check --features b_server --bin broadcast_server` is clean except the intentional `RocksDb` import that requires the `kv-rocksdb` feature (restored for the final full build).
- **Coverage tests added** (`src/routes/tests.rs`): invite → join → 409-on-reinvite → 404-unknown-user, typing PUT (member + 403 non-member), redact (happy path, 404 unknown event, 403 non-owner), read_receipt 501, custom-role creation denial (403 without `ManagePermissions`), non-member send 403, messages pagination (`limit`/`direction`/invalid `from` → 400), sync filtered (room filter, `timeline_limit`, `include_state`), sync invalid `since` → 400, empty room filter → zero rooms.
- **Real bug found & fixed — OTP attempt counter never incremented** (`db.rs::increment_otp_attempts_if_under_limit`): the `BEGIN TRANSACTION; … IF … THEN ( UPDATE …; true ) …` SurrealQL block is a **parse error** (`;` is not a valid statement separator inside an IF block), so every wrong OTP made the query fail — and the handler's fail-closed `.unwrap_or(false)` masked the DB error as `OTP_MAX_ATTEMPTS_EXCEEDED` (429) on the *first* wrong attempt. Rewritten as a single atomic conditional update: `UPDATE type::record('otp', $id) SET attempts = attempts + 1 WHERE attempts < $max RETURN VALUE attempts` — empty result set ⇒ at-limit, and the read-check-write race is gone entirely. The handler now logs the underlying error before failing closed (so a broken query can never hide as a 429 again). Regression test: `db::tests::otp_attempts_increment_round_trip` (store → fetch → increment → counter persisted).
- **Server-role permission baseline**: `PermissionSet::defaults_for_server_role(User)` grants `CreateRooms`/`SendMessages`/`ReadMessages`/`InviteUsers` — room-scoped powers still come from room roles; membership checks precede permission checks in every handler.
- **WS wire format aligned to postcard** (follow-up to the remote's protocol-crate wire pinning): `nigig-common`'s `to_bytes`/`from_bytes` now use **postcard** (they had been reconstructed with serde_json), matching `nimanyatta-protocol` and the pre-existing `chat_load_tester_consistency` suite, which already called `postcard::from_bytes` on `ServerToClientMsg`. `decode_all` uses `postcard::take_from_bytes` so a fragmented frame re-buffers and bytes trailing a complete message are preserved. Remaining server/client **shape** divergences (variant sets, `LoginSuccess`/`MessageDeleted`/`MessageContent` fields) are pre-existing and documented with a remediation path in KNOWN_ISSUES ("WS wire parity").
- **Final verification**: `cargo test --features b_server` → **51 passed, 0 failed** (47 prior + OTP round-trip + WS-origin fail-closed config tests + 2 new route-coverage suites); `cargo test -p nigig-common` → **21 passed**; `cargo check --features load --bins` → clean (suite mains repaired); full `kv-rocksdb` compile check re-verified after restoring the production feature set.

### Remaining / Future Work (post-Phase-8 roadmap)

Scope docs (`crates/apps/nigig-site/construction-site-app-scope.md`, `construction-site-app-scope2.md`) Phase-3 chat gaps **documented as open** (not yet implemented — see KNOWN_ISSUES "Scope gaps"):
1. **Channel structure** per construction site (site channels auto-provisioned per project; management channel; DMs) — currently all rooms are flat, user-created.
2. **Pinned document library** per room/site (pins exist in the permission model; no pin storage/REST yet).
3. **Read receipts on documents** (message receipts land via WS `MarkRead`; document-level receipts need schema work).
4. **Mentions / search / broadcast / moderation (kick/ban enforcement) / retention policies** — permission bits exist (`PinMessages`, `KickUsers`, `BanUsers`); no handlers.
5. **Offline queue** — inbox structures exist in nigig-common (`InboxError`); server-side offline inbox not wired.

Non-scope-gap follow-ups:
- **X-Forwarded-For without trusted proxy**: requests from untrusted peers keep the raw TCP peer IP (headers ignored) — documented in README; a real reverse proxy must set `PROXY_TRUSTED_IPS`.
- **`cargo llvm-cov` re-run** after Phase 8 to refresh the coverage numbers (rooms/sync percentages will rise with the newly registered routes + tests).