Agent engine: turn & thread state-machine correctness (7 PRs)

[ahueb]

Verified defects in the agent engine's turn and thread state machine. All live in src/agent/simulation.py (plus state.py, message_log.py, agent.py), all run in the agent-run container, so they share one reviewer and one test surface.

Originally verified at origin/main @ b7edcbc (2026-07-30). Re-verified 2026-08-11 against the open PR-stack tip (issue-29-authorship-grounding @ b1d54da = main + #30/#31/#32); line numbers below refer to that tree. Items marked fixed in stack land when those PRs merge — do not re-fix them.

Priority (triage 2026-08-11)

Tier 1 — live-harm, do next after the #30/#31/#32 stack merges: E4 (COR-10 kills the whole sim on one transient network error; its DB-inbound variant also silently loses the PI message), then E2's COR-2 (a single @-mention closes a thread as "timeout" with zero replies), E3 (any human — including another lab's PI — clears a review block; a duplicate PI-guidance row is persisted per restart), and E5's COR-9(b), which re-verification upgraded to a confidentiality defect.
Tier 3 — batch with other engine work: E7, E1 remainder, E6 remainder, COR-8.

Suggested order: E4 → E2 → E3 → E5 → E7 → E1 → E6. Land against the stack, not main, to avoid simulation.py conflicts.

PR E1 — Thread-outcome state-machine integrity (small)

• COR-1 — partially fixed in stack. _post_message is now -> bool (:3234) and all four call sites gate on it (:1453, :2329, :2349, :2391); the ThreadNotFound-evict path returns False before any LogEntry, so the phantom ProposalRef/ThreadDecision/PI-DM chain is closed. Remaining: the swallowed-SlackApiError half — _post_one returns None on any non-thread_not_found error (slack_client.py:709-711), after which _post_message mints a local id, persists the row and returns True (:3384-3387), so a connected client whose post failed is indistinguishable from the Slack-off path: the turn counts, threads close and proposals mint for a message absent from Slack. Also _evict_dead_thread (:1747-1783) still never purges the message log and does _closed_thread_ids.discard (it un-closes a Slack-dead thread) — low severity now that no phantom entry is written; and _check_private_channel_outcome runs outside the posted guard (:2415-2420), a latent re-instance of the same mechanism on the private path. Fix: signal the Slack failure distinctly from the mock path; purge the log on evict; move the private outcome check under the posted guard.
• COR-3 — _check_thread_outcome (:1486-1497) finalizes on any ✅ against the first prior other-agent :memo: in reversed history — no adjacency/recency check, so an incidental "✅ done" finalizes a stale memo. The private sibling (:1638-1651) explicitly added a false-finalize defense ("without this a casual ✅ could finalize the un-revised proposal") — the codebase knows this failure class and hardened only the private path. Fix: require the memo be the most-recent other-agent message / newer than our last.
• COR-4 — public path matches only raw ✅ (:1486); private path matches ✅ and :white_check_mark: (:1633). Within the same function the pause signal accepts both forms (:1522), so two of three markers are dual-form and only ✅ is single-form. Consequence is a missed finalization → the thread grinds to the 12-message timeout close — lost yield, not corrupt data (medium). Fix: one shared marker check for both paths.
• COR-7 — _close_thread's _prior_threads append (:1544-1548) lacks dedup; the dict carries no thread_id, so dedup isn't even expressible, while the pending_proposals sibling 30 lines down filters by p.thread_id (:1558-1561). The DB-rebuild path is guarded and tested (tests/integration/test_state_rebuild.py:207-231), leaving the in-process append the only hole — and since each reclose writes a fresh ThreadDecision, duplicates legitimately reappear after restart. Severity low (prompt bloat). Fix: carry thread_id in the entry and dedup; idempotency-guard _close_thread.

PR E2 — Scan-cursor semantics + Phase-3 activation offset (small-medium)

• COR-6 — _run_turn sets last_seen_cursor = time.time() (:992, wall clock) while MessageLog since-filters use posted_at <= since (message_log.py:198/310/338/426). Narrower than originally filed: turns are serialized, so in-process agents can't append mid-turn — the live loss window is external writers (Slack humans, the web app, and GrantBot, a separate process minting posted_at from its own clock) plus cross-process clock skew (which message_log.py:222 itself flags). PI-origin rows are mitigated (the pollers apply side effects directly); bot-origin rows are not — a GrantBot funding post can be invisible to an agent forever. Phase 4 uses the same cursor (:1261) and its own comment concedes "The cursor advances unconditionally each turn, so has_new can't be relied on" (:1268-1273). Fix: advance from message_log.latest_timestamp (:436-445) — which _rebuild_agent_state (:4286-4289) already uses.
• COR-2 — Phase-3 activation builds ThreadState with no message_count_offset (:1178-1184 tag path, :1219-1225 reply path), so _reply_to_thread recomputes message_count (:1307) and closes an activated ≥12-message thread as "timeout" (:1320) before composing anything. The reopen paths do set the offset (:2943/:2954/:5017/:5028) — proving the omission. Reachability is strongest where it matters most: funding threads are open-to-all (get_thread_allowed_agents returns None, message_log.py:363), so the allowed-set guard doesn't block a newly tagged agent, and multi-party funding threads cross 12 messages quickly. Net effect of one @-mention: a ThreadDecision(outcome="timeout") row, a PI DM, two memory events — and zero replies. Fix: set the offset on Phase-3 activation.

PR E3 — Durable + authorized PI reviews and reopens (medium)

• COR-5 — _check_pi_proposal_review (:2895-2908) matches only thread_id, flips reviewed=True for all agents with no sender check, and persists nothing → dashboard/email keep showing "unreviewed" (both read ProposalReview rows, which this never writes) and _rebuild_agent_state (:4181-4189) re-blocks on restart. The Slack call site runs outside the pi_agent_ids loop (:2734-2760), so any workspace human clears it; on the DB path the web writer takes free-form thread_ts (agent_page.py:955-1010) and pi_may_post_to_channel allows any non-private channel — so any authenticated PI can clear another lab's block. Fix: require the sender be the owning PI; insert a ProposalReview.
• COR-13 — two DB→memory readers use different keys — rebuild (thread_decision_id, agent_id) (:4144-4151) vs per-tick (agent_id, thread_id) (:4892) — because ProposalRef carries only thread_id (state.py:50-58); after a re-propose cycle the rebuild blocks and the next tick silently unblocks. The rating-0 web reopen is deduped only by the in-memory _db_reopened_thread_ids (initialised empty at :303, never seeded from DB), and the reopen mints a fresh id and appends a synthetic PI-guidance entry that is persisted (:4988-5003) → one extra persisted PI-guidance row per restart, plus a fresh reply budget each time. Slack-native _reopen_thread (:2910-2960) writes nothing durable, and the rebuild re-closes every ThreadDecision thread (:4051-4072), erasing it on restart. Fix: add thread_decision_id to ProposalRef; unify the review key; persist reopen/dedup state.

PR E4 — Poller & LLM-log-flush robustness (trivial)

• COR-10 — three parts, re-rated individually:
  1. _poll_pi_dms calls poll_dm_messages unguarded (:2982); _call_with_retry catches only SlackApiError (slack_client.py:310-341; the SDK re-raises timeouts/SSL/DNS), and the poller runs outside the turn try (:643 vs :706-710) → a transient socket error kills the whole sim (one-off container → no restart; main.py:268-270 logs and the run ends). Both sibling pollers prove the intended per-item except Exception pattern (:2792, :3160). High.
  2. No DM poll throttle — re-rated low: each agent polls with its own token, the DM channel id is cached, and tick cadence is floored by the idle backoff.
  3. _poll_inbound_from_db advances the cursor (:2832-2833) and appends to the log (:2850) before the unguarded handler call (:2853) → on a raise the run dies and the side effect is permanently lost, because the lookback re-scan dedups on the now-present log entry (:2836-2840). Worse than originally rated: silent data loss on top of the crash. Fix: wrap both like the sibling pollers; apply side effects before (or transactionally with) the cursor advance.
• COR-11 — _flush_llm_logs clears the buffer before the write (:4337-4339); the except only logs (:4360-4361) → up to 10 rows lost per failed flush. Re-rated low-medium: not purely observability anymore — the org1 parity: generic blackbird work + prod hardening, agent behaviour frozen (0018→0024) #30 sliding-window rate limiter rebuilds call_times from llm_call_logs on restart (:4250-4276), so dropped rows under-count an agent's in-window calls and let it exceed its allowance after a restart. Fix: mirror PR Database as primary conversations #19's H1 re-queue.

PR E5 — Phase-5 output hygiene + Slack mention resolution (small)

• COR-9 — one of three fixed:
  • (a) visibility mislabel — fixed in stack: _post_message now resolves visibility from the channel (:3356, :3379) and persists it.
  • (b) reply channel from LLM output — still present, severity raised: the reply channel is action_data.get("channel", "general") (:2223), never reconciled with the target post. A reply targeting a collab_private post while declaring "general" passes the private-reply bypass, posts publicly, and is persisted channel="general" / visibility="public" — which now feeds the public memory-synthesis segment. This is a private→public confidentiality path, not a routing bug; the (a) fix makes the row faithfully record the leak. Fix: derive the reply channel from the target entry.
  • (c) memory-synthesis strip — downgraded to hygiene: the synthesis call's prompt never asks for <slack_message> tags, and the stack now routes the response through strip_ungrounded_authorship_lines (:5225-5231). A defensive strip remains worthwhile; there is no live trigger.
• COR-8 (remainder) — real Slack <@Uxxx> mentions are undetected (_extract_tagged_agent regex @(\w+[Bb]ot)\b, message_log.py:397-401; PI-tag literal check f"@{bot_name.lower()}", simulation.py:2785-2787) and the regex is case-blind above [Bb]. grep -rn '<@' src/ → zero hits; the bot_uid_to_agent map exists (:3918-3922) but only for authorship attribution. Corroborating evidence that the literal form is the only one understood: the web UI synthesizes it (agent_page.py:979-981). Fix: translate <@Uxxx> via the bot-user-id map; add IGNORECASE.
  • Cross-issue: the same IGNORECASE gap exists in funding_rules._TAG_RE — see PR V8 in issue External clients: Slack SDK, GrantBot, FOA regexes, HTTP robustness (3 PRs) #23. Land one shared helper or keep the two in sync.

PR E6 — Budget & rate-limit state integrity (medium)

PR #30 replaced the cumulative --budget cap with a sliding-window limiter (_within_rate_limit, :458-481; budget_cap default now 0 and deprecated, main.py:253-262) and consolidated the five raw increment sites into Agent.record_api_call (agent.py:94-104) — which retires part of the original framing. Remaining:

1. Still no mid-turn check — limiter and cap are consulted only at selection (_turn_eligible, :841-856); Phase 4 fans out over all of an agent's threads in one gather (:1286-1291) with per-retry booking, so a turn can overshoot the window allowance arbitrarily.
2. Roster re-add builds a fresh Agent (:4543-4545): an inactive→active flip discards pending_proposals (the unreviewed-proposal block evaporates), active threads, cursors — and now also call_times/throttled, i.e. a status flip is a rate-limiter reset. Severity raised; this is state loss, not accounting. No _rebuild_agent_state() follows the add.
3. total_api_calls recomputed from the live roster (:3887, main.py:293) is non-monotonic — cosmetic (the code labels it so).
   Fix: one authoritative mid-turn charge check; rebuild re-added agents from the DB instead of fresh.

PR E7 — Liveness / cost loops (small)

All four still present: the daily-cap gate returns before the PI-priority/private/funding bypasses are even computed (:2002-2004 vs :2020-2091); has_pi_directive is cleared unconditionally at _run_turn scope (:989) — a directive arriving on a capped or throttled turn is consumed without ever reaching a prompt; thread.pi_context is set at four sites (:2780/:2885/:2942/:5016) and never cleared in-process (the DB rebuild omits it, so a restart clears it) → re-injected as "authoritative" into every future Phase-4 prompt (agent.py:461-467); the interesting_posts swap/restore both sit before the try (:2093-2094 vs :2174, try at :2177) so an exception in the prompt-build window permanently narrows state. Fix: move the cap gate after bypass evaluation; clear pi_context after consumption; clear has_pi_directive only when acted on; wrap the swap in try/finally.

Definition of done: each PR ships a test that covers its defect line and fails against the pre-fix code. The offline gate now enforces COV_MIN=60 with a src/ lint ratchet (see issue #27 I1); this issue's tests should be written against the #30/#31/#32 stack.