diff --git a/docs/approvals.md b/docs/approvals.md index fa75fa16..e587add0 100644 --- a/docs/approvals.md +++ b/docs/approvals.md @@ -63,21 +63,6 @@ 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 d8339480..af111b09 100644 --- a/frontend/packages/dashboard/src/dashboard.css +++ b/frontend/packages/dashboard/src/dashboard.css @@ -656,11 +656,9 @@ 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 dae04d2f..ff16ce67 100644 --- a/frontend/packages/dashboard/src/tabs.js +++ b/frontend/packages/dashboard/src/tabs.js @@ -1598,7 +1598,6 @@ 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 63593d97..1a28c02b 100644 --- a/hive-ag3nt/src/mcp.rs +++ b/hive-ag3nt/src/mcp.rs @@ -261,10 +261,8 @@ 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\", \"reminder\", or \"approval\")" + "cancel_loose_end: unknown kind '{other}' (expected \"question\" or \"reminder\")" )), } } @@ -277,7 +275,6 @@ 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", } } @@ -1631,14 +1628,11 @@ impl ManagerServer { #[tool( description = "Cancel any open thread in the swarm — a `question` (cancels \ - 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." + 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." )] 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 957fff1f..279a8eff 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', 'cancelled') + WHERE status IN ('approved', 'denied', 'failed') ORDER BY resolved_at DESC, id DESC LIMIT ?1", )?; @@ -246,67 +246,6 @@ 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 { @@ -362,7 +301,6 @@ 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, @@ -385,11 +323,7 @@ fn row_to_approval(row: &rusqlite::Row<'_>) -> rusqlite::Result { }) } -/// 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 { +fn kind_to_str(kind: ApprovalKind) -> &'static str { match kind { ApprovalKind::ApplyCommit => "apply_commit", ApprovalKind::Spawn => "spawn", @@ -460,41 +394,6 @@ 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 0a79bf11..86ef4f6d 100644 --- a/hive-c0re/src/dashboard.rs +++ b/hive-c0re/src/dashboard.rs @@ -602,7 +602,6 @@ 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 339d377c..c2ddcb6e 100644 --- a/hive-c0re/src/questions.rs +++ b/hive-c0re/src/questions.rs @@ -18,7 +18,6 @@ use std::sync::Arc; -use crate::approvals::kind_to_str; use crate::coordinator::Coordinator; use crate::limits; use crate::manager_server::spawn_question_watchdog; @@ -180,72 +179,6 @@ 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. - check_approval_canceller_is_manager(canceller)?; - 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(()) - } - } -} - -/// Manager-only guard on the `Approval` cancel arm. Pulled out so -/// the auth check has its own focused unit test (argus nit on #508) -/// — testing the full `handle_cancel_loose_end` flow would need a -/// `Coordinator` fixture (broker + sqlite + in-memory questions), -/// which we don't have today. The check is a single string compare, -/// so a function-level test gives the same coverage with no harness. -fn check_approval_canceller_is_manager(canceller: &str) -> Result<(), String> { - if canceller != hive_sh4re::MANAGER_AGENT { - return Err("cancel_loose_end: only the manager can cancel approval rows".to_owned()); - } - Ok(()) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn approval_cancel_rejects_sub_agent_callers() { - // Argus nit on #508: sub-agents must not be able to cancel - // approval rows even if they invent an id. The guard is - // server-side so client cooperation is irrelevant. - let err = check_approval_canceller_is_manager("bitburner").unwrap_err(); - assert!(err.contains("only the manager"), "{err}"); - // Bonus: empty / operator strings also rejected (only the - // exact MANAGER_AGENT constant passes). - assert!(check_approval_canceller_is_manager("").is_err()); - assert!( - check_approval_canceller_is_manager(hive_sh4re::OPERATOR_RECIPIENT) - .is_err(), - "operator surface uses the dashboard cancel path, not this dispatcher", - ); - } - - #[test] - fn approval_cancel_allows_manager() { - check_approval_canceller_is_manager(hive_sh4re::MANAGER_AGENT) - .expect("MANAGER_AGENT must pass the guard"); } } diff --git a/hive-sh4re/src/lib.rs b/hive-sh4re/src/lib.rs index d7e447db..684a9496 100644 --- a/hive-sh4re/src/lib.rs +++ b/hive-sh4re/src/lib.rs @@ -136,11 +136,6 @@ 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. @@ -276,10 +271,10 @@ pub enum ReminderTiming { /// as a short bulleted list — the per-row fields are all the context /// needed without a follow-up fetch. /// -/// 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. +/// `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). #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "snake_case")] pub enum LooseEnd { @@ -330,20 +325,15 @@ pub enum LooseEnd { } /// Kind discriminator for `CancelLooseEnd`. Maps to which underlying -/// 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). +/// 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. #[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