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.

[malanjary-tsri]

DB-primary conversations — residual failure modes (post H1/H2/M1a fixes)

What this is: a deep inspection of the db-primary-conversations branch (HEAD
9548c08) hunting for cases where local-DB-backed conversations fail compared to
the original Slack-primary system. Complements .notes/PR19-review.md, whose three
data-loss bugs (H1 flush re-queue, H2 inbox cursor skip, M1a human-row clobber) are
now fixed on this branch. The findings below are what remains after those fixes.

Bottom line: the all-off steady state is functionally solid — a live resume ran 34+
turns with agents doing Phase 4 replies and Phase 5 posts, no errors, all agents
unblocked. The remaining gaps are all at the cross-process and durability
boundaries that Slack used to cover for free (unique authoritative ts, always-durable
on post, global ordering). Two are worth fixing; one is a latent fragility; the rest are
minor. R1 and R2 were reproduced by execution against the live modules/DB (rolled back).

Status 2026-07-25: R1, R2 and R3 are fixed (see the per-finding notes below).
The two "minor observations" are still open. Full suite green: 500 passed.

---

R1 — Cross-process canonical-id collision silently drops a message (confirmed)

Severity: medium · Probability: low (grows with run length + PI activity)

TsMinter guarantees monotonic/unique ids only per process (src/agent/ids.py:29-36,
docstring says so explicitly). Three processes mint canonical ids into the same run:
the engine (self._ts_minter), the web app / PI inbox (mint_local_ts() → module
_default, src/agent/ids.py:63-68), and GrantBot. Two of them minting in the same
microsecond emit the identical id. The DB uq_agent_messages_run_ts constraint catches
it, and the M1a ON CONFLICT … WHERE guard (src/agent/simulation.py:2958-2961) resolves
it by keeping the human row and dropping the bot message (or a later human upsert
overwrites a bot row). Slack never had this — Slack guarantees a unique ts. The
resolution is lossy (drop, no re-mint), and the DB is now the only durable store.

Reproduced (independent minters at a fixed instant; then the real on-conflict statement,
rolled back):

[collision] engine=1800000.000000  web=1800000.000000  SAME=True
[conflict]  rows_at_ts=1  surviving is_bot=False  content='PI: please pivot to aging biology'
[conflict]  -> bot message LOST = True

Fix options: add a per-writer discriminator to the minted id; or re-mint + retry on
conflict instead of DO UPDATE; or mint canonical ids from a DB sequence. The M1a guard
(protect PI data) is correct and should stay; the gap is that the losing row is
discarded rather than re-minted.

FIXED 2026-07-25 — per-writer discriminator (option 1). TsMinter(writer_id) now
quantizes ids to a 100 µs slot and stamps the writer id into the low microsecond digits,
so each minter owns a residue class and a cross-writer collision is structurally
impossible. Writer ids (WRITER_ENGINE / _WEB / _GRANTBOT / _ENGINE_AUX) are
claimed at process entry via set_default_writer_id() in src/main.py,
src/agent/main.py and src/agent/grantbot.py. Ids stay ts-shaped, float-parseable and
strictly ordered; the cost is resolution (1 id per writer per 100 µs), far above the real
posting rate. Chosen over re-mint-on-conflict because re-minting a thread root would
have to rewrite MessageLog._by_ts, every child's thread_ts, PostRef.post_id,
ThreadState.thread_id, ProposalRef.thread_id and the ThreadDecision rows; and over a
DB sequence because mint_ts() is called from sync code. The M1a guard and the web
route's M1b retry stay as backstops. Covered by tests/unit/test_ids.py::TestWriterSlots
and an integration test that drives both writers at a frozen clock against the real
unique constraint.

R2 — Un-flushed messages lost on hard kill, unlike Slack (confirmed by code path)

Severity: medium · Probability: medium

Posts are appended to the in-memory MessageLog and only written to Postgres by
_flush_persisted, which runs once per turn (src/agent/simulation.py:471) and on
graceful shutdown via the SIGTERM/SIGINT handler → stop() (src/agent/main.py:225-230,
simulation.py:481). The documented restart step docker rm -f agent-run sends
SIGKILL, bypassing the graceful flush. Any message posted during the in-flight turn is
lost, and since Slack is off it is unrecoverable (restart rebuilds from the DB, where
those rows never landed). In the Slack-primary system those posts were durable on Slack
the instant they were sent. The H1 fix only re-queues on flush exception, not on
process death.

Fix / mitigation (easy): use docker stop agent-run (SIGTERM, 10 s grace — the
handler already flushes) before docker rm; update the CLAUDE.md restart recipe
accordingly. Stronger: flush after each post rather than each turn, or persist
synchronously inside _post_message.

FIXED 2026-07-25. Three parts, because the doc change alone was not enough:

1. Restart recipe is now docker stop -t 30 agent-run && docker rm agent-run in
   CLAUDE.md, README.md and scripts/provision_slack_bots.py; the agent service
   also carries stop_grace_period: 30s, which is baked into the container's
   StopTimeout so a bare docker stop gets it too.
2. The graceful path was itself racy (not in the original finding): the handler did
   asyncio.ensure_future(sim_engine.stop()), so the main loop could return first and
   asyncio.run would cancel the orphan flush mid-await. The handler is now the sync
   request_stop() (flag only), and await sim_engine.stop() runs in the entry point's
   finally-block — on every exit path, signal or not.
3. SIGTERM could not be noticed in time: the idle backoff sleeps up to 30 s, longer
   than the old 10 s grace. Those sleeps go through _sleep(), which races the sleep
   against a stop event, so shutdown starts immediately.

An in-flight LLM call is still not interruptible — hence -t 30 rather than the default.

R3 — Inbox delivery depends on wall-clock agreement + a fixed 300 s lookback

Severity: low · Probability: low on a single host, real cross-host

Both DB pollers select new rows with posted_at > cursor - 300s
(src/agent/simulation.py:2138 and :2376; PI_INBOX_LOOKBACK_S = 300,
:120). The cursor is the max posted_at seen across all rows, including the
engine's own bot posts, so it tracks ≈ now during active runs. A PI/web message whose
posted_at lands more than 300 s below the engine's cursor is silently skipped
forever. This assumes the writing process's clock and the engine's clock agree to
within 300 s. Safe on one host (shared clock); Slack's authoritative ordering made it a
non-issue. Cross-host deployment or large clock skew breaks inbound PI delivery with no
error. Consider an ordering guard (e.g. a monotonic DB sequence / id-based cursor)
rather than a pure wall-clock window.

FIXED 2026-07-25 — both pollers now page over created_at instead of posted_at.
agent_messages.created_at / pi_dm_messages.created_at are server_default=now(), i.e.
stamped by the single Postgres server, so the window depends on one clock rather than on
every writer's clock agreeing with the engine's. posted_at remains the ordering key for
conversation content; it is just no longer the delivery cursor. An id-based cursor was
rejected: the PK is a random uuid4, so it would have meant a new sequence column plus a
backfill, where created_at already exists on every row. The lookback stays (it covers
commit visibility, not skew — H2). Cursors are datetime now, seeded from
MAX(created_at) over the whole run (_seed_pi_inbox_cursor) rather than from the
windowed rebuild's loaded rows. Migration 0021 adds
ix_agent_messages_run_created and ix_pi_dm_run_direction_created to back the new
access path. Covered by
test_inbound_poller_delivers_a_row_from_a_skewed_writer_clock.

---

Minor observations

• Thread history is not posted_at-ordered. MessageLog.get_thread_history
  (src/agent/message_log.py:123-131) returns root + replies in log-insertion order,
  not posted_at order. Single-process appends are ~ordered, but reconcile / DB-poll
  appends arrive later and can subtly reorder the LLM's thread context vs. the
  Slack-primary rebuild (which fetched in ts order).

  FIXED 2026-07-25 — replies are sorted by posted_at (stable, so ties keep
  insertion order); the root stays pinned first, since it is the parent by definition
  even if a skewed writer stamps a reply below it. Note this also re-orders the input to
  get_thread_allowed_agents' first-two-participants rule, which is the intended
  correction and is covered by tests.

• Mid-life Slack toggling breaks threading. A reply's slack_thread_ts is set to the
  canonical (possibly minted) thread_ts (src/agent/simulation.py:2923). If a
  conversation started Slack-off and Slack is later enabled, that minted id is not a valid
  Slack ts, so the mirrored reply detaches or errors. Design targets all-on or all-off;
  toggling mid-conversation is unsupported.

  FIXED 2026-07-25 — it now degrades safely rather than corrupting the mirror. The
  bug was wider than the DB column: _post_message passed the canonical thread_ts
  straight to client.post_message, so the Slack API call itself was wrong, not just
  the recorded mapping. New _slack_parent_ts() resolves canonical → Slack via the root
  entry's slack_ts; a reply whose root has no Slack presence is kept DB-only with a
  warning instead of being posted against an unknown id. LogEntry gained
  slack_thread_ts, set at every Slack-origin site and used by the flush.
  Prerequisite found while fixing: the rebuild/hydrate paths were dropping slack_ts
  from restored entries entirely, so after any restart every root looked DB-origin — that
  would have turned the fix into a regression that stopped mirroring all replies. They
  now restore the mapping, inferring it for pre-Stage-6 rows (real Slack channel_id +
  NULL slack_ts ⇒ canonical id is the Slack ts).

[malanjary-tsri]

Additional minor fixes (see commit descriptions) added and testing on su08 show local DB conversations are healthy