diff --git a/hive-agent-mcp/src/mcp/mod.rs b/hive-agent-mcp/src/mcp/mod.rs index 6e16b4ef..f2b1fd8c 100644 --- a/hive-agent-mcp/src/mcp/mod.rs +++ b/hive-agent-mcp/src/mcp/mod.rs @@ -242,9 +242,22 @@ impl AgentServer { answer: args.answer, }) .await; - if resp.is_ok() { - // Best-effort — this agent is done owing a reply for `id`; - // drop its `answering` mirror row. + // Clear the local `answering` mirror row whenever this agent is + // done owing a reply for `id`: either a genuine success, or + // c0re telling us the question already resolved without us + // (the asker cancelled/answered it first, surfacing as an + // "already answered"/"not found" rejection) — in both cases + // nothing is still owed, so the row would otherwise linger + // stale. Any other rejection (e.g. wrong answerer) means the + // question is still genuinely outstanding, so leave it be. + let should_clear = match &resp { + Ok(hive_core_agent_sock::Response::Ok) => true, + Ok(hive_core_agent_sock::Response::Err { message }) => { + message.contains("not found") || message.contains("already answered") + } + _ => false, + }; + if should_clear { let _ = dial_agent_socket(&hive_agent_sock::Request::ClearQuestion { id }).await; } annotate_retries( diff --git a/hive-agent/src/questions.rs b/hive-agent/src/questions.rs index 9e333f0c..c2c5f8ca 100644 --- a/hive-agent/src/questions.rs +++ b/hive-agent/src/questions.rs @@ -19,12 +19,14 @@ //! `answer()` for `id` (see `hive-agent-mcp`'s tool impl). //! //! Known gap: if the asker cancels their own question, the target is not -//! notified today (`hive-c0re::questions::handle_cancel_loose_end` only -//! notifies a *different* canceller than the asker, which never happens -//! via the ownership-gated agent-socket cancel path). A target's -//! `answering` row then lingers until they call `answer()` (now a "no -//! such question" error from c0re) or the row goes stale. Flagged on the -//! tracking issue rather than fixed here. +//! proactively notified today (`hive-c0re::questions::handle_cancel_loose_end` +//! only notifies a *different* canceller than the asker, which never +//! happens via the ownership-gated agent-socket cancel path). A target's +//! `answering` row is cleared the next time they call `answer()` — c0re's +//! "already answered"/"not found" rejection is treated as resolved-without- +//! us and clears the mirror row (see `hive-agent-mcp`'s `answer()` tool) — +//! but if they never call `answer()` at all, the row lingers with no +//! proactive nudge. Flagged on the tracking issue rather than fully fixed. use std::path::Path; use std::sync::Mutex; @@ -141,11 +143,15 @@ impl Questions { } /// List every mirrored row, oldest-asked first — for `get_loose_ends` - /// rendering. + /// rendering. A row whose `role` column doesn't parse (corruption — + /// should never happen via this module's own writes) is logged and + /// skipped rather than either propagating a hard error (which would + /// hide every other, valid row) or silently misattributing it as + /// `Asked` (which would lie about who owes whom a reply). /// /// # Errors /// - /// Propagates the sqlite prepare / query failures. + /// Propagates the sqlite prepare / query / column-read failures. /// /// # Panics /// @@ -155,25 +161,27 @@ impl Questions { let mut stmt = conn.prepare( "SELECT id, role, peer, question, asked_at FROM questions ORDER BY asked_at ASC", )?; - let rows = stmt - .query_map([], row_to_question)? - .collect::>>()?; - Ok(rows) + let mut out = Vec::new(); + let mut rows = stmt.query([])?; + while let Some(row) = rows.next()? { + let id: i64 = row.get(0)?; + let role_str: String = row.get(1)?; + let Some(role) = Role::parse(&role_str) else { + tracing::warn!(%id, %role_str, "questions mirror: unknown role, skipping row"); + continue; + }; + out.push(QuestionMirror { + id, + role, + peer: row.get(2)?, + question: row.get(3)?, + asked_at: row.get(4)?, + }); + } + Ok(out) } } -fn row_to_question(row: &rusqlite::Row) -> rusqlite::Result { - let role_str: String = row.get(1)?; - let role = Role::parse(&role_str).unwrap_or(Role::Asked); - Ok(QuestionMirror { - id: row.get(0)?, - role, - peer: row.get(2)?, - question: row.get(3)?, - asked_at: row.get(4)?, - }) -} - #[cfg(test)] mod tests { use super::*; diff --git a/hive-agent/src/todo_server.rs b/hive-agent/src/todo_server.rs index 3c2e4a99..835b8850 100644 --- a/hive-agent/src/todo_server.rs +++ b/hive-agent/src/todo_server.rs @@ -172,9 +172,9 @@ async fn handle_conn( /// Apply one request to the store, firing `wake` on a new/changed upsert so /// the serve loop runs a turn. Each arm calls a small named handler function /// directly — no sub-match/`unreachable!()` indirection per family (that -/// pattern got reviewed out of the todo family in #2679; kept the reminder -/// and question families consistent with it here rather than reintroducing -/// it). `reminders`/`questions` are `None` when that store failed to open at +/// pattern got reviewed out of the todo family in the diagnostic-logging +/// follow-up PR; kept the reminder and question families consistent with it +/// here rather than reintroducing it). `reminders`/`questions` are `None` when that store failed to open at /// boot, in which case every op in that family returns an `Err` — each /// handler checks for its own `None` case. fn dispatch(