approvals: manager can withdraw pending approvals (closes #250)
This commit is contained in:
parent
f866f1822e
commit
c1f27e3b7b
8 changed files with 182 additions and 16 deletions
|
|
@ -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<Approval> {
|
||||
let mut conn = self.conn.lock().unwrap();
|
||||
let tx = conn.transaction()?;
|
||||
let row: Option<(
|
||||
String,
|
||||
String,
|
||||
String,
|
||||
i64,
|
||||
String,
|
||||
Option<String>,
|
||||
Option<String>,
|
||||
)> = 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<usize> {
|
||||
|
|
@ -301,6 +362,7 @@ fn row_to_approval(row: &rusqlite::Row<'_>) -> rusqlite::Result<Approval> {
|
|||
"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<Approval> {
|
|||
})
|
||||
}
|
||||
|
||||
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 <who>" 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
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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(())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue