scheduled prompts: edit existing schedule (closes #474)
This commit is contained in:
parent
0072946ea3
commit
9fee1a1e56
6 changed files with 428 additions and 0 deletions
|
|
@ -75,6 +75,10 @@ pub async fn serve(port: u16, coord: Arc<Coordinator>) -> Result<()> {
|
|||
.route("/op-send", post(post_op_send))
|
||||
.route("/meta-update", post(post_meta_update))
|
||||
.route("/api/schedules", get(api_schedules).post(post_schedule_new))
|
||||
.route(
|
||||
"/api/schedules/{id}",
|
||||
axum::routing::patch(patch_schedule),
|
||||
)
|
||||
.route("/api/schedules/{id}/cancel", post(post_schedule_cancel))
|
||||
.route("/api/schedules/{id}/fire-now", post(post_schedule_fire_now))
|
||||
.route("/api/rebuild-queue/{id}/cancel", post(post_rebuild_queue_cancel))
|
||||
|
|
@ -1489,6 +1493,69 @@ struct CancelScheduleForm {
|
|||
targets: Option<Vec<String>>,
|
||||
}
|
||||
|
||||
#[derive(serde::Deserialize, Default)]
|
||||
struct EditScheduleForm {
|
||||
#[serde(default)]
|
||||
body: Option<String>,
|
||||
/// Double-`Option` semantics on the wire: missing key = leave
|
||||
/// alone, explicit `null` = clear, value = set. serde's
|
||||
/// `deserialize_with` trick to distinguish missing from null:
|
||||
/// we wrap each editable field in its own helper. Simpler
|
||||
/// here — keep them plain `Option<Option<_>>` and document
|
||||
/// that the dashboard caller passes JSON `null` to clear.
|
||||
#[serde(default, deserialize_with = "deserialize_some")]
|
||||
description: Option<Option<String>>,
|
||||
#[serde(default, deserialize_with = "deserialize_some")]
|
||||
interval_seconds: Option<Option<u64>>,
|
||||
#[serde(default)]
|
||||
next_fire_at_unix: Option<i64>,
|
||||
}
|
||||
|
||||
/// serde adaptor: turns missing-key into `None`, explicit-null
|
||||
/// into `Some(None)`, value into `Some(Some(v))`. Standard trick
|
||||
/// for distinguishing "field absent" from "field set to null" in
|
||||
/// JSON PATCH bodies.
|
||||
fn deserialize_some<'de, T, D>(deserializer: D) -> Result<Option<T>, D::Error>
|
||||
where
|
||||
T: serde::Deserialize<'de>,
|
||||
D: serde::Deserializer<'de>,
|
||||
{
|
||||
T::deserialize(deserializer).map(Some)
|
||||
}
|
||||
|
||||
/// `PATCH /api/schedules/{id}` — partial update of an existing
|
||||
/// schedule (#474). Mutable fields: `body`, `description`,
|
||||
/// `interval_seconds`, `next_fire_at_unix`. Targets stay
|
||||
/// immutable (per-target last-result history is keyed on them;
|
||||
/// the "I want different targets" workaround is cancel + submit
|
||||
/// a new schedule). JSON body uses missing-key = "leave alone",
|
||||
/// explicit null = "clear" for `description` + `interval_seconds`.
|
||||
/// Cancelled schedules are refused — submit a new one instead.
|
||||
/// Returns the updated `WireSchedule` so the caller's post-edit
|
||||
/// refresh has the new state inline.
|
||||
async fn patch_schedule(
|
||||
State(state): State<AppState>,
|
||||
AxumPath(id): AxumPath<i64>,
|
||||
axum::Json(form): axum::Json<EditScheduleForm>,
|
||||
) -> Response {
|
||||
let patch = crate::scheduled_prompts::UpdateSchedule {
|
||||
body: form.body,
|
||||
description: form.description,
|
||||
interval_seconds: form.interval_seconds,
|
||||
next_fire_at_unix: form.next_fire_at_unix,
|
||||
};
|
||||
if let Err(e) = state.coord.scheduled_prompts.update(id, patch) {
|
||||
return error_response(&format!("edit schedule {id}: {e:#}"));
|
||||
}
|
||||
match state.coord.scheduled_prompts.get(id) {
|
||||
Ok(Some(s)) => {
|
||||
axum::Json(crate::manager_server::schedule_to_wire_public(s)).into_response()
|
||||
}
|
||||
Ok(None) => error_response(&format!("edit schedule {id}: row vanished post-update")),
|
||||
Err(e) => error_response(&format!("re-read schedule {id}: {e:#}")),
|
||||
}
|
||||
}
|
||||
|
||||
/// `POST /api/schedules/{id}/cancel` — operator-side cancel
|
||||
/// (whole schedule when no `targets` field, partial when one is
|
||||
/// provided). Operator bypasses the topology check; the manager
|
||||
|
|
|
|||
|
|
@ -350,6 +350,21 @@ async fn dispatch(req: &ManagerRequest, coord: &Arc<Coordinator>) -> ManagerResp
|
|||
ManagerRequest::CancelSchedule { id, targets } => {
|
||||
handle_cancel_schedule(coord, hive_sh4re::MANAGER_AGENT, *id, targets.as_deref())
|
||||
}
|
||||
ManagerRequest::EditSchedule {
|
||||
id,
|
||||
body,
|
||||
description,
|
||||
interval_seconds,
|
||||
next_fire_at_unix,
|
||||
} => handle_edit_schedule(
|
||||
coord,
|
||||
hive_sh4re::MANAGER_AGENT,
|
||||
*id,
|
||||
body.clone(),
|
||||
description.clone(),
|
||||
*interval_seconds,
|
||||
*next_fire_at_unix,
|
||||
),
|
||||
ManagerRequest::ListSchedules => match coord.scheduled_prompts.list() {
|
||||
Ok(schedules) => ManagerResponse::Schedules {
|
||||
schedules: schedules.into_iter().map(schedule_to_wire).collect(),
|
||||
|
|
@ -863,6 +878,59 @@ async fn handle_fire_schedule_now(
|
|||
}
|
||||
}
|
||||
|
||||
/// Authorize + dispatch a `EditSchedule` patch (#474). Same
|
||||
/// ownership rules as `CancelSchedule` — the manager can edit
|
||||
/// schedules it owns + any owned by an agent in its subtree.
|
||||
/// Forwards the partial payload to
|
||||
/// `ScheduledPrompts::update` which enforces the cancelled-row
|
||||
/// + zero-interval validation. Returns `Ok` on a clean update;
|
||||
/// `Err` with the underlying message on any auth / validation
|
||||
/// failure so the dashboard can surface it verbatim.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
fn handle_edit_schedule(
|
||||
coord: &Arc<Coordinator>,
|
||||
requester: &str,
|
||||
schedule_id: i64,
|
||||
body: Option<String>,
|
||||
description: Option<Option<String>>,
|
||||
interval_seconds: Option<Option<u64>>,
|
||||
next_fire_at_unix: Option<i64>,
|
||||
) -> ManagerResponse {
|
||||
let schedule = match coord.scheduled_prompts.get(schedule_id) {
|
||||
Ok(Some(s)) => s,
|
||||
Ok(None) => {
|
||||
return ManagerResponse::Err {
|
||||
message: format!("schedule {schedule_id} not found"),
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
return ManagerResponse::Err {
|
||||
message: format!("read schedule {schedule_id}: {e:#}"),
|
||||
}
|
||||
}
|
||||
};
|
||||
if !cancel_authorized(requester, &schedule.owner) {
|
||||
return ManagerResponse::Err {
|
||||
message: format!(
|
||||
"not authorized: {requester} cannot edit schedule owned by {owner}",
|
||||
owner = schedule.owner
|
||||
),
|
||||
};
|
||||
}
|
||||
let patch = crate::scheduled_prompts::UpdateSchedule {
|
||||
body,
|
||||
description,
|
||||
interval_seconds,
|
||||
next_fire_at_unix,
|
||||
};
|
||||
match coord.scheduled_prompts.update(schedule_id, patch) {
|
||||
Ok(()) => ManagerResponse::Ok,
|
||||
Err(e) => ManagerResponse::Err {
|
||||
message: format!("edit schedule {schedule_id}: {e:#}"),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/// Permission check for `CancelSchedule` on the manager surface.
|
||||
/// `requester` (always `hm1nd` here) can cancel its own schedules.
|
||||
/// Sub-agent ownership is delegated to topology — see
|
||||
|
|
|
|||
|
|
@ -145,6 +145,22 @@ pub struct NewSchedule {
|
|||
pub source: ScheduleSource,
|
||||
}
|
||||
|
||||
/// Partial-update payload for `ScheduledPrompts::update` (#474).
|
||||
/// Every field is `Option<_>`; `None` keeps the existing value.
|
||||
/// The doubly-wrapped `Option<Option<u64>>` on `interval_seconds`
|
||||
/// is intentional: outer `None` = "don't touch", outer
|
||||
/// `Some(None)` = "set to NULL (toggle to one-shot)", outer
|
||||
/// `Some(Some(n))` = "set to n seconds." Same encoding as JSON
|
||||
/// "missing key" vs "explicit null" — the dashboard surface
|
||||
/// preserves the distinction.
|
||||
#[derive(Debug, Clone, Default)]
|
||||
pub struct UpdateSchedule {
|
||||
pub body: Option<String>,
|
||||
pub description: Option<Option<String>>,
|
||||
pub interval_seconds: Option<Option<u64>>,
|
||||
pub next_fire_at_unix: Option<i64>,
|
||||
}
|
||||
|
||||
pub struct ScheduledPrompts {
|
||||
conn: Mutex<Connection>,
|
||||
}
|
||||
|
|
@ -331,6 +347,67 @@ impl ScheduledPrompts {
|
|||
Ok(skipped)
|
||||
}
|
||||
|
||||
/// Partial-update an existing schedule's mutable fields (#474).
|
||||
/// Every field is `Option<_>`; `None` means "leave the existing
|
||||
/// value alone", `Some(_)` means "set it to this." Refuses
|
||||
/// cancelled rows (no point editing a tombstone — operator can
|
||||
/// just submit a new schedule). Refuses `interval_seconds = Some(0)`
|
||||
/// — same rule as submit-time validation. Targets stay
|
||||
/// immutable: per-target last-result history is keyed on
|
||||
/// (schedule_id, target); changing the target set would orphan
|
||||
/// or duplicate history rows. Operator workaround for "I want
|
||||
/// different targets" is cancel-target + submit a new schedule.
|
||||
pub fn update(&self, id: i64, patch: UpdateSchedule) -> Result<()> {
|
||||
// Pre-flight: row must exist and not be cancelled.
|
||||
let mut conn = self.conn.lock().unwrap();
|
||||
let cancelled: Option<i64> = conn
|
||||
.query_row(
|
||||
"SELECT cancelled_at_unix FROM scheduled_prompts WHERE id = ?1",
|
||||
params![id],
|
||||
|row| row.get(0),
|
||||
)
|
||||
.optional()?
|
||||
.ok_or_else(|| anyhow::anyhow!("schedule {id} not found"))?;
|
||||
if cancelled.is_some() {
|
||||
bail!("schedule {id} is cancelled — submit a new one");
|
||||
}
|
||||
if let Some(Some(0)) = patch.interval_seconds {
|
||||
bail!("interval_seconds must be > 0 (use None for one-shot)");
|
||||
}
|
||||
// Single transaction so a partial failure can't leave a
|
||||
// half-updated row.
|
||||
let tx = conn.transaction()?;
|
||||
if let Some(body) = patch.body {
|
||||
tx.execute(
|
||||
"UPDATE scheduled_prompts SET body = ?1 WHERE id = ?2",
|
||||
params![body, id],
|
||||
)?;
|
||||
}
|
||||
if let Some(description) = patch.description {
|
||||
tx.execute(
|
||||
"UPDATE scheduled_prompts SET description = ?1 WHERE id = ?2",
|
||||
params![description, id],
|
||||
)?;
|
||||
}
|
||||
if let Some(interval) = patch.interval_seconds {
|
||||
// `Some(Some(n))` = set to n. `Some(None)` = clear (one-shot).
|
||||
// `None` is filtered out by the outer `if let`.
|
||||
let value: Option<i64> = interval.map(i64::try_from).and_then(Result::ok);
|
||||
tx.execute(
|
||||
"UPDATE scheduled_prompts SET interval_seconds = ?1 WHERE id = ?2",
|
||||
params![value, id],
|
||||
)?;
|
||||
}
|
||||
if let Some(next) = patch.next_fire_at_unix {
|
||||
tx.execute(
|
||||
"UPDATE scheduled_prompts SET next_fire_at_unix = ?1 WHERE id = ?2",
|
||||
params![next, id],
|
||||
)?;
|
||||
}
|
||||
tx.commit()?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Delete a one-shot row after its single fire. Cascades the
|
||||
/// target rows via the FK constraint. Idempotent on a missing
|
||||
/// id.
|
||||
|
|
@ -638,6 +715,137 @@ mod tests {
|
|||
assert_eq!(bob.last_result.as_deref(), Some("ok"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_partial_only_touches_set_fields() {
|
||||
let (_dir, db) = open();
|
||||
let id = db
|
||||
.submit(NewSchedule {
|
||||
owner: "operator".into(),
|
||||
targets: vec!["alice".into()],
|
||||
body: "old body".into(),
|
||||
first_fire_at_unix: 100,
|
||||
interval_seconds: Some(60),
|
||||
description: Some("old desc".into()),
|
||||
source: ScheduleSource::Operator,
|
||||
})
|
||||
.expect("submit");
|
||||
// Only change body — everything else must stay.
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
body: Some("new body".into()),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("update body");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
assert_eq!(s.body, "new body");
|
||||
assert_eq!(s.description.as_deref(), Some("old desc"));
|
||||
assert_eq!(s.interval_seconds, Some(60));
|
||||
assert_eq!(s.next_fire_at_unix, 100);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_interval_toggle_recurring_to_one_shot() {
|
||||
let (_dir, db) = open();
|
||||
let id = db
|
||||
.submit(NewSchedule {
|
||||
owner: "operator".into(),
|
||||
targets: vec!["alice".into()],
|
||||
body: "x".into(),
|
||||
first_fire_at_unix: 100,
|
||||
interval_seconds: Some(60),
|
||||
description: None,
|
||||
source: ScheduleSource::Operator,
|
||||
})
|
||||
.expect("submit");
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
interval_seconds: Some(None),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("update");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
assert_eq!(s.interval_seconds, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_interval_toggle_one_shot_to_recurring() {
|
||||
let (_dir, db) = open();
|
||||
let id = submit_one_shot(&db, 100, &["alice"]);
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
interval_seconds: Some(Some(3600)),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("update");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
assert_eq!(s.interval_seconds, Some(3600));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_rejects_cancelled_row() {
|
||||
let (_dir, db) = open();
|
||||
let id = submit_one_shot(&db, 100, &["alice"]);
|
||||
db.cancel_all(id).expect("cancel");
|
||||
let err = db
|
||||
.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
body: Some("nope".into()),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.unwrap_err();
|
||||
assert!(format!("{err:#}").contains("cancelled"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_rejects_zero_interval() {
|
||||
let (_dir, db) = open();
|
||||
let id = submit_one_shot(&db, 100, &["alice"]);
|
||||
let err = db
|
||||
.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
interval_seconds: Some(Some(0)),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.unwrap_err();
|
||||
assert!(format!("{err:#}").contains("interval_seconds"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_clears_description() {
|
||||
let (_dir, db) = open();
|
||||
let id = db
|
||||
.submit(NewSchedule {
|
||||
owner: "operator".into(),
|
||||
targets: vec!["alice".into()],
|
||||
body: "x".into(),
|
||||
first_fire_at_unix: 100,
|
||||
interval_seconds: None,
|
||||
description: Some("first".into()),
|
||||
source: ScheduleSource::Operator,
|
||||
})
|
||||
.expect("submit");
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
description: Some(None),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("update");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
assert!(s.description.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_source_round_trips() {
|
||||
let (_dir, db) = open();
|
||||
|
|
|
|||
Loading…
Reference in a new issue