# Known Issues

Tracked from a prior code review; status updated after the Phase 8 hardening pass.
Severity and category noted per issue.

## Gateway

| # | Severity | Category | Issue | Status |
|---|----------|----------|-------|--------|
| 1 | Critical | Correctness | Double cleanup across three code paths | **Fixed** — all close paths route through idempotent `cleanup_connection` |
| 2 | Critical | Correctness | `send_direct_to` after `remove_session` in logout | **Fixed** — `LoggedOut` sent before `remove_session` |
| 3 | Critical | Correctness | Token restore skips session limit enforcement | **Verified enforced** — `session_manager.authenticate` evicts prior same-username connections on handshake, in-band login and token-restore paths (in-memory single-connection-per-user policy; the DB-level limit applies to password logins) |
| 4 | High | Design | `clone_minimal` will silently miss new fields | Open (accepted risk — struct is small and central to the hot path) |
| 5 | High | Design | Cleanup logic duplicated between task and method | **Fixed** — DRY'd onto `cleanup_connection` |
| 6 | High | Reliability | Forward task buffer not flushed on shutdown | Open — mitigated by graceful-shutdown sequencing from Phase 7 |
| 7 | Medium | Design | Stochastic 90% message drop under pressure | Open (backpressure policy decision) |
| 8 | Low | Correctness | Rate limiter refill/consume non-atomic | Open |
| 9 | Low | Correctness | `WS_DUMMY_HASH` parameters may not match production | Open |
| 10 | Low | Style | `as_millis() as u64` silent truncation | Open |
| 11 | Low | Style | Captured `token_id_for_cleanup` used only for logging | Open |

## Routes / Handlers

| # | Severity | Category | Issue | Status |
|---|----------|----------|-------|--------|
| 1 | Critical | register | Compensating cleanup failure is silent; token generation has no cleanup | Open (mitigated by logging) |
| 2 | Critical | sms_verify_otp | TOCTOU on attempt counter despite "FIXED" comment | **Fixed** — `increment_otp_attempts_if_under_limit` is now a single atomic conditional `UPDATE … WHERE attempts < $max` (no read-check-write race). The earlier BEGIN/IF transaction form was a SurrealQL parse error that the fail-closed handler masked as a first-try 429 — found by the Phase 8 route tests, with a regression test (`otp_attempts_increment_round_trip`) |
| 3 | Critical | sms_verify_otp | No session limit for SMS — undocumented and inconsistent | Open (policy decision) |
| 4 | High | update_custom_role | Room ownership verified after role priority is used | Open |
| 5 | High | Multiple | Redundant DB queries; unscoped fetch used for permission check | Open |
| 6 | High | login | `SessionLimitExceeded` masking leaks timing signal | Open |
| 7 | Medium | refresh_token | Malformed User-Agent collapses to "Unknown" fingerprint | Open |
| 8 | Medium | set_server_role | Admin bypass undocumented; unnecessary DB call on rejection path | Open |
| 9 | Medium | send_message | HTTP handler does unbounded WS fan-out with no error handling | Open |
| 10 | Medium | sync | Breaking API change to empty filter semantics without version bump | Documented — empty `room_ids` filter returns zero rooms; asserted in route tests |
| 11 | Low | validate_username | Byte/character count inconsistency (harmless but confusing) | Covered by regression suite (`regression_username_byte_vs_char_count`) |
| 12 | Low | validate_password | Asymmetric byte/char limits undocumented | Covered by regression suite (`regression_password_byte_limit_documented`) |
| 13 | Low | login_types | Hardcoded strings not tied to enum values | Open |
| 14 | Low | leave_room | Race condition on owner-member count check | Open |
| 15 | — | ~~BroadcastDistributor::clear~~ | Resolved — distributor deleted in Phase 3 | — |

## Scope gaps (Phase-3 chat features from the construction-site scope docs)

Not bugs — functionality from `construction-site-app-scope.md` / `construction-site-app-scope2.md`
that is **not yet implemented** and tracked as the post-Phase-8 roadmap:

| Feature | Current state |
|---------|---------------|
| Per-site channel structure (site / management / DM channels) | Rooms are flat, user-created; no site-provisioned channel tree |
| Pinned document library | `PinMessages` permission exists; no pin storage or REST/WS handlers |
| Read receipts on documents | Message receipts via WS `MarkRead` only; REST `read_receipt` returns 501 by design |
| Mentions / message search | `mentions` field parsed on `MessageContent::Text`; no delivery/notification or search |
| Broadcast messages | Not implemented |
| Moderation (kick/ban enforcement) | `KickUsers`/`BanUsers` permission bits + `MembershipState::Ban` exist; no handlers |
| Retention policies | Not implemented |
| Offline queue / inbox | `InboxError` types exist in nigig-common; server-side offline inbox not wired |

## WS wire parity: `nigig-common` (server) vs `nimanyatta-protocol` (client)

The websocket wire **format** is now aligned: both sides encode/decode with
**postcard** (`to_bytes`/`from_bytes`/`decode_all` in `nigig-common` match
`nimanyatta-protocol`; the gateway's fragmentation buffering uses
`postcard::take_from_bytes` so trailing bytes are preserved). `AuthMethod`
(13 variants) is fully aligned in shape and order.

**Remaining divergences** (pre-existing in the original design — the server's
own handlers destructure shapes the client library does not emit, e.g.
`MessageContent::Text { body, .. }` server-side vs `Text { text }`
client-side). Postcard encodes enum variants as positional indices and fields
in declaration order, so these gaps change bytes on the wire:

| Area | `nimanyatta-protocol` (client) | `nigig-common` (server) |
|------|-------------------------------|-------------------------|
| `ClientToServerMsg` | 49 variants incl. `AddReaction`, `RemoveReaction`, `GetPresence`, `RequestUpload`, `UploadComplete`, `GetHistory` | 31 variants — reactions/uploads/history/presence-query missing; shared subset not in matching order |
| `ServerToClientMsg` | 81 variants incl. reactions, `ReadReceipt`, `UploadReady/Failed/Progress`, `History` | 60 variants — upload/history/reaction families missing |
| `LoginSuccess` fields | `user_id, access_token, refresh_token, device_id, expires_in` | `user_id, access_token, device_id, session_id` |
| `MessageDeleted` | `message_id, deleted_at, reason: DeletionReason, deleted_by` | `message_id, deleted_at_ms, deleted_by` (no `reason`) |
| `MessageContent` | `Text { text, formatted? }`, `Media`, `Reply`, `Deleted { …, reason }, System`, `File` | `Text { body, formatted_body, mentions }`, `Image/File/Audio/Video` (REST JSON contract — pinned by the route tests and load suites) |
| `MessageId` | `String` alias | strong `EventId` newtype (serialises as the same prefixed string) |

**Remediation path** (roadmap, not a repair): unify both enums on the
`nimanyatta-protocol` shapes (the client library now has byte-pinned
characterization tests — treat those as the contract), bump
`PROTOCOL_VERSION`, and add cross-crate wire tests that feed
`nigig_common::ServerToClientMsg::to_bytes()` into
`nimanyatta_protocol::ServerToClientMsg::from_bytes()`. The REST JSON DTOs
(`SendMessageRequest` etc.) should keep their current externally-tagged JSON
contract — they are HTTP, not the WS wire.

## Notes on earlier "stale suite" breakage

The seven load suites (`fast_integration_suite`, `chat_load_tester_realistic`,
`security_suite`, `regression_suite`, `session_limit_test`,
`enhanced_security_tests`, `enhanced_security`) failed to compile for two
independent reasons, both fixed in Phase 8:

1. The `nigig-common` protocol crate they (and the server) depend on had been
   lost and replaced by mismatched shapes. It was reconstructed as
   `nigig-lite/crates/common` from the wire contract in
   `crates/nimanyatta-protocol` plus the server's own call sites.
2. Three of the suites are `[[bin]]` targets whose bodies were
   `#[tokio::test]` functions. `#[test]` items are `cfg(test)`-gated and are
   **removed from normal (non-test) builds**, so those bins had no `main` and
   failed with `E0601`. Phase 8 adds small `main` entry points and strips the
   test attributes from the orchestrator functions, so
   `cargo run --bin <suite> --features load` works as documented.