From c1f27e3b7b54fec8493bef9814122ca2f8737813 Mon Sep 17 00:00:00 2001 From: damocles Date: Wed, 27 May 2026 11:57:27 +0200 Subject: [PATCH] approvals: manager can withdraw pending approvals (closes #250) --- docs/approvals.md | 15 +++ frontend/packages/dashboard/src/dashboard.css | 2 + frontend/packages/dashboard/src/tabs.js | 1 + hive-ag3nt/src/mcp.rs | 18 ++- hive-c0re/src/approvals.rs | 105 +++++++++++++++++- hive-c0re/src/dashboard.rs | 1 + hive-c0re/src/questions.rs | 30 +++++ hive-sh4re/src/lib.rs | 26 +++-- 8 files changed, 182 insertions(+), 16 deletions(-) diff --git a/docs/approvals.md b/docs/approvals.md index e587add0..fa75fa16 100644 --- a/docs/approvals.md +++ b/docs/approvals.md @@ -63,6 +63,21 @@ happens after a decision lands. ApplyCommit kind) land in the manager's inbox, carrying both the canonical sha and the terminal tag. +### Withdrawing a pending approval + +The manager can call `cancel_loose_end(kind: "approval", id)` to +withdraw an approval that hasn't been acted on yet (closes #250). +The row transitions to `ApprovalStatus::Cancelled` (distinct from +`Denied`/`Failed`), the dashboard pulls the card out of the +pending pane, and `ApprovalResolved { status: "cancelled" }` fires +on the manager + dashboard channels. Approvals that have already +been approved/denied/failed return an error — the resolution is +final once the operator (or a lifecycle failure) acted on the row. + +Sub-agent surface refuses the `approval` kind with a clear error: +sub-agents don't submit approvals, so they have nothing of their +own to withdraw. Manager-only. + `InitConfig` approvals are the first step in a two-step spawn flow. On approve, hive-c0re seeds the proposed config repo with a default `agent.nix` template and sends the manager diff --git a/frontend/packages/dashboard/src/dashboard.css b/frontend/packages/dashboard/src/dashboard.css index af111b09..d8339480 100644 --- a/frontend/packages/dashboard/src/dashboard.css +++ b/frontend/packages/dashboard/src/dashboard.css @@ -656,9 +656,11 @@ code { .status-approved { color: var(--green); } .status-denied { color: var(--red); } .status-failed { color: var(--amber); } +.status-cancelled { color: var(--muted); } .glyph-approved { color: var(--green); } .glyph-denied { color: var(--red); } .glyph-failed { color: var(--amber); } +.glyph-cancelled { color: var(--muted); } .meta-inputs { list-style: none; padding: 0; diff --git a/frontend/packages/dashboard/src/tabs.js b/frontend/packages/dashboard/src/tabs.js index ff16ce67..dae04d2f 100644 --- a/frontend/packages/dashboard/src/tabs.js +++ b/frontend/packages/dashboard/src/tabs.js @@ -1598,6 +1598,7 @@ window.marked = marked; const row = el('div', { class: 'row' }); const glyph = a.status === 'approved' ? '✓' : a.status === 'denied' ? '✗' + : a.status === 'cancelled' ? '⊘' : '⚠'; row.append( el('span', { class: 'glyph glyph-' + a.status }, glyph), ' ', diff --git a/hive-ag3nt/src/mcp.rs b/hive-ag3nt/src/mcp.rs index 1a28c02b..63593d97 100644 --- a/hive-ag3nt/src/mcp.rs +++ b/hive-ag3nt/src/mcp.rs @@ -261,8 +261,10 @@ fn parse_loose_end_kind(raw: &str) -> Result Ok(hive_sh4re::CancelLooseEndKind::Question), "reminder" | "r" => Ok(hive_sh4re::CancelLooseEndKind::Reminder), + "approval" | "a" => Ok(hive_sh4re::CancelLooseEndKind::Approval), other => Err(format!( - "cancel_loose_end: unknown kind '{other}' (expected \"question\" or \"reminder\")" + "cancel_loose_end: unknown kind '{other}' \ + (expected \"question\", \"reminder\", or \"approval\")" )), } } @@ -275,6 +277,7 @@ fn loose_end_kind_label(kind: hive_sh4re::CancelLooseEndKind) -> &'static str { match kind { hive_sh4re::CancelLooseEndKind::Question => "question", hive_sh4re::CancelLooseEndKind::Reminder => "reminder", + hive_sh4re::CancelLooseEndKind::Approval => "approval", } } @@ -1628,11 +1631,14 @@ impl ManagerServer { #[tool( description = "Cancel any open thread in the swarm — a `question` (cancels \ - with the operator-override sentinel so the asker unblocks) or a `reminder` \ - (hard-deleted before fire). `kind` is `\"question\"` or `\"reminder\"`; `id` \ - is the row id from `get_loose_ends` or the original submission reply. \ - Manager surface bypasses the owner check on the sub-agent flavour — use for \ - hive-wide cleanup of stuck or stale threads." + with the operator-override sentinel so the asker unblocks), a `reminder` \ + (hard-deleted before fire), or an `approval` (withdraws a pending approval \ + you submitted; the dashboard pulls the card from pending and the row resolves \ + as `cancelled` instead of approved/denied/failed — closes #250). `kind` is \ + `\"question\"`, `\"reminder\"`, or `\"approval\"`; `id` is the row id from \ + `get_loose_ends` or the original submission reply. Manager surface bypasses \ + the owner check on the sub-agent flavour — use for hive-wide cleanup of \ + stuck or stale threads, or to drop your own approvals that got superseded." )] async fn cancel_loose_end(&self, Parameters(args): Parameters) -> String { let log = format!("{args:?}"); diff --git a/hive-c0re/src/approvals.rs b/hive-c0re/src/approvals.rs index 279a8eff..957fff1f 100644 --- a/hive-c0re/src/approvals.rs +++ b/hive-c0re/src/approvals.rs @@ -132,7 +132,7 @@ impl Approvals { let mut stmt = conn.prepare( "SELECT id, agent, kind, commit_ref, requested_at, status, resolved_at, note, fetched_sha, description FROM approvals - WHERE status IN ('approved', 'denied', 'failed') + WHERE status IN ('approved', 'denied', 'failed', 'cancelled') ORDER BY resolved_at DESC, id DESC LIMIT ?1", )?; @@ -246,6 +246,67 @@ impl Approvals { Ok(()) } + /// Withdraw a pending approval (closes #250). Returns the now-updated + /// row so the caller can emit `ApprovalResolved` with the right + /// kind / agent / sha. Errors if the approval isn't pending — once + /// it's approved/denied/failed/cancelled, the resolution is final. + pub fn mark_cancelled(&self, id: i64, canceller: &str) -> Result { + let mut conn = self.conn.lock().unwrap(); + let tx = conn.transaction()?; + let row: Option<( + String, + String, + String, + i64, + String, + Option, + Option, + )> = tx + .query_row( + "SELECT agent, kind, commit_ref, requested_at, status, fetched_sha, description + FROM approvals WHERE id = ?1", + params![id], + |row| { + Ok(( + row.get(0)?, + row.get(1)?, + row.get(2)?, + row.get(3)?, + row.get(4)?, + row.get(5)?, + row.get(6)?, + )) + }, + ) + .optional()?; + let Some((agent, kind, commit_ref, requested_at, status, fetched_sha, description)) = row + else { + bail!("approval {id} not found"); + }; + if status != "pending" { + bail!("approval {id} is {status}, not pending"); + } + let resolved_at = now_unix(); + let note = format!("cancelled by {canceller}"); + tx.execute( + "UPDATE approvals SET status = 'cancelled', resolved_at = ?1, note = ?2 WHERE id = ?3", + params![resolved_at, note, id], + )?; + tx.commit()?; + Ok(Approval { + id, + agent, + kind: kind_from_str(&kind)?, + commit_ref, + requested_at, + status: ApprovalStatus::Cancelled, + resolved_at: Some(resolved_at), + note: Some(note), + fetched_sha, + description, + }) + } + /// Mark every pending approval for `agent` as failed (returns rows affected). /// Used by `destroy` to clear the queue of an agent that no longer exists. pub fn fail_pending_for_agent(&self, agent: &str, note: &str) -> Result { @@ -301,6 +362,7 @@ fn row_to_approval(row: &rusqlite::Row<'_>) -> rusqlite::Result { "approved" => ApprovalStatus::Approved, "denied" => ApprovalStatus::Denied, "failed" => ApprovalStatus::Failed, + "cancelled" => ApprovalStatus::Cancelled, other => { return Err(rusqlite::Error::FromSqlConversionFailure( 5, @@ -323,7 +385,11 @@ fn row_to_approval(row: &rusqlite::Row<'_>) -> rusqlite::Result { }) } -fn kind_to_str(kind: ApprovalKind) -> &'static str { +/// Stable kind→str mapping used wherever we emit `ApprovalResolved` +/// or persist a kind to sqlite. `pub(crate)` so callers like +/// `questions::handle_cancel_loose_end` don't have to duplicate the +/// match; bumping a kind here is the single source of truth. +pub(crate) fn kind_to_str(kind: ApprovalKind) -> &'static str { match kind { ApprovalKind::ApplyCommit => "apply_commit", ApprovalKind::Spawn => "spawn", @@ -394,6 +460,41 @@ mod tests { assert_eq!(pending.len(), 3, "all three kinds must be visible"); } + #[test] + fn mark_cancelled_transitions_pending_row() { + // #250: manager withdraws a pending approval. Row leaves + // pending(), shows up in recent_resolved() with the cancelled + // status + a "cancelled by " note. + let (_dir, _path, db) = open_temp(); + let id = db + .submit_kind("bitburner", ApprovalKind::ApplyCommit, "cafef00d", Some("test")) + .unwrap(); + let row = db.mark_cancelled(id, "manager").expect("cancel"); + assert_eq!(row.id, id); + assert!(matches!(row.status, ApprovalStatus::Cancelled)); + assert_eq!(row.note.as_deref(), Some("cancelled by manager")); + assert!(row.resolved_at.is_some()); + assert!(db.pending().unwrap().is_empty(), "row leaves pending"); + let resolved = db.recent_resolved(10).unwrap(); + assert_eq!(resolved.len(), 1); + assert!(matches!(resolved[0].status, ApprovalStatus::Cancelled)); + } + + #[test] + fn mark_cancelled_refuses_already_resolved_row() { + // Once approved/denied/failed/cancelled the resolution is + // final — re-cancelling errors instead of silently overwriting. + let (_dir, _path, db) = open_temp(); + let id = db + .submit_kind("a", ApprovalKind::Spawn, "deadbeef", None) + .unwrap(); + db.mark_cancelled(id, "manager").expect("first cancel"); + let err = db + .mark_cancelled(id, "manager") + .expect_err("second cancel must fail"); + assert!(err.to_string().contains("not pending"), "{err}"); + } + #[test] fn unknown_kind_row_is_skipped_not_fatal() { // A single malformed / future-kind row must not blank the diff --git a/hive-c0re/src/dashboard.rs b/hive-c0re/src/dashboard.rs index 86ef4f6d..0a79bf11 100644 --- a/hive-c0re/src/dashboard.rs +++ b/hive-c0re/src/dashboard.rs @@ -602,6 +602,7 @@ fn history_view(a: Approval) -> ApprovalHistoryView { hive_sh4re::ApprovalStatus::Approved => "approved", hive_sh4re::ApprovalStatus::Denied => "denied", hive_sh4re::ApprovalStatus::Failed => "failed", + hive_sh4re::ApprovalStatus::Cancelled => "cancelled", // Pending shouldn't appear in recent_resolved, but be defensive. hive_sh4re::ApprovalStatus::Pending => "pending", }; diff --git a/hive-c0re/src/questions.rs b/hive-c0re/src/questions.rs index c2ddcb6e..b634d1f8 100644 --- a/hive-c0re/src/questions.rs +++ b/hive-c0re/src/questions.rs @@ -18,6 +18,7 @@ use std::sync::Arc; +use crate::approvals::kind_to_str; use crate::coordinator::Coordinator; use crate::limits; use crate::manager_server::spawn_question_watchdog; @@ -179,6 +180,35 @@ pub fn handle_cancel_loose_end( tracing::info!(%id, %canceller, %owner, "reminder cancelled"); Ok(()) } + hive_sh4re::CancelLooseEndKind::Approval => { + // Manager-only: only the agent that can submit approvals + // is allowed to withdraw them. Sub-agents would have no + // pending approvals of their own to cancel anyway. + if canceller != hive_sh4re::MANAGER_AGENT { + return Err( + "cancel_loose_end: only the manager can cancel approval rows".to_owned(), + ); + } + let approval = coord + .approvals + .mark_cancelled(id, canceller) + .map_err(|e| format!("{e:#}"))?; + tracing::info!(%id, %canceller, agent = %approval.agent, "approval cancelled"); + let sha_short = approval + .fetched_sha + .as_deref() + .map(|s| s[..s.len().min(12)].to_owned()); + coord.emit_approval_resolved( + approval.id, + &approval.agent, + kind_to_str(approval.kind), + sha_short, + "cancelled", + approval.note, + approval.description, + ); + Ok(()) + } } } diff --git a/hive-sh4re/src/lib.rs b/hive-sh4re/src/lib.rs index 684a9496..d7e447db 100644 --- a/hive-sh4re/src/lib.rs +++ b/hive-sh4re/src/lib.rs @@ -136,6 +136,11 @@ pub enum ApprovalStatus { Approved, Denied, Failed, + /// Manager withdrew the request before the operator acted on it + /// (closes #250). Distinct from `Denied` (operator decision) and + /// `Failed` (post-approval lifecycle error) so the dashboard can + /// chip / sort cancellations separately. + Cancelled, } /// Reminder activity statistics for an agent over a time window. @@ -271,10 +276,10 @@ pub enum ReminderTiming { /// as a short bulleted list — the per-row fields are all the context /// needed without a follow-up fetch. /// -/// `Question` and `Reminder` rows are cancellable via the -/// `CancelLooseEnd` request (and the `cancel_loose_end` MCP tool); -/// `Approval` rows are not (operator approves/denies via the -/// dashboard, manager has no withdraw path today). +/// All three variants are cancellable via `CancelLooseEnd` / +/// `cancel_loose_end`. `Question` and `Reminder` can be cancelled +/// from either surface (subject to ownership checks); `Approval` +/// is manager-only since sub-agents can't submit approvals. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "snake_case")] pub enum LooseEnd { @@ -325,15 +330,20 @@ pub enum LooseEnd { } /// Kind discriminator for `CancelLooseEnd`. Maps to which underlying -/// store the dispatcher reaches into (`OperatorQuestions` vs -/// `Broker::reminders`). Approvals are deliberately not cancellable -/// — the operator approves/denies via the dashboard, manager has no -/// withdraw path today. +/// store the dispatcher reaches into (`OperatorQuestions` / +/// `Broker::reminders` / `Approvals`). The `Approval` variant is +/// manager-only — sub-agents can't submit approvals so they have +/// nothing to withdraw (closes #250). #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "snake_case")] pub enum CancelLooseEndKind { Question, Reminder, + /// Withdraw a pending approval (manager surface only). The row + /// transitions to `ApprovalStatus::Cancelled` and an + /// `ApprovalResolved` event fires so the dashboard pulls the card + /// out of the pending pane. + Approval, } /// Requests on a per-agent socket. The agent's identity is the socket