hive-c0re: scope list_schedules to what the requester can actually act on
handle_list_schedules took no requester and returned every schedule unfiltered, unlike cancel_schedule/edit_schedule/fire_schedule_now which all gate on the shared ownership predicate (self, operator, or subtree via topology::is_descendant_of). list_schedules now filters through the same predicate, renamed cancel_authorized -> schedule_authorized since it backs all four verbs now, not just cancel. Fixed five stale 'every schedule' / 'unfiltered' claims found while in here: filter_ghost_schedule_targets's doc comment, the list_schedules MCP tool description, docs/tools/scheduling.md's per-verb section (already self-contradicting its own top-of-file subtree-scoping claim before this fix), and hive-core-agent-sock's ListSchedules/Schedules wire-type doc comments (including a stale '(privileged)' marker from the pre-topology-subtree model). Credit to atlas: independently found the same fix while finishing PR #4233 (which documents this bug per mara's 'fix it, don't document it' ruling) and caught two stale doc spots I'd missed (hive-core-agent-sock's comments) plus proposed the schedule_authorized rename. Compared diffs directly before either of us pushed; he dropped his scheduling.rs changes so we didn't collide. fixes #4237
This commit is contained in:
parent
2ec5c9433f
commit
79c43a15d8
5 changed files with 75 additions and 31 deletions
|
|
@ -47,10 +47,13 @@ consumes one-shot schedules and cancels them afterwards.
|
||||||
|
|
||||||
### `list_schedules()`
|
### `list_schedules()`
|
||||||
|
|
||||||
Snapshot every schedule (active + cancelled-but-not-reaped): id,
|
Snapshot the schedules you're authorized to see (active, and cancelled
|
||||||
owner, body, per-target `last_fired_at` + `last_result`,
|
but not yet reaped) — same read scope as the rest of this group: your
|
||||||
`next_fire_at_unix`, `interval_seconds`. Use to look up an id before
|
own, plus any owned by a sub-agent in your subtree (everything, for
|
||||||
cancelling, or to audit upcoming wake-ups across the swarm.
|
the operator). Returns id, owner, body, per-target `last_fired_at`
|
||||||
|
and `last_result`, `next_fire_at_unix`, `interval_seconds`. Use to
|
||||||
|
look up an id before cancelling, or to audit upcoming wake-ups in
|
||||||
|
your subtree.
|
||||||
|
|
||||||
## `diagnostics` tool group
|
## `diagnostics` tool group
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -977,11 +977,12 @@ impl AgentServer {
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tool(
|
#[tool(
|
||||||
description = "List every scheduled prompt in the queue (active + cancelled but \
|
description = "List the scheduled prompts you're authorized to see — your own, plus \
|
||||||
not yet reaped). Returns the full snapshot — schedule id, owner, body, target set \
|
any owned by a sub-agent in your subtree (everything, for the operator) — active and \
|
||||||
with per-target last_fired_at + last_result, next fire time, recurring interval. \
|
cancelled-but-not-yet-reaped. Returns schedule id, owner, body, target set with \
|
||||||
|
per-target last_fired_at + last_result, next fire time, recurring interval. \
|
||||||
Use this to look up an id before calling `cancel_schedule`, or to audit what \
|
Use this to look up an id before calling `cancel_schedule`, or to audit what \
|
||||||
the swarm is going to be woken up about next."
|
your subtree is going to be woken up about next."
|
||||||
)]
|
)]
|
||||||
async fn list_schedules(&self) -> String {
|
async fn list_schedules(&self) -> String {
|
||||||
run_tool_envelope("list_schedules", String::new(), async move {
|
run_tool_envelope("list_schedules", String::new(), async move {
|
||||||
|
|
|
||||||
|
|
@ -633,7 +633,7 @@ async fn dispatch_orchestration(req: &Request, agent: &str, coord: &Arc<Coordina
|
||||||
if let Some(err) = require_group(agent, "scheduling", "list schedules") {
|
if let Some(err) = require_group(agent, "scheduling", "list schedules") {
|
||||||
return err;
|
return err;
|
||||||
}
|
}
|
||||||
handle_list_schedules(coord)
|
handle_list_schedules(coord, agent)
|
||||||
}
|
}
|
||||||
Request::FireScheduleNow { id } => {
|
Request::FireScheduleNow { id } => {
|
||||||
if let Some(err) = require_group(agent, "scheduling", "fire a schedule") {
|
if let Some(err) = require_group(agent, "scheduling", "fire a schedule") {
|
||||||
|
|
|
||||||
|
|
@ -10,11 +10,20 @@ use hive_core_agent_sock::Response;
|
||||||
|
|
||||||
use crate::coordinator::Coordinator;
|
use crate::coordinator::Coordinator;
|
||||||
|
|
||||||
/// `ListSchedules` — snapshot every scheduled prompt onto the wire.
|
/// `ListSchedules` — snapshot the scheduled prompts `requester` is
|
||||||
pub(super) fn handle_list_schedules(coord: &Arc<Coordinator>) -> Response {
|
/// authorized to see: its own schedules, ones owned by an agent in its
|
||||||
|
/// subtree, or (for the operator) everything. Uses the same
|
||||||
|
/// `schedule_authorized` rule `CancelSchedule`/`EditSchedule`/
|
||||||
|
/// `FireScheduleNow` already enforce, applied here as a per-row filter
|
||||||
|
/// instead of a hard reject.
|
||||||
|
pub(super) fn handle_list_schedules(coord: &Arc<Coordinator>, requester: &str) -> Response {
|
||||||
match coord.scheduled_prompts.list() {
|
match coord.scheduled_prompts.list() {
|
||||||
Ok(schedules) => Response::Schedules {
|
Ok(schedules) => Response::Schedules {
|
||||||
schedules: schedules.into_iter().map(schedule_to_wire).collect(),
|
schedules: schedules
|
||||||
|
.into_iter()
|
||||||
|
.filter(|s| schedule_authorized(requester, &s.owner))
|
||||||
|
.map(schedule_to_wire)
|
||||||
|
.collect(),
|
||||||
},
|
},
|
||||||
Err(e) => Response::Err {
|
Err(e) => Response::Err {
|
||||||
message: format!("list scheduled prompts: {e:#}"),
|
message: format!("list scheduled prompts: {e:#}"),
|
||||||
|
|
@ -115,7 +124,7 @@ pub(super) fn handle_cancel_schedule(
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
if !cancel_authorized(requester, &schedule.owner) {
|
if !schedule_authorized(requester, &schedule.owner) {
|
||||||
return Response::Err {
|
return Response::Err {
|
||||||
message: format!(
|
message: format!(
|
||||||
"not authorized: {requester} cannot cancel schedule owned by {owner}",
|
"not authorized: {requester} cannot cancel schedule owned by {owner}",
|
||||||
|
|
@ -165,7 +174,7 @@ pub(super) async fn handle_fire_schedule_now(
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
if !cancel_authorized(requester, &schedule.owner) {
|
if !schedule_authorized(requester, &schedule.owner) {
|
||||||
return Response::Err {
|
return Response::Err {
|
||||||
message: format!(
|
message: format!(
|
||||||
"not authorized: {requester} cannot fire schedule owned by {owner}",
|
"not authorized: {requester} cannot fire schedule owned by {owner}",
|
||||||
|
|
@ -239,7 +248,7 @@ pub(super) fn handle_edit_schedule(
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
if !cancel_authorized(requester, &schedule.owner) {
|
if !schedule_authorized(requester, &schedule.owner) {
|
||||||
return Response::Err {
|
return Response::Err {
|
||||||
message: format!(
|
message: format!(
|
||||||
"not authorized: {requester} cannot edit schedule owned by {owner}",
|
"not authorized: {requester} cannot edit schedule owned by {owner}",
|
||||||
|
|
@ -266,21 +275,22 @@ pub(super) fn handle_edit_schedule(
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Permission check for `CancelSchedule` on the manager surface.
|
/// Permission check shared by the whole scheduling surface: `requester`
|
||||||
/// `requester` (always `ruth` here) can cancel its own schedules.
|
/// can act on a schedule it owns, on one owned by an agent in its
|
||||||
/// Sub-agent ownership is delegated to topology — see
|
/// subtree (delegated to topology — see `crate::topology::is_descendant_of`),
|
||||||
/// `crate::topology::is_descendant_of`. Also reused by
|
/// or, as the operator, on anything. `CancelSchedule`/`EditSchedule`/
|
||||||
/// `handle_fire_schedule_now` — fire-auth follows the same shape.
|
/// `FireScheduleNow` apply it as a hard reject; `ListSchedules` applies
|
||||||
fn cancel_authorized(requester: &str, owner: &str) -> bool {
|
/// it as a per-row filter.
|
||||||
|
fn schedule_authorized(requester: &str, owner: &str) -> bool {
|
||||||
if requester == owner {
|
if requester == owner {
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
if requester == hive_sh4re::manager::OPERATOR_RECIPIENT {
|
if requester == hive_sh4re::manager::OPERATOR_RECIPIENT {
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
// Manager can cancel anything owned by an agent in its subtree.
|
// Manager can act on anything owned by an agent in its subtree. For
|
||||||
// For the current single-manager topology that covers everything,
|
// the current single-manager topology that covers everything, but
|
||||||
// but the check stays correct as the tree grows.
|
// the check stays correct as the tree grows.
|
||||||
crate::topology::is_descendant_of(owner, requester)
|
crate::topology::is_descendant_of(owner, requester)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -302,11 +312,12 @@ pub fn schedule_to_wire_public(
|
||||||
/// last `nixos-container list` scan (stopped agents included, destroyed
|
/// last `nixos-container list` scan (stopped agents included, destroyed
|
||||||
/// ones absent); the `operator` pseudo-target is always retained since
|
/// ones absent); the `operator` pseudo-target is always retained since
|
||||||
/// it isn't a container. Applied only to the dashboard wire paths
|
/// it isn't a container. Applied only to the dashboard wire paths
|
||||||
/// (`api_schedules` + the `SchedulesChanged` SSE emit) — the
|
/// (`api_schedules` + the `SchedulesChanged` SSE emit) — `list_schedules`
|
||||||
/// manager-facing `list_schedules` stays unfiltered so agents can still
|
/// doesn't apply this filter (it still returns every live target, ghost
|
||||||
/// see and cancel stale targets. This is a view filter: the underlying
|
/// or not, on the schedules the requester is authorized to see) so an
|
||||||
/// schedule rows keep every target, so a re-spawned agent's targets
|
/// agent can still see and cancel its own stale targets. This is a view
|
||||||
/// reappear on their own.
|
/// filter: the underlying schedule rows keep every target, so a
|
||||||
|
/// re-spawned agent's targets reappear on their own.
|
||||||
pub(crate) fn filter_ghost_schedule_targets(
|
pub(crate) fn filter_ghost_schedule_targets(
|
||||||
schedules: &mut [hive_sh4re::schedule::WireSchedule],
|
schedules: &mut [hive_sh4re::schedule::WireSchedule],
|
||||||
live: &std::collections::HashSet<String>,
|
live: &std::collections::HashSet<String>,
|
||||||
|
|
@ -379,6 +390,32 @@ mod tests {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn schedule_authorized_self_is_true() {
|
||||||
|
// Disk-free branch — never reaches `is_descendant_of`.
|
||||||
|
assert!(schedule_authorized("iris", "iris"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn schedule_authorized_operator_is_true() {
|
||||||
|
// Disk-free branch — never reaches `is_descendant_of`.
|
||||||
|
assert!(schedule_authorized(
|
||||||
|
hive_sh4re::manager::OPERATOR_RECIPIENT,
|
||||||
|
"iris"
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn schedule_authorized_unrelated_requester_is_false() {
|
||||||
|
// Control: neither self nor operator, and no on-disk topology in
|
||||||
|
// a test sandbox connects these two names — pins that the
|
||||||
|
// predicate isn't accidentally stuck at `true`.
|
||||||
|
assert!(!schedule_authorized(
|
||||||
|
"definitely-not-the-owner",
|
||||||
|
"also-not-the-requester"
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn ghost_filter_drops_dead_agents_keeps_live_and_operator() {
|
fn ghost_filter_drops_dead_agents_keeps_live_and_operator() {
|
||||||
let live: std::collections::HashSet<String> = ["iris".to_owned(), "damocles".to_owned()]
|
let live: std::collections::HashSet<String> = ["iris".to_owned(), "damocles".to_owned()]
|
||||||
|
|
|
||||||
|
|
@ -191,7 +191,9 @@ pub enum Request {
|
||||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
targets: Option<Vec<String>>,
|
targets: Option<Vec<String>>,
|
||||||
},
|
},
|
||||||
/// *(privileged)* List every schedule in the queue.
|
/// List the schedules the requester is authorized to see: its own,
|
||||||
|
/// plus any owned by an agent in its subtree (everything, for the
|
||||||
|
/// operator).
|
||||||
ListSchedules,
|
ListSchedules,
|
||||||
/// List the calling agent's subtree — children, their children, and so
|
/// List the calling agent's subtree — children, their children, and so
|
||||||
/// on down, plus the caller itself, which is part of its own subtree.
|
/// on down, plus the caller itself, which is part of its own subtree.
|
||||||
|
|
@ -294,7 +296,8 @@ pub enum Response {
|
||||||
/// requested filters. Returned on the agent socket when the agent
|
/// requested filters. Returned on the agent socket when the agent
|
||||||
/// holds the `read_host_journal` capability.
|
/// holds the `read_host_journal` capability.
|
||||||
HostJournal { content: String },
|
HostJournal { content: String },
|
||||||
/// `ListSchedules` result. Snapshot of every schedule.
|
/// `ListSchedules` result. Snapshot of the schedules the requester is
|
||||||
|
/// authorized to see — see `ListSchedules`'s own doc comment.
|
||||||
/// Returned on the manager socket only.
|
/// Returned on the manager socket only.
|
||||||
Schedules { schedules: Vec<WireSchedule> },
|
Schedules { schedules: Vec<WireSchedule> },
|
||||||
/// `CreateRepo` result: the new repo's full name (`agents/<repo>`)
|
/// `CreateRepo` result: the new repo's full name (`agents/<repo>`)
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue