Database as primary conversations

[malanjary-tsri]

Disintegration of Slack as a dependency but maintain Slack as redundant storage / interface that can update the local database (now the ground truth for agent conversations).

Tested full disintegration with 5 successful proposals created (Slack totally disabled in .env)
Slack functions as normal when enabled, conversations are working, private-channels are able to be created. All functionality seems unaffected

*Note: Retroactive conversations, e.g. Slack_enabled=False, are not propagated when Slack is re-enabled. Only live conversations and interactions while enabled (due to heavy changes needed to align conversation ID's in this situation). This is only a view-only restriction as the store of truth/conversations now lives in the local DB

Slack to DB working (PI interaction updates the database)
DB to Slack operational

see issue: #18

[ahueb]

PR #19 "Database as primary conversations" — Review

What this is: an independent verification of PR #19, run against an ephemeral Postgres migrated to head 0020, driving the real SimulationEngine (live LLM for the functional part; deterministic no-LLM repros for the bug confirmations). Every claim below was reproduced by execution, not just code reading.

Bottom line: the feature works as described — Slack-off conversations persist, restart continuity holds, PI web interaction drives the agents, and Slack-on parity is intact. Recommended: mergeable after addressing 3 data-loss bugs (H1, H2, M1a) that can silently drop or overwrite committed conversation content in the store this PR makes authoritative. All three reproduce on demand.

---

1. What works (functional verification, live LLM)

Claim	Result	Evidence
DB is the primary store; content persists (Slack off)	✅	A real threaded 2-lab dialogue persisted with local: channel ids, null slack_ts, unique ids, correct threading/phases, recomputed run totals.
Runs identically with Slack fully disabled	✅	Full 5-phase loop ran on NullTransport, no Slack calls.
Restart / resume rebuilds from the DB, no duplicates	✅	2nd engine on the same run hydrated the log with content, continued the timeline, 0 duplicate ids.
PI interaction updates the DB (web message + DM)	✅	A web @tag and a web DM drove a live round-trip: DM classified → private profile rewritten → confirmation recorded as an outbound DM row — all with Slack off.
Slack-on mirror parity (message_ts == slack_ts)	✅	Mirrored row had message_ts exactly equal to slack_ts.
Migrations apply over existing rows	✅	0018→0019→0020 clean on a populated DB; legacy rows survive on server defaults; --fresh also wipes pi_dm_messages.

23 of 24 live checks passed; the one miss was a message-count threshold in a short budget window, not a correctness failure. Cost of the live run: ~$0.70.

Design is sound and better than the old Slack-primary model: cursor-bounded pollers backed by the new indexes, an idempotent ON CONFLICT upsert, a clean transport seam, a DB-agnostic MessageLog (persistence injected via callback), and a well-separated pi_inbox service. The four items below are the exceptions.

---

2. Please fix before production (all reproduced at runtime)

🔴 H1 — A failed flush silently drops conversation content

simulation.py _flush_persisted (~:2762, :2831). The buffer is detached (entries = self._pending_persist; self._pending_persist = []) before the try, and the except Exception only logs. On any transient DB error the batch is gone — and because the DB is now the source of truth, a restart cannot recover it.
Reproduced: forced a commit error → buffer emptied to 0 and 0 rows persisted; the two messages vanished.
Fix: re-queue the batch on failure (prepend back to _pending_persist) instead of dropping it.

🔴 H2 — Inbox cursor can permanently skip a PI message

simulation.py _poll_inbound_from_db (~:2098–:2108; same pattern for DMs at :2317). The poller filters posted_at > _pi_inbox_cursor and advances the cursor to the max posted_at seen — but posted_at is stamped at row creation time (pi_inbox.py:62, time.time()), not commit time. If the engine flushes its own later message and advances the cursor in the window before a web PI row commits, that PI row lands below the high-water mark and is never ingested — silently, at debug log level.
Reproduced: a row with posted_at=150 against cursor=200 was never ingested or routed, even on a second poll; a control at cursor=0 ingested the identical row (so it's the cursor logic, not the harness).
Fix: reconcile by "message_ts not already in the log" or order by a DB-assigned sequence, rather than a wall-clock high-water cursor.

🟠 M1a/M1b — Canonical-id collision between processes (clobber + 500)

The id format f"{time.time():.6f}" is minted in 4 places but only mint_ts (simulation.py:2463) is monotonic/unique; the web (pi_inbox.py:62,93) and grantbot (grantbot.py:173) writers use raw time.time(). mint_ts's guarantee is per-process only, so the engine and the web app can mint the same message_ts for the same run.

• M1a (silent): the engine's ON CONFLICT DO UPDATE set includes content/agent_id/is_bot, so on collision it overwrites a human PI row with a bot message. Reproduced: the human row became is_bot=True, agent_id=<bot>, content=<bot text>.
• M1b (visible): the same collision through POST /message raises an uncaught IntegrityError (no guard in post_agent_message) → HTTP 500. Reproduced with pinned time.
  Fix: share one mint_local_ts() helper across all writers so they inherit mint_ts's guarantee; and either ON CONFLICT DO NOTHING for genuinely distinct births or add an IntegrityError guard on the web route.

🟠 M2 — Transport Protocol under-declares its real contract

transport.py declares the Transport Protocol, but the engine mutates client._channel_name_to_id (simulation.py:1355, :2597), a private attribute not in the Protocol (NullTransport carries it "for parity" — the tell). A new backend that satisfies the declared Protocol will crash.
Reproduced: a stub with isinstance(t, Transport) == True crashed with AttributeError: '_channel_name_to_id' in _ensure_seeded_channels.
Fix: declare _channel_name_to_id (or a public accessor) on the Protocol. Minor: set_visibility_lookup on NullTransport has 0 callers in src/ — it's dead code, safe to drop.

---

2b. How likely is each to bite in production?

Calibrated to the actual topology: one web worker + one long-lived sim process sharing Postgres, turn_delay_seconds=0 (the sim flushes every tick), no pool_pre_ping, and PI web writes that are human-paced. Ranked most- to least-likely to actually surface.

Bug	Likelihood in prod	Conditioned on	Impact when it hits
H1 flush-loss	Moderate — expect it	any DB error at flush time (Postgres restart, connection reset, stale pooled conn — worsened by no pool_pre_ping)	small, silent, recurring history gaps (only the current tick's buffer; visible after next restart)
B1/B2 scaling	Gradual, ~certain	message count → 10k–100k+	rising restart time + RAM (rebuild loads all rows) and continuous per-tick overhead
H2 cursor race	Rare	a PI web write committed in a ~ms window while the sim is actively posting (cursor pinned to ~now); only-input path when Slack is off	that single PI message silently, permanently ignored
M1a/M1b id collision	Very rare	Slack-off only + two processes minting the same microsecond; ↑ sharply on coarse VM/container clocks	M1a silently overwrites a PI's row; M1b a visible, retryable 500
M2 protocol gap	Not today	only when a 3rd transport backend is added	crash for the next maintainer, not a running deployment

Reading of it:

• H1 is the one to actually plan for — a long-running process will hit transient DB errors periodically (especially around Postgres restarts), and each one silently drops that flush's messages from the store this PR makes authoritative. Low per-incident, but cumulative and invisible. Adding pool_pre_ping/pool_recycle reduces the trigger; re-queueing the batch removes the loss.
• B1/B2 are a slow tax, not an incident — fine at pilot scale, worth addressing before history grows past ~10k messages.
• H2 is rare but sharp — it undermines the feature's own promise (PI input via web) exactly when the sim is busy, and it's invisible at default log level.
• M1 and M2 are the least likely to surface in the current Slack-capable, single-worker deployment — M1 needs a microsecond cross-process coincidence in Slack-off mode (watch it only on VMs with coarse clock resolution); M2 is a future-maintenance trap, not a runtime risk today.

Net: none are high-frequency. Priority order for real-world exposure is H1 → B1/B2 → H2 → M1 → M2, which is close to (but not the same as) the severity order — H1 combines real severity and real likelihood, so it's the clear first fix.

---

3. Lower-priority notes (not blocking)

• Scaling (simulation.py): _flush_persisted runs a full COUNT(*) + SimulationRun update every tick with pending rows (cosmetic counter, index-backed but continuous); _rebuild_state_from_db loads all of a run's rows with content into memory at startup, no cap/window (measured: 2000 rows → all 2000 loaded). Both bite only at large message counts. Good: the two pollers are correctly index-backed (confirmed via EXPLAIN), and each flush is a single INSERT…ON CONFLICT round-trip.
• Modularity: simulation.py grew 3423 → 3820 lines; the new persistence/rebuild/poller/minting concerns landed on an already-large class. Consider extracting a MessagePersister/DbInboxPoller collaborator later.
• DRY / clean code: the id format (×4) and 9 near-identical LogEntry(...) builders could be centralized; the PR adds 10 broad except Exception handlers, 3 of them at debug level on the only Slack-off PI input path — a persistently failing poll would be invisible at default log level. Consider logger.warning for those three.

4. Not covered here (worth a manual pass)

• Live Slack workspace round-trip — parity was verified with a fake transport, not the real Slack Web API. You reported testing this manually ("5 successful proposals, Slack functions as normal"); a reviewer should confirm an inbound PI Slack message lands in agent_messages with slack_ts set.
• scripts/backfill_slack_history_to_db.py — not executed.
• The documented retroactive-history limitation (Slack-off history not back-propagated when Slack is re-enabled) is understood to be by design.

---

Verification used an isolated ephemeral database; the dev stack and repository were untouched (no commits, no source/test changes). Confirmation scripts and raw evidence are available on request.