diff --git a/crates/gitlawb-node/src/api/mod.rs b/crates/gitlawb-node/src/api/mod.rs index df10175a9..ecfa07edc 100644 --- a/crates/gitlawb-node/src/api/mod.rs +++ b/crates/gitlawb-node/src/api/mod.rs @@ -22,6 +22,7 @@ pub mod replicas; pub mod repos; pub mod resolve; pub mod stars; +pub mod task_cursor; pub mod tasks; pub mod visibility; pub mod webhooks; diff --git a/crates/gitlawb-node/src/api/task_cursor.rs b/crates/gitlawb-node/src/api/task_cursor.rs new file mode 100644 index 000000000..c56f14836 --- /dev/null +++ b/crates/gitlawb-node/src/api/task_cursor.rs @@ -0,0 +1,629 @@ +//! Opaque, integrity-protected continuation tokens for the task read surfaces. +//! +//! The task list pages by keyset over `(created_at, id)`, but the rows a +//! caller may *see* are a filtered subset of the rows the query has to +//! *examine*. Handing the caller a raw `(created_at, id)` cursor therefore +//! forced a choice between two broken options (#327 review): anchor the cursor +//! on the last visible row, and a window of denied tasks longer than the scan +//! budget stalls paging forever; or anchor it on the last examined row, and a +//! denied read leaks the id and timestamp of a task `GET /tasks/{id}` other- +//! wise 404s. +//! +//! A server-issued token removes the choice. The position it carries is the +//! last *examined* candidate, so paging always advances, and the payload is +//! opaque and MAC'd, so the caller learns nothing from it and cannot forge one +//! naming a row of their choosing. +//! +//! The position must be *confidential*, not merely unforgeable: a token that +//! merely signed a base64 payload would still let its holder read the id and +//! timestamp of the denied row it names, which is the disclosure the token +//! exists to prevent. The payload is therefore encrypted under a +//! synthetic-IV construction (SIV): the tag is an HMAC over the filter and the +//! plaintext, and it doubles as the IV seeding the keystream the plaintext is +//! XORed with. That needs no randomness source and no dependency beyond the +//! `hmac`/`sha2` pair already used for webhook signatures and blob recipient +//! tags, and it is decrypt-last: nothing is parsed until the tag verifies. +//! +//! Making the token the *only* accepted cursor also fixes the ordering-domain +//! bug the same review found. `agent_tasks.created_at` is TEXT and compared as +//! TEXT, so `...Z` and `...+00:00` denote one instant but sort differently. A +//! caller-typed timestamp could silently skip or repeat same-time rows. The +//! token instead carries the stored string verbatim, so the value compared is +//! always a value the server wrote. +//! +//! A token is bound to the *caller* as well as the filter. The position it +//! names is the last examined candidate, not the last visible one, so it +//! encodes how far a scan got under one caller's visibility. Resuming it as a +//! different caller would start the scan past rows that caller is entitled to +//! read, silently dropping them from the answer (#327 review). The presenting +//! caller's normalized DID (empty and flagged absent when anonymous) is +//! therefore part of the MAC input, so a token presented by anyone else fails +//! to verify rather than under-reporting. +//! +//! Wire form: `v1..`, both parts base64url unpadded. + +use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use base64::Engine; +use hmac::{Hmac, Mac}; +use serde::{Deserialize, Serialize}; +use sha2::Sha256; + +use crate::error::AppError; + +type HmacSha256 = Hmac; + +const CURSOR_PREFIX: &str = "v1"; +const KEY_DERIVATION_LABEL: &[u8] = b"gitlawb/tasks-cursor-key/v1"; +const TAG_DOMAIN: &[u8] = b"gitlawb/tasks-cursor-tag/v1"; +const STREAM_DOMAIN: &[u8] = b"gitlawb/tasks-cursor-stream/v1"; +/// Truncated MAC length, and the synthetic IV width. 128 bits is far beyond +/// forgery reach for a token that carries no authority of its own (every page +/// re-runs the visibility gate against the presenting caller), and keeps the +/// token short enough to sit in a query string. +const TAG_LEN: usize = 16; + +/// How long an issued cursor stays acceptable. Keyset positions never go stale +/// on their own — `created_at`/`id` are immutable — so this is not a +/// correctness bound. It bounds how long a token stays valid across a node +/// restart-and-rotate and keeps an abandoned page from being resumed +/// indefinitely. +const CURSOR_TTL_SECS: i64 = 24 * 60 * 60; + +/// Node-keyed MAC key for continuation tokens, derived from the node keypair +/// seed so it needs no configuration and survives restarts of the same node. +/// Derived rather than used directly so a token forgery oracle could not +/// bear on the signing key itself. +#[derive(Clone)] +pub struct TaskCursorKey([u8; 32]); + +impl std::fmt::Debug for TaskCursorKey { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("TaskCursorKey()") + } +} + +impl TaskCursorKey { + pub fn derive(node_seed: &[u8; 32]) -> Self { + let mut mac = HmacSha256::new_from_slice(node_seed).expect("HMAC accepts any key length"); + mac.update(KEY_DERIVATION_LABEL); + let mut key = [0u8; 32]; + key.copy_from_slice(&mac.finalize().into_bytes()); + Self(key) + } +} + +/// The keyset position a token carries: the last candidate row the previous +/// request examined, whether or not the caller was allowed to see it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TaskPosition { + pub created_at: String, + pub id: String, +} + +impl TaskPosition { + pub fn new(created_at: impl Into, id: impl Into) -> Self { + Self { + created_at: created_at.into(), + id: id.into(), + } + } + + pub fn as_pair(&self) -> (&str, &str) { + (self.created_at.as_str(), self.id.as_str()) + } +} + +#[derive(Serialize, Deserialize)] +struct CursorPayload<'a> { + /// `created_at` of the last examined candidate, verbatim as stored. + t: &'a str, + /// `id` of the last examined candidate. + i: &'a str, + /// Unix expiry. + e: i64, +} + +/// The filter a page was issued under. A cursor is only meaningful against the +/// same `status`/`assignee_did` filter that produced it: resuming a filtered +/// scan from an unfiltered position (or the reverse) silently skips rows. +/// Bound into the MAC rather than stored in the payload, so it costs no token +/// length and cannot be edited without invalidating the tag. +#[derive(Debug, Clone, Copy)] +pub struct TaskFilter<'a> { + pub status: Option<&'a str>, + pub assignee_did: Option<&'a str>, +} + +/// The visibility identity a token was issued to. Normalized through +/// `normalize_owner_key` so the two spellings of one `did:key` identity +/// (`did:key:X` and bare `X`) bind identically, matching `did_matches` on the +/// read path: a caller who authenticates with the other spelling of their own +/// DID must be able to resume their own page. +fn caller_binding(caller: Option<&str>) -> &str { + caller.map(crate::db::normalize_owner_key).unwrap_or("") +} + +/// The assignee filter a token was issued under. Normalized through +/// `normalize_owner_key` so the two spellings of one `did:key` identity +/// (`did:key:X` and bare `X`) bind identically, matching `list_tasks_keyset` +/// in SQL: minting under `did:key:X` and resuming with bare `X` (or the reverse) +/// hits the same rows and must verify under the MAC. +fn assignee_binding(assignee: Option<&str>) -> &str { + assignee.map(crate::db::normalize_owner_key).unwrap_or("") +} + +fn cursor_mac( + key: &TaskCursorKey, + filter: TaskFilter<'_>, + caller: Option<&str>, + plaintext: &[u8], +) -> HmacSha256 { + let mut mac = HmacSha256::new_from_slice(&key.0).expect("HMAC accepts any key length"); + mac.update(TAG_DOMAIN); + // Length-prefix every field so no two distinct filter/caller/plaintext + // tuples can produce the same MAC input. + for field in [ + filter.status.unwrap_or("").as_bytes(), + assignee_binding(filter.assignee_did).as_bytes(), + caller_binding(caller).as_bytes(), + plaintext, + ] { + mac.update(&(field.len() as u64).to_be_bytes()); + mac.update(field); + } + // Presence is distinct from emptiness for all three optional fields, so an + // anonymous token cannot collide with one issued to a caller whose + // normalized DID is the empty string. + mac.update(&[ + u8::from(filter.status.is_some()), + u8::from(filter.assignee_did.is_some()), + u8::from(caller.is_some()), + ]); + mac +} + +/// Synthetic IV: the authentication tag over the filter and plaintext, which +/// also seeds the keystream. Deterministic by construction, so no randomness +/// source is needed and two tokens for the same page are byte-identical. +fn siv( + key: &TaskCursorKey, + filter: TaskFilter<'_>, + caller: Option<&str>, + plaintext: &[u8], +) -> [u8; TAG_LEN] { + let mut out = [0u8; TAG_LEN]; + out.copy_from_slice( + &cursor_mac(key, filter, caller, plaintext) + .finalize() + .into_bytes()[..TAG_LEN], + ); + out +} + +/// XOR `buf` with the keystream for `iv`. Its own inverse, so encrypt and +/// decrypt are the same call. +fn apply_keystream(key: &TaskCursorKey, iv: &[u8; TAG_LEN], buf: &mut [u8]) { + for (block_index, chunk) in buf.chunks_mut(32).enumerate() { + let mut mac = HmacSha256::new_from_slice(&key.0).expect("HMAC accepts any key length"); + mac.update(STREAM_DOMAIN); + mac.update(iv); + mac.update(&(block_index as u64).to_be_bytes()); + let block = mac.finalize().into_bytes(); + for (byte, k) in chunk.iter_mut().zip(block.iter()) { + *byte ^= k; + } + } +} + +/// Mint a token resuming at `position` for `filter`, usable only by `caller`. +pub fn encode( + key: &TaskCursorKey, + filter: TaskFilter<'_>, + caller: Option<&str>, + position: &TaskPosition, +) -> String { + let mut payload = serde_json::to_vec(&CursorPayload { + t: &position.created_at, + i: &position.id, + e: chrono::Utc::now().timestamp() + CURSOR_TTL_SECS, + }) + .expect("cursor payload is plain strings and an integer"); + let iv = siv(key, filter, caller, &payload); + apply_keystream(key, &iv, &mut payload); + format!( + "{CURSOR_PREFIX}.{}.{}", + URL_SAFE_NO_PAD.encode(iv), + URL_SAFE_NO_PAD.encode(&payload) + ) +} + +/// One rejection message for every way a token can fail to verify. A caller +/// who mangled a token, replayed an expired one, or tried to move one to a +/// different filter or a different presenting identity learns only that the +/// cursor is not usable — never which of those it was, and never anything +/// about the row it named. +const INVALID_CURSOR: &str = "invalid or expired cursor"; + +fn reject() -> AppError { + AppError::BadRequest(INVALID_CURSOR.into()) +} + +/// Verify a token against `filter` and the presenting `caller`, and return the +/// position it carries. +pub fn decode( + key: &TaskCursorKey, + filter: TaskFilter<'_>, + caller: Option<&str>, + token: &str, +) -> crate::error::Result { + let mut parts = token.split('.'); + let (Some(version), Some(iv_b64), Some(body_b64), None) = + (parts.next(), parts.next(), parts.next(), parts.next()) + else { + return Err(reject()); + }; + if version != CURSOR_PREFIX { + return Err(reject()); + } + let iv_bytes = URL_SAFE_NO_PAD.decode(iv_b64).map_err(|_| reject())?; + let mut plaintext = URL_SAFE_NO_PAD.decode(body_b64).map_err(|_| reject())?; + let iv: [u8; TAG_LEN] = iv_bytes.try_into().map_err(|_| reject())?; + + apply_keystream(key, &iv, &mut plaintext); + // Authenticate before parsing: until the tag matches, `plaintext` is just + // attacker-chosen bytes run through a keystream. `verify_truncated_left` + // is the constant-time compare, so a forgery attempt cannot be steered by + // timing the first differing byte. + cursor_mac(key, filter, caller, &plaintext) + .verify_truncated_left(&iv) + .map_err(|_| reject())?; + + let decoded: CursorPayload<'_> = serde_json::from_slice(&plaintext).map_err(|_| reject())?; + if decoded.e < chrono::Utc::now().timestamp() { + return Err(reject()); + } + Ok(TaskPosition::new(decoded.t, decoded.i)) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn key() -> TaskCursorKey { + TaskCursorKey::derive(&[7u8; 32]) + } + + fn unfiltered() -> TaskFilter<'static> { + TaskFilter { + status: None, + assignee_did: None, + } + } + + #[test] + fn round_trips_position_verbatim() { + let k = key(); + // A stored timestamp the server wrote, fractional digits and all. + let pos = TaskPosition::new("2026-01-03T00:00:00.123456789+00:00", "task-a"); + let token = encode(&k, unfiltered(), None, &pos); + assert_eq!(decode(&k, unfiltered(), None, &token).unwrap(), pos); + } + + /// The whole point of the token is that the caller may hold it without + /// learning the denied row it names. A signed-but-plaintext payload would + /// pass every other test here and still fail this one. + #[test] + fn token_does_not_expose_the_row_it_names() { + let k = key(); + let token = encode( + &k, + unfiltered(), + None, + &TaskPosition::new("2026-01-03T00:00:00+00:00", "denied-task-id"), + ); + let body = token.split('.').nth(2).expect("token has a body part"); + let raw = URL_SAFE_NO_PAD.decode(body).unwrap(); + let as_text = String::from_utf8_lossy(&raw); + for secret in ["denied-task-id", "2026-01-03", "\"t\"", "\"i\""] { + assert!( + !as_text.contains(secret), + "token body must not carry {secret:?} in the clear: {as_text:?}" + ); + } + // Also assert it is not merely reordered or whitespace-mangled JSON. + assert!( + serde_json::from_slice::(&raw).is_err(), + "token body must not be parseable as JSON" + ); + } + + /// Callers paste the token straight into a query string, so it must carry + /// no character that needs percent-encoding. + #[test] + fn token_is_url_safe_verbatim() { + let token = encode( + &key(), + TaskFilter { + status: Some("pending"), + assignee_did: Some("did:key:z6MkAssignee"), + }, + None, + &TaskPosition::new("2026-01-03T00:00:00.123456789+00:00", "task-a"), + ); + assert!( + token + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.')), + "token must be query-safe without encoding: {token}" + ); + } + + /// Flipping any byte of a token must fail the tag, not decode to a + /// different position: the keystream is malleable on its own, the tag is + /// what stops a chosen-position forgery. + #[test] + fn rejects_any_tampered_byte() { + let k = key(); + let token = encode( + &k, + unfiltered(), + None, + &TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"), + ); + let parts: Vec<&str> = token.split('.').collect(); + for part in [1usize, 2] { + for position in [0usize, 3] { + let mut bytes = parts[part].as_bytes().to_vec(); + bytes[position] = if bytes[position] == b'A' { b'B' } else { b'A' }; + let mut mangled: Vec = parts.iter().map(|p| p.to_string()).collect(); + mangled[part] = String::from_utf8(bytes).unwrap(); + assert!( + decode(&k, unfiltered(), None, &mangled.join(".")).is_err(), + "byte {position} of part {part} must not be malleable" + ); + } + } + // Sanity: the untouched token still decodes, so the loop above is not + // passing because every token is rejected. + assert!(decode(&k, unfiltered(), None, &token).is_ok()); + } + + #[test] + fn rejects_token_from_another_node() { + let pos = TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"); + let token = encode(&TaskCursorKey::derive(&[1u8; 32]), unfiltered(), None, &pos); + assert!(decode( + &TaskCursorKey::derive(&[2u8; 32]), + unfiltered(), + None, + &token + ) + .is_err()); + } + + #[test] + fn rejects_cursor_moved_to_a_different_filter() { + let k = key(); + let pos = TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"); + let token = encode( + &k, + TaskFilter { + status: Some("pending"), + assignee_did: None, + }, + None, + &pos, + ); + assert!(decode(&k, unfiltered(), None, &token).is_err()); + assert!(decode( + &k, + TaskFilter { + status: Some("claimed"), + assignee_did: None + }, + None, + &token + ) + .is_err()); + assert!(decode( + &k, + TaskFilter { + status: Some("pending"), + assignee_did: None + }, + None, + &token + ) + .is_ok()); + } + + /// A filter of `Some("")` must not verify a token minted with `None`. + #[test] + fn distinguishes_absent_filter_from_empty_filter() { + let k = key(); + let pos = TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"); + let token = encode(&k, unfiltered(), None, &pos); + assert!(decode( + &k, + TaskFilter { + status: Some(""), + assignee_did: None + }, + None, + &token + ) + .is_err()); + } + + /// A cursor names how far a scan got under one caller's visibility. Moved + /// to a different presenting identity it would start that caller's scan + /// past rows they are entitled to read, so it must fail to verify rather + /// than silently under-report (#327 review). + #[test] + fn rejects_cursor_moved_to_a_different_caller() { + let k = key(); + let pos = TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"); + let alice = Some("did:key:z6MkAlice"); + let bob = Some("did:key:z6MkBob"); + + let anon_token = encode(&k, unfiltered(), None, &pos); + assert!( + decode(&k, unfiltered(), alice, &anon_token).is_err(), + "an anonymously minted cursor must not resume an authenticated scan" + ); + assert!(decode(&k, unfiltered(), None, &anon_token).is_ok()); + + let alice_token = encode(&k, unfiltered(), alice, &pos); + assert!( + decode(&k, unfiltered(), bob, &alice_token).is_err(), + "one caller's cursor must not resume another caller's scan" + ); + assert!( + decode(&k, unfiltered(), None, &alice_token).is_err(), + "an authenticated cursor must not resume an anonymous scan" + ); + assert!(decode(&k, unfiltered(), alice, &alice_token).is_ok()); + } + + /// The binding is on identity, not spelling: `did:key:X` and bare `X` are + /// one caller everywhere else on the read path (`did_matches`), so a caller + /// who presents the other form of their own DID must keep their own page. + #[test] + fn caller_binding_is_normalized_across_did_key_spellings() { + let k = key(); + let pos = TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"); + let token = encode(&k, unfiltered(), Some("did:key:z6MkAlice"), &pos); + assert_eq!( + decode(&k, unfiltered(), Some("z6MkAlice"), &token).unwrap(), + pos + ); + // A different method sharing the base58 tail is a different principal. + assert!(decode(&k, unfiltered(), Some("did:web:z6MkAlice"), &token).is_err()); + } + + /// The filter binding is on identity, not spelling: `assignee_did` matches + /// normalized keys in SQL (`normalize_owner_key`), so minting under + /// `did:key:X` and resuming under bare `X` (or vice versa) must succeed, + /// while `did:web:X` remains distinct. + #[test] + fn assignee_filter_binding_is_normalized_across_did_key_spellings() { + let k = key(); + let pos = TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"); + let filter_did_key = TaskFilter { + status: Some("pending"), + assignee_did: Some("did:key:z6MkAssignee"), + }; + let filter_bare = TaskFilter { + status: Some("pending"), + assignee_did: Some("z6MkAssignee"), + }; + let filter_web = TaskFilter { + status: Some("pending"), + assignee_did: Some("did:web:z6MkAssignee"), + }; + + let token_from_did_key = encode(&k, filter_did_key, None, &pos); + assert_eq!( + decode(&k, filter_bare, None, &token_from_did_key).unwrap(), + pos + ); + assert!(decode(&k, filter_web, None, &token_from_did_key).is_err()); + + let token_from_bare = encode(&k, filter_bare, None, &pos); + assert_eq!( + decode(&k, filter_did_key, None, &token_from_bare).unwrap(), + pos + ); + assert!(decode(&k, filter_web, None, &token_from_bare).is_err()); + } + + /// Anonymous is a distinct binding from a caller whose normalized DID is + /// the empty string, the same way `Some("")` is distinct from `None` for + /// the filter fields. + #[test] + fn distinguishes_anonymous_from_empty_caller() { + let k = key(); + let pos = TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"); + let token = encode(&k, unfiltered(), None, &pos); + assert!(decode(&k, unfiltered(), Some(""), &token).is_err()); + } + + /// Length-prefixing must stop a status/assignee pair from being re-split. + #[test] + fn rejects_field_boundary_shift() { + let k = key(); + let pos = TaskPosition::new("2026-01-03T00:00:00+00:00", "task-a"); + let token = encode( + &k, + TaskFilter { + status: Some("pend"), + assignee_did: Some("ing"), + }, + None, + &pos, + ); + assert!(decode( + &k, + TaskFilter { + status: Some("pending"), + assignee_did: Some("") + }, + None, + &token + ) + .is_err()); + } + + #[test] + fn rejects_expired_token() { + let k = key(); + // Minted the same way `encode` does, but already past its expiry. + let mut payload = serde_json::to_vec(&CursorPayload { + t: "2026-01-03T00:00:00+00:00", + i: "task-a", + e: chrono::Utc::now().timestamp() - 1, + }) + .unwrap(); + let iv = siv(&k, unfiltered(), None, &payload); + apply_keystream(&k, &iv, &mut payload); + let expired = format!( + "v1.{}.{}", + URL_SAFE_NO_PAD.encode(iv), + URL_SAFE_NO_PAD.encode(&payload) + ); + let err = decode(&k, unfiltered(), None, &expired).unwrap_err(); + assert!(err.to_string().contains(INVALID_CURSOR)); + } + + #[test] + fn rejects_malformed_shapes() { + let k = key(); + for bad in [ + "", + "v1", + "v1.", + "v1.abc", + "v2.abc.def", + "v1.abc.def.ghi", + "v1.!!!.def", + ] { + assert!( + decode(&k, unfiltered(), None, bad).is_err(), + "must reject {bad:?}" + ); + } + } + + /// Every rejection path renders the same text, so a caller cannot tell a + /// forged token from an expired one from a filter mismatch. + #[test] + fn every_rejection_is_indistinguishable() { + let k = key(); + for bad in ["v1.abc.def", "not-a-cursor", "v1..", "v9.a.b"] { + assert_eq!( + decode(&k, unfiltered(), None, bad).unwrap_err().to_string(), + format!("invalid request: {INVALID_CURSOR}") + ); + } + } +} diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index de22134ae..c61c815d7 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -8,6 +8,8 @@ //! POST /api/v1/tasks/{id}/complete — complete task //! POST /api/v1/tasks/{id}/fail — fail task +use std::collections::{HashMap, HashSet}; + use axum::{ extract::{Extension, Path, Query, State}, http::StatusCode, @@ -19,7 +21,8 @@ use serde_json::{json, Value}; use uuid::Uuid; use crate::auth::AuthenticatedDid; -use crate::db::AgentTask; +use crate::db::{AgentTask, RepoRecord, VisibilityRule}; +use crate::error::AppError; use crate::state::{AppState, TaskEventBroadcast}; /// 403 in this module's error shape (`(StatusCode, Json)`, not `AppError`). @@ -30,6 +33,16 @@ fn forbidden(msg: &str) -> (StatusCode, Json) { ) } +/// Map a db-layer `anyhow` from claim/finish: connection-class sqlx failures +/// stay retryable 503, business "not claimable / not claimed" stays 409 with +/// a fixed message (not the anyhow text). +pub(crate) fn task_write_conflict(err: anyhow::Error, message: &str) -> AppError { + match AppError::from(err) { + db @ AppError::Db(_) => db, + _ => AppError::Conflict(message.into()), + } +} + // ── Request / response types ────────────────────────────────────────────────── #[derive(Deserialize)] @@ -50,6 +63,13 @@ pub struct ListTasksQuery { pub assignee_did: Option, #[serde(default = "default_limit")] pub limit: i64, + /// Opaque continuation token from a previous response's `next_cursor`. + /// The raw `after_created_at`/`after_id`/`cursor_created_at`/`cursor_id` + /// pairs this replaces are gone (#327 review): a caller-typed timestamp + /// compared against TEXT storage had no single ordering domain, and a + /// caller-held cursor could only ever name a row the caller had already + /// seen, which is what made a long denied window unpageable. + pub cursor: Option, } fn default_limit() -> i64 { @@ -89,6 +109,323 @@ fn task_to_json(t: &AgentTask) -> Value { }) } +/// Same projection as `task_to_json`, minus `ucan_token` (#268). The read +/// surfaces (`list_tasks`, `get_task`) never need to echo it back to anyone, +/// including the delegator/assignee: it was handed to the assignee at +/// delegation/claim time via the write-side responses, which still use +/// `task_to_json` unchanged. +fn task_to_read_json(t: &AgentTask) -> Value { + json!({ + "id": t.id, + "repo_id": t.repo_id, + "kind": t.kind, + "status": t.status, + "delegator_did": t.delegator_did, + "assignee_did": t.assignee_did, + "capability": t.capability, + "payload": t.payload, + "result": t.result, + "created_at": t.created_at, + "updated_at": t.updated_at, + "deadline": t.deadline, + }) +} + +/// Hard ceiling on rows a task read surface fetches for one request, mirroring +/// `MAX_VISIBLE_REF_UPDATES` in `api/events.rs` (#112/#114) for the same +/// reason: bound the underlying query before an unauthenticated caller's +/// request size controls how much the visibility filter has to scan. +const MAX_VISIBLE_TASKS: i64 = 200; + +/// Maximum task candidates one list request may inspect while searching for +/// visible rows. This keeps a denied request from walking the full task table. +const MAX_TASK_SCAN_CANDIDATES: i64 = 1_000; + +/// Whether `task` should be visible to `caller` (`None` = anonymous). +/// +/// The delegator and assignee can always read a task they are already party +/// to — they hold its `payload` (and held `ucan_token`, though reads never +/// echo it back) from creating or being assigned it. Otherwise, a task naming +/// a locally-hosted repo follows that repo's normal read gate, the same way +/// `ref_update_row_visible` (`visibility.rs`) drops a ref-update row for a +/// repo the caller can't read. A task with no `repo_id`, or naming a repo this +/// node does not host, is visible only to its delegator/assignee — fail +/// closed, since an open-to-everyone default is exactly the gap #268 found +/// (`GET /api/v1/tasks` and `/tasks/{id}` had no gate at all). +pub(crate) fn task_visible( + task: &AgentTask, + caller: Option<&str>, + repos_by_id: &HashMap, + rules_by_repo: &HashMap>, +) -> bool { + if let Some(c) = caller { + if crate::api::did_matches(c, &task.delegator_did) { + return true; + } + let assignee_match = task + .assignee_did + .as_deref() + .map(|a| crate::api::did_matches(c, a)) + .unwrap_or(false); + if assignee_match { + return true; + } + } + let Some(repo_id) = task.repo_id.as_deref() else { + return false; + }; + // Slash-form ids are mirror rows. Mirrors are public placeholders and do + // not replicate visibility rules, so they cannot establish read access. + if repo_id.contains('/') { + return false; + } + let Some(record) = repos_by_id.get(repo_id) else { + return false; + }; + let rules = rules_by_repo + .get(&record.id) + .map(Vec::as_slice) + .unwrap_or(&[]); + crate::visibility::listable_at_root(rules, record.is_public, &record.owner_did, caller) +} + +#[derive(Debug, Clone)] +pub(crate) struct VisibleTasks { + pub tasks: Vec, + /// True when candidate rows remain past this page, whether or not the + /// page filled. `next_position` is `Some` exactly when this is true. + pub has_more: bool, + /// True when this request stopped at `MAX_TASK_SCAN_CANDIDATES` with the + /// page still unfilled, so a short or empty page is a paused scan rather + /// than the end of the stream. + /// + /// Kept separate from `has_more` because they answer different questions + /// (#327 review): `has_more` says another page exists, `incomplete` says + /// *this* page is short only because the authorization scan hit its safety + /// wall. Overloading one flag for both left a caller unable to tell a + /// finished stream from a paused one. + pub incomplete: bool, + /// Keyset position of the last candidate this request *examined*, visible + /// or not. Handed back only inside a MAC'd token (`api::task_cursor`), so + /// resuming a scan can step past a denied window without the denied rows' + /// ids or timestamps ever reaching the caller in the clear. + pub next_position: Option, +} + +/// Collect up to `limit` tasks visible to `caller`, applying the same gate the +/// GraphQL `tasks` query uses (`collect_visible_tasks` is called from both) so +/// the two surfaces cannot drift, matching the `collect_visible_ref_updates` +/// pattern in `api/events.rs`. `limit` is clamped here so a caller-supplied +/// value never reaches SQL unclamped. +/// +/// `resume` is the position decoded from a continuation token, never a +/// caller-typed cursor: the token carries `created_at` verbatim as stored, so +/// the TEXT comparison in `list_tasks_keyset` always runs against a string the +/// server wrote. That is what keeps equivalent RFC3339 spellings (`Z` versus +/// `+00:00`, differing fractional widths) from silently skipping or repeating +/// same-timestamp rows. +pub(crate) async fn collect_visible_tasks( + db: &crate::db::Db, + status: Option<&str>, + assignee_did: Option<&str>, + limit: i64, + resume: Option<&crate::api::task_cursor::TaskPosition>, + caller: Option<&str>, +) -> crate::error::Result { + use crate::api::task_cursor::TaskPosition; + + let bounded_limit = limit.clamp(0, MAX_VISIBLE_TASKS) as usize; + if bounded_limit == 0 { + return Ok(VisibleTasks { + tasks: Vec::new(), + has_more: false, + incomplete: false, + next_position: None, + }); + } + // Probe for bounded_limit + 1 visible tasks to prove has_more from visible rows only (#327 review). + let target_visible = bounded_limit + 1; + let mut visible = Vec::with_capacity(target_visible); + let mut examined: Option = resume.cloned(); + let mut resume_position_for_page: Option = None; + let mut scanned: i64 = 0; + let mut stream_ended = false; + + while scanned < MAX_TASK_SCAN_CANDIDATES && visible.len() < target_visible { + let batch_limit = MAX_VISIBLE_TASKS.min(MAX_TASK_SCAN_CANDIDATES - scanned); + let batch = db + .list_tasks_keyset( + status, + assignee_did, + batch_limit, + examined.as_ref().map(TaskPosition::as_pair), + ) + .await?; + if batch.is_empty() { + stream_ended = true; + break; + } + let batch_len = batch.len() as i64; + + let referenced: Vec = batch + .iter() + .filter_map(|task| task.repo_id.clone()) + .collect::>() + .into_iter() + .collect(); + let repos_by_id: HashMap = db + .list_repos_deduped_by_ids(&referenced) + .await? + .into_iter() + .map(|repo| (repo.id.clone(), repo)) + .collect(); + let repo_ids: Vec = repos_by_id.keys().cloned().collect(); + let rules_by_repo = db.list_visibility_rules_for_repos(&repo_ids).await?; + + let mut consumed = 0usize; + for task in &batch { + // Advance the examined position per row, not per batch: when the + // page fills mid-batch the resume point is that row, so the next + // request neither repeats nor skips its successors. + scanned += 1; + consumed += 1; + let current_pos = TaskPosition::new(task.created_at.clone(), task.id.clone()); + examined = Some(current_pos.clone()); + if task_visible(task, caller, &repos_by_id, &rules_by_repo) { + visible.push(task.clone()); + if visible.len() == bounded_limit { + resume_position_for_page = Some(current_pos); + } else if visible.len() == target_visible { + break; + } + } + } + + // A short batch only ends the stream once every row in it has been + // examined. + if consumed == batch.len() && batch_len < batch_limit { + stream_ended = true; + break; + } + } + + // While the scan is running, `has_more` is derived from visible rows only: + // finding a (bounded_limit + 1)-th visible row is what proves another page + // exists, so trailing denied rows inside one scan can never set it (#327 + // review, `trailing_denied_tasks_do_not_set_has_more_or_leak_existence`). + // + // The scan ceiling is the one case that cannot be answered from visible + // rows, and the `else` branch below settles it with an un-gated + // `LIMIT 1` probe at the last examined position. That is deliberate, and + // the disclosure it carries is bounded as follows (#327 review): + // + // * What it can tell a caller: whether *any* candidate row — readable or + // not — trails the position this request stopped at. One bit. + // * Where it can be asked: only at positions the server chose, which + // advance a full `MAX_TASK_SCAN_CANDIDATES` per request, and only via a + // MAC'd continuation token bound to the caller and filter + // (`api::task_cursor`). A caller cannot aim the probe at a row of their + // choosing, so the coarsest thing it yields is the candidate count to + // `MAX_TASK_SCAN_CANDIDATES` granularity. + // * What it never carries: no denied row's id, timestamp, payload or + // `ucan_token` reaches the caller, and `get_task` stays opaquely 404. + // + // Withholding the probe does not remove that bit, it only defers it: + // enumeration past a denied window longer than one scan budget is a hard + // requirement (`denied_window_longer_than_scan_budget_is_pageable_to_the_end`), + // so the ceiling has to hand back a continuation, and following that + // continuation returns the same terminal page the probe predicted — one + // round trip later. Paying an extra request per scan window to relocate + // the same bit is not a gate. `scan_ceiling_continuation_discloses_only_a_terminal_page` + // pins the bound end to end. + let (has_more, next_position) = if visible.len() > bounded_limit { + visible.truncate(bounded_limit); + (true, resume_position_for_page) + } else if stream_ended { + (false, None) + } else { + // Scan budget exhausted with the page unproven: ask whether the + // candidate stream itself is finished, so an exhausted scan that + // happens to land on the last row is reported as the end rather than + // as a paused scan the caller would re-request forever. + let more_in_db = !db + .list_tasks_keyset( + status, + assignee_did, + 1, + examined.as_ref().map(TaskPosition::as_pair), + ) + .await? + .is_empty(); + if more_in_db { + (true, examined) + } else { + (false, None) + } + }; + + let incomplete = has_more && visible.len() < bounded_limit; + + Ok(VisibleTasks { + tasks: visible, + has_more, + incomplete, + next_position, + }) +} + +/// Fetch a single task gated the same way `collect_visible_tasks` gates a +/// page. Returns `None` both when the task does not exist and when the caller +/// may not see it — the two are indistinguishable to the caller, matching +/// `authorize_repo_read`'s opaque not-found-vs-denied handling, so an +/// unauthorized caller cannot use this to probe which task IDs exist. +pub(crate) async fn get_visible_task( + db: &crate::db::Db, + id: &str, + caller: Option<&str>, +) -> crate::error::Result> { + let Some(task) = db.get_task(id).await? else { + return Ok(None); + }; + let (repos_by_id, rules_by_repo) = match task.repo_id.as_deref() { + Some(repo_id) => { + let ids = [repo_id.to_string()]; + let repos = db.list_repos_deduped_by_ids(&ids).await?; + match repos.into_iter().find(|r| r.id == repo_id) { + Some(record) => { + let rules = db.list_visibility_rules(&record.id).await?; + ( + HashMap::from([(record.id.clone(), record)]), + HashMap::from([(repo_id.to_string(), rules)]), + ) + } + None => (HashMap::new(), HashMap::new()), + } + } + None => (HashMap::new(), HashMap::new()), + }; + Ok(task_visible(&task, caller, &repos_by_id, &rules_by_repo).then_some(task)) +} + +/// Broadcast a task event only when the task is publicly visible. +/// Matches `if announce` on ref updates: private-task status changes stay off +/// the unauthenticated GraphQL subscription. +pub(crate) async fn announce_task_event( + db: &crate::db::Db, + tx: &tokio::sync::broadcast::Sender, + event: TaskEventBroadcast, +) { + match get_visible_task(db, &event.task_id, None).await { + Ok(Some(_)) => { + let _ = tx.send(event); + } + Ok(None) => {} + Err(e) => { + tracing::warn!(error = %e, task_id = %event.task_id, "skipping task event broadcast"); + } + } +} + // ── Handlers ────────────────────────────────────────────────────────────────── /// POST /api/v1/tasks @@ -96,7 +433,7 @@ pub async fn create_task( State(state): State, Extension(auth): Extension, Json(body): Json, -) -> Result<(StatusCode, Json), (StatusCode, Json)> { +) -> std::result::Result<(StatusCode, Json), (StatusCode, Json)> { // Bind the delegator to the authenticated signer (N13). if !crate::api::did_matches(&auth.0, &body.delegator_did) { return Err(forbidden("delegator_did must be the authenticated signer")); @@ -127,39 +464,69 @@ pub async fn create_task( } /// GET /api/v1/tasks +/// +/// Open to anonymous callers, but every row is gated by `collect_visible_tasks` +/// (#268): an anonymous or unrelated caller only sees tasks against a repo they +/// can read, never another party's repo-less task or its `ucan_token`/`payload`. +/// +/// Paging follows the module-level contract. `limit` is echoed back as the +/// value actually applied, so a caller asking for more than +/// `MAX_VISIBLE_TASKS` can see the clamp rather than mistaking a full page for +/// the whole answer. pub async fn list_tasks( State(state): State, Query(q): Query, -) -> Result, (StatusCode, Json)> { - let tasks = state - .db - .list_tasks(q.status.as_deref(), q.assignee_did.as_deref(), q.limit) - .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })?; - let items: Vec = tasks.iter().map(task_to_json).collect(); - Ok(Json(json!({ "tasks": items, "count": items.len() }))) + auth: Option>, +) -> crate::error::Result> { + use crate::api::task_cursor::{self, TaskFilter}; + + let caller = auth.as_ref().map(|e| e.0 .0.as_str()); + let filter = TaskFilter { + status: q.status.as_deref(), + assignee_did: q.assignee_did.as_deref(), + }; + let resume = q + .cursor + .as_deref() + .map(|token| task_cursor::decode(&state.task_cursor_key, filter, caller, token)) + .transpose()?; + let result = collect_visible_tasks( + &state.db, + q.status.as_deref(), + q.assignee_did.as_deref(), + q.limit, + resume.as_ref(), + caller, + ) + .await?; + let next_cursor = result + .next_position + .as_ref() + .map(|pos| task_cursor::encode(&state.task_cursor_key, filter, caller, pos)); + let items: Vec = result.tasks.iter().map(task_to_read_json).collect(); + Ok(Json(json!({ + "tasks": items, + "count": items.len(), + "limit": q.limit.clamp(0, MAX_VISIBLE_TASKS), + "has_more": result.has_more, + "incomplete": result.incomplete, + "next_cursor": next_cursor, + }))) } /// GET /api/v1/tasks/{id} +/// +/// Gated the same way as `list_tasks` (#268): a task the caller may not see +/// 404s, indistinguishable from a task that doesn't exist. pub async fn get_task( State(state): State, Path(id): Path, -) -> Result, (StatusCode, Json)> { - match state.db.get_task(&id).await { - Ok(Some(t)) => Ok(Json(task_to_json(&t))), - Ok(None) => Err(( - StatusCode::NOT_FOUND, - Json(json!({ "error": "task not found" })), - )), - Err(e) => Err(( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - )), + auth: Option>, +) -> crate::error::Result> { + let caller = auth.as_ref().map(|e| e.0 .0.as_str()); + match get_visible_task(&state.db, &id, caller).await? { + Some(t) => Ok(Json(task_to_read_json(&t))), + None => Err(AppError::NotFound("task not found".into())), } } @@ -169,24 +536,34 @@ pub async fn claim_task( Extension(auth): Extension, Path(id): Path, Json(body): Json, -) -> Result, (StatusCode, Json)> { +) -> crate::error::Result> { // Bind the assignee to the authenticated signer (N13). if !crate::api::did_matches(&auth.0, &body.assignee_did) { - return Err(forbidden("assignee_did must be the authenticated signer")); + return Err(AppError::Forbidden( + "assignee_did must be the authenticated signer".into(), + )); } - let task = state.db.claim_task(&id, &auth.0).await.map_err(|e| { - ( - StatusCode::CONFLICT, - Json(json!({ "error": e.to_string() })), - ) - })?; - let _ = state.task_event_tx.send(TaskEventBroadcast { - task_id: id, - old_status: "pending".to_string(), - new_status: "claimed".to_string(), - by_did: auth.0, - at: Utc::now().to_rfc3339(), - }); + // Same visibility gate as complete/fail: invisible tasks are 404 so + // existence is not leaked via a successful claim or a leaking 409. + get_visible_task(&state.db, &id, Some(&auth.0)) + .await? + .ok_or_else(|| AppError::NotFound("task not found".into()))?; + let task = + state.db.claim_task(&id, &auth.0).await.map_err(|e| { + task_write_conflict(e, "task not claimable: not found or already claimed") + })?; + announce_task_event( + &state.db, + &state.task_event_tx, + TaskEventBroadcast { + task_id: id, + old_status: "pending".to_string(), + new_status: "claimed".to_string(), + by_did: auth.0, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(Json(task_to_json(&task))) } @@ -196,51 +573,39 @@ pub async fn complete_task( Extension(auth): Extension, Path(id): Path, Json(body): Json, -) -> Result, (StatusCode, Json)> { - // Authorize the actor, not just bind their identity: the N13 signer-binding - // proved the caller was whoever they claimed, but never that they were the - // task's assignee. Load the task and require the caller to be its assignee; - // finish_task then transitions only a claimed task. - let existing = state - .db - .get_task(&id) - .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })? - .ok_or_else(|| { - ( - StatusCode::NOT_FOUND, - Json(json!({ "error": "task not found" })), - ) - })?; +) -> crate::error::Result> { + // Authorize the actor, not just bind their identity: the task must be visible + // to the caller (returning 404 for invisible tasks so existence is not leaked), + // and only the task's assignee may complete it. + let existing = get_visible_task(&state.db, &id, Some(&auth.0)) + .await? + .ok_or_else(|| AppError::NotFound("task not found".into()))?; if !crate::api::did_matches( &auth.0, existing.assignee_did.as_deref().unwrap_or_default(), ) { - return Err(forbidden("only the task assignee can complete it")); + return Err(AppError::Forbidden( + "only the task assignee can complete it".into(), + )); } let by_did = auth.0; let task = state .db .finish_task(&id, "completed", body.result.as_deref()) .await - .map_err(|e| { - ( - StatusCode::CONFLICT, - Json(json!({ "error": e.to_string() })), - ) - })?; - let _ = state.task_event_tx.send(TaskEventBroadcast { - task_id: id, - old_status: "claimed".to_string(), - new_status: "completed".to_string(), - by_did, - at: Utc::now().to_rfc3339(), - }); + .map_err(|e| task_write_conflict(e, "task not found or not in claimed state"))?; + announce_task_event( + &state.db, + &state.task_event_tx, + TaskEventBroadcast { + task_id: id, + old_status: "claimed".to_string(), + new_status: "completed".to_string(), + by_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(Json(task_to_json(&task))) } @@ -250,31 +615,20 @@ pub async fn fail_task( Extension(auth): Extension, Path(id): Path, Json(body): Json, -) -> Result, (StatusCode, Json)> { - // Authorize the actor, not just bind their identity (see complete_task): only - // the task's assignee may fail it, and finish_task transitions only a claimed - // task. - let existing = state - .db - .get_task(&id) - .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })? - .ok_or_else(|| { - ( - StatusCode::NOT_FOUND, - Json(json!({ "error": "task not found" })), - ) - })?; +) -> crate::error::Result> { + // Authorize the actor: the task must be visible to the caller (returning + // 404 for invisible tasks so existence is not leaked), and only the task's + // assignee may fail it. + let existing = get_visible_task(&state.db, &id, Some(&auth.0)) + .await? + .ok_or_else(|| AppError::NotFound("task not found".into()))?; if !crate::api::did_matches( &auth.0, existing.assignee_did.as_deref().unwrap_or_default(), ) { - return Err(forbidden("only the task assignee can fail it")); + return Err(AppError::Forbidden( + "only the task assignee can fail it".into(), + )); } let by_did = auth.0; let reason = body.reason.unwrap_or_default(); @@ -282,18 +636,1752 @@ pub async fn fail_task( .db .finish_task(&id, "failed", Some(&reason)) .await - .map_err(|e| { + .map_err(|e| task_write_conflict(e, "task not found or not in claimed state"))?; + announce_task_event( + &state.db, + &state.task_event_tx, + TaskEventBroadcast { + task_id: id, + old_status: "claimed".to_string(), + new_status: "failed".to_string(), + by_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; + Ok(Json(task_to_json(&task))) +} + +#[cfg(test)] +mod visible_tasks_tests { + use super::*; + use crate::test_support::{signed_request_as, test_state}; + use axum::body::Body; + use axum::http::{Method, Request, StatusCode}; + use axum::Router; + use chrono::Utc; + use sqlx::PgPool; + use tower::ServiceExt; + + const DELEGATOR: &str = "did:key:z6MkDelegator"; + const ASSIGNEE: &str = "did:key:z6MkAssignee"; + const STRANGER: &str = "did:key:z6MkStranger"; + const SECRET_UCAN: &str = "SECRET-UCAN-TOKEN"; + + fn repo(id: &str, owner_did: &str, name: &str, is_public: bool) -> RepoRecord { + let now = Utc::now(); + RepoRecord { + id: id.into(), + name: name.into(), + owner_did: owner_did.into(), + description: None, + is_public, + default_branch: "main".into(), + created_at: now, + updated_at: now, + disk_path: format!("/tmp/{id}"), + forked_from: None, + machine_id: None, + } + } + + fn task(id: &str, repo_id: Option<&str>, delegator: &str) -> AgentTask { + let now = Utc::now().to_rfc3339(); + AgentTask { + id: id.into(), + repo_id: repo_id.map(String::from), + kind: "build".into(), + status: "pending".into(), + delegator_did: delegator.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: Some(SECRET_UCAN.into()), + payload: Some("payload-data".into()), + result: None, + created_at: now.clone(), + updated_at: now, + deadline: None, + } + } + + fn list_router(state: crate::state::AppState) -> Router { + Router::new() + .route("/api/v1/tasks", axum::routing::get(super::list_tasks)) + .route("/api/v1/tasks/{id}", axum::routing::get(super::get_task)) + .with_state(state) + } + + fn anon_get(uri: &str) -> Request { + Request::builder() + .method(Method::GET) + .uri(uri) + .body(Body::empty()) + .expect("request builder") + } + + async fn body_json(resp: axum::response::Response) -> serde_json::Value { + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("body bytes"); + serde_json::from_slice(&bytes).expect("json body") + } + + /// #268 — load-bearing RED→GREEN: before this fix, `list_tasks`/`get_task` + /// had no gate at all, so an anonymous caller could enumerate every task on + /// the node, including another party's repo-less task, its `ucan_token`, + /// and its `payload`. An anonymous caller must now see neither. + #[sqlx::test] + async fn anon_cannot_list_or_read_repo_less_task_of_another(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!( + body["tasks"].as_array().unwrap().len(), + 0, + "anon must not see another party's repo-less task" + ); + assert_eq!(body["count"], 0); + let serialized = body.to_string(); + assert!(!serialized.contains("t1")); + assert!(!serialized.contains("payload-data")); + assert!(!serialized.contains(SECRET_UCAN)); + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks/t1")) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::NOT_FOUND, + "anon get_task on an invisible task must 404, not leak it" + ); + let body = body_json(resp).await; + let serialized = body.to_string(); + assert!(!serialized.contains("t1")); + assert!(!serialized.contains("payload-data")); + assert!(!serialized.contains(SECRET_UCAN)); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + STRANGER, + Method::GET, + "/api/v1/tasks", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["tasks"].as_array().unwrap().len(), 0); + assert_eq!(body["count"], 0); + let serialized = body.to_string(); + assert!(!serialized.contains("t1")); + assert!(!serialized.contains("payload-data")); + assert!(!serialized.contains(SECRET_UCAN)); + + let resp = list_router(state) + .oneshot(signed_request_as( + STRANGER, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let body = body_json(resp).await; + let serialized = body.to_string(); + assert!(!serialized.contains("t1")); + assert!(!serialized.contains("payload-data")); + assert!(!serialized.contains(SECRET_UCAN)); + } + + /// The delegator can always read their own repo-less task — the party who + /// created it is not locked out by the new gate. + #[sqlx::test] + async fn delegator_sees_own_repo_less_task(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["tasks"].as_array().unwrap().len(), 1); + + let resp = list_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + } + + /// The assignee can read a task they were assigned, even though they are + /// not its delegator. + #[sqlx::test] + async fn assignee_sees_assigned_repo_less_task(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + state.db.claim_task("t1", ASSIGNEE).await.unwrap(); + + let resp = list_router(state) + .oneshot(signed_request_as( + ASSIGNEE, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + } + + /// #268 — `ucan_token` must never appear on the read surfaces, even to the + /// delegator who legitimately holds it: they already received it via the + /// write-side `create_task` response, so a read echo is unnecessary + /// exposure, not a feature. + #[sqlx::test] + async fn ucan_token_never_appears_in_read_responses(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert!( + body.get("ucan_token").is_none(), + "get_task must never echo ucan_token, got {body:?}" + ); + + let resp = list_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert!( + !body.to_string().contains(SECRET_UCAN), + "list_tasks must never echo ucan_token, got {body:?}" + ); + } + + /// A repo-scoped task inherits that repo's read-visibility gate: hidden + /// from a stranger, visible to the repo owner even though the owner is + /// neither the task's delegator nor its assignee. + #[sqlx::test] + async fn repo_scoped_private_task_follows_repo_visibility(pool: PgPool) { + const OWNER: &str = "did:key:z6MkRepoOwner"; + const OTHER_DELEGATOR: &str = "did:key:z6MkOtherDelegator"; + let state = test_state(pool).await; + state + .db + .create_repo(&repo("r1", OWNER, "priv", false)) + .await + .unwrap(); + state + .db + .create_task(&task("t1", Some("r1"), OTHER_DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + STRANGER, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::NOT_FOUND, + "a stranger must not see a private repo's task" + ); + + let resp = list_router(state) + .oneshot(signed_request_as( + OWNER, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::OK, + "the repo owner must see the task via the repo's read gate" + ); + } + + #[sqlx::test] + async fn mirror_only_repo_task_is_hidden_from_anonymous_reads(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .upsert_mirror_repo(DELEGATOR, "mirror", "/tmp/mirror", None, false) + .await + .unwrap(); + let mirror_id = format!("{DELEGATOR}/mirror"); + state + .db + .create_task(&task("t1", Some(&mirror_id), DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 0); + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks/t1")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + } + + /// A negative limit must clamp to zero through `collect_visible_tasks`, + /// not fall through to the visible set. + #[sqlx::test] + async fn negative_limit_returns_empty(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks?limit=-1", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 0, "negative limit must clamp to 0"); + } + + #[sqlx::test] + async fn older_visible_task_is_not_hidden_by_newer_denied_window(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let mut visible = task("visible", Some("public-repo"), DELEGATOR); + visible.created_at = "2026-01-01T00:00:00Z".into(); + visible.updated_at = visible.created_at.clone(); + state.db.create_task(&visible).await.unwrap(); + + for i in 0..MAX_VISIBLE_TASKS { + let mut hidden = task(&format!("hidden-{i:03}"), None, DELEGATOR); + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + state.db.create_task(&hidden).await.unwrap(); + } + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 1); + assert_eq!(body["tasks"][0]["id"], "visible"); + } + + /// #327 review: a visible row behind a denied window longer than the scan + /// budget was permanently unreachable. The only cursor a caller could hold + /// named the last row they *saw*, so every retry rescanned the same denied + /// window and returned `{ tasks: [], incomplete: true }` forever. + /// + /// The server-issued token names the last row *examined*, so each request + /// advances a full scan budget. This walks the whole recovery path using + /// nothing but cursors the server handed back, and asserts the denied rows + /// never appear in any response. + #[sqlx::test] + async fn denied_window_longer_than_scan_budget_is_pageable_to_the_end(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let mut visible_newer = task("newer-visible", Some("public-repo"), DELEGATOR); + visible_newer.created_at = "2026-01-03T00:00:00Z".into(); + visible_newer.updated_at = visible_newer.created_at.clone(); + state.db.create_task(&visible_newer).await.unwrap(); + + // Two and a half scan budgets' worth of rows an anonymous caller may + // not read, so recovery provably takes more than one continuation. + let denied = MAX_TASK_SCAN_CANDIDATES * 2 + MAX_TASK_SCAN_CANDIDATES / 2; + for i in 0..denied { + let mut hidden = task(&format!("hidden-{i:05}"), None, DELEGATOR); + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + state.db.create_task(&hidden).await.unwrap(); + } + + let mut visible_older = task("past-ceiling", Some("public-repo"), DELEGATOR); + visible_older.created_at = "2026-01-01T00:00:00Z".into(); + visible_older.updated_at = visible_older.created_at.clone(); + state.db.create_task(&visible_older).await.unwrap(); + + let mut seen: Vec = Vec::new(); + let mut cursor: Option = None; + let mut requests = 0; + loop { + requests += 1; + assert!(requests <= 10, "recovery must terminate, not spin"); + let uri = match &cursor { + Some(c) => format!("/api/v1/tasks?limit=1&cursor={}", c), + None => "/api/v1/tasks?limit=1".to_string(), + }; + let resp = list_router(state.clone()) + .oneshot(anon_get(&uri)) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + assert!( + !body.to_string().contains("hidden-"), + "no response may disclose a denied row's id: {body}" + ); + for t in body["tasks"].as_array().unwrap() { + seen.push(t["id"].as_str().unwrap().to_string()); + } + // A short page mid-stream is the scan wall, and must say so. + if body["has_more"].as_bool().unwrap() && body["tasks"].as_array().unwrap().is_empty() { + assert_eq!( + body["incomplete"], true, + "an empty page with more rows behind it is a paused scan, not an end: {body}" + ); + } + match body["next_cursor"].as_str() { + Some(c) => { + assert_eq!(body["has_more"], true); + cursor = Some(c.to_string()); + } + None => { + assert_eq!(body["has_more"], false); + assert_eq!( + body["incomplete"], false, + "a terminal page is complete, not incomplete: {body}" + ); + break; + } + } + } + + assert_eq!( + seen, + vec!["newer-visible".to_string(), "past-ceiling".to_string()], + "both visible rows must be reachable using only server-issued cursors" + ); + assert!( + requests > 2, + "the denied window spans multiple scan budgets, so recovery must \ + take more than one continuation (took {requests})" + ); + } + + /// The raw `after_*`/`cursor_*` pairs are gone (#327 review): there is one + /// ordering domain, and it is the one the server writes. An unknown query + /// parameter must be ignored rather than silently paging, so a client + /// still sending the old pair gets page one, not a skipped window. + #[sqlx::test] + async fn removed_raw_cursor_params_do_not_page(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + for (id, ts) in [ + ("t-newer", "2026-01-03T00:00:00Z"), + ("t-older", "2026-01-01T00:00:00Z"), + ] { + let mut t = task(id, Some("public-repo"), DELEGATOR); + t.created_at = ts.into(); + t.updated_at = t.created_at.clone(); + state.db.create_task(&t).await.unwrap(); + } + + for uri in [ + "/api/v1/tasks?after_created_at=2026-01-03T00:00:00Z&after_id=t-newer", + "/api/v1/tasks?cursor_created_at=2026-01-03T00:00:00Z&cursor_id=t-newer", + "/api/v1/tasks?after_id=t-newer", + ] { + let resp = list_router(state.clone()) + .oneshot(anon_get(uri)) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK, "{uri}"); + let body = body_json(resp).await; + assert_eq!( + body["count"], 2, + "{uri}: a removed cursor param must not page, it must be inert" + ); + } + } + + /// Every way a token can fail must be one indistinguishable 400, so a + /// caller cannot use cursor validation as an oracle. + #[sqlx::test] + async fn list_tasks_rejects_unusable_cursors(pool: PgPool) { + use crate::api::task_cursor::{self, TaskCursorKey, TaskFilter, TaskPosition}; + + let state = test_state(pool).await; + let position = TaskPosition::new("2026-01-03T00:00:00Z", "some-task"); + let unfiltered = TaskFilter { + status: None, + assignee_did: None, + }; + + let forged = task_cursor::encode( + &TaskCursorKey::derive(&[9u8; 32]), + unfiltered, + None, + &position, + ); + let wrong_filter = task_cursor::encode( + &state.task_cursor_key, + TaskFilter { + status: Some("pending"), + assignee_did: None, + }, + None, + &position, + ); + + for (label, uri) in [ + ("garbage", "/api/v1/tasks?cursor=not-a-cursor".to_string()), + ( + "forged by another node's key", + format!("/api/v1/tasks?cursor={forged}"), + ), ( - StatusCode::CONFLICT, - Json(json!({ "error": e.to_string() })), + "issued for a different filter", + format!("/api/v1/tasks?cursor={wrong_filter}"), + ), + ] { + let resp = list_router(state.clone()) + .oneshot(anon_get(&uri)) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST, "{label}"); + let body = body_json(resp).await; + assert_eq!( + body["message"], "invalid or expired cursor", + "{label}: every rejection must render the same message" + ); + } + + // The same token against the filter it was issued for is accepted, so + // the rejections above are the binding and not a blanket refusal. + let resp = list_router(state.clone()) + .oneshot(anon_get(&format!( + "/api/v1/tasks?status=pending&cursor={wrong_filter}" + ))) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + } + + /// The per-IP brake on the task read routes is WIRED, not a silent no-op: + /// `rate_limit_by_ip` without its `IpRateLimiter` extension does nothing, + /// so this drives the production router with a tight bucket and asserts a + /// 429. Both routes are anon-reachable and run the #268 visibility gate (a + /// task lookup plus deduped-repo and visibility-rule queries) before the + /// opaque 404, so an unauthenticated prober costs the node work per request + /// whether the id exists or not (#327 review). + /// MUTATION (RED): drop the `axum::Extension(task_read_limiter)` layer in + /// `server.rs` and the probes below reach the handler (200/404) instead. + #[sqlx::test] + async fn task_read_routes_ip_rate_limit_is_attached(pool: PgPool) { + use std::net::SocketAddr; + + let mut state = test_state(pool).await; + // Two slots: one known-id probe and one random-id probe pass, the third + // request from that IP is braked whichever route it targets. + state.task_read_rate_limiter = + crate::rate_limit::RateLimiter::new(2, std::time::Duration::from_secs(3600)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let router = crate::server::build_router(state); + let probe = |peer: SocketAddr, uri: &str| { + let mut req = anon_get(uri); + req.extensions_mut() + .insert(axum::extract::ConnectInfo(peer)); + req + }; + let peer: SocketAddr = "203.0.113.42:5000".parse().unwrap(); + + // A known id and a random one cost the same work and debit the same + // bucket: the gate runs before the response can distinguish them. + for uri in ["/api/v1/tasks/t1", "/api/v1/tasks/does-not-exist"] { + let resp = router.clone().oneshot(probe(peer, uri)).await.unwrap(); + assert_ne!( + resp.status(), + StatusCode::TOO_MANY_REQUESTS, + "{uri}: the first probes from an IP must pass the brake" + ); + assert_eq!( + resp.status(), + StatusCode::NOT_FOUND, + "{uri}: an anonymous probe is an opaque 404 either way" + ); + } + + let resp = router + .clone() + .oneshot(probe(peer, "/api/v1/tasks")) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::TOO_MANY_REQUESTS, + "an exhausted per-IP bucket must brake the list route too — the \ + IpRateLimiter extension must be attached to task_read_routes" + ); + + let other: SocketAddr = "203.0.113.43:5000".parse().unwrap(); + let resp = router + .oneshot(probe(other, "/api/v1/tasks/t1")) + .await + .unwrap(); + assert_ne!( + resp.status(), + StatusCode::TOO_MANY_REQUESTS, + "a different IP must not be braked by another IP's exhausted bucket" + ); + } + + /// The GraphQL task resolvers reach the SAME `collect_visible_tasks` / + /// `get_visible_task` gate as the REST read routes, so they must carry the + /// same per-IP brake — otherwise `task_read_routes` is a fence with an open + /// gate beside it and a prober just asks over /graphql instead (#327 + /// review). The brake rides as request data rather than a router layer + /// because /graphql is one endpoint for every operation; see + /// `rate_limit::TaskReadBrake`. + /// MUTATION (RED): drop the `TaskReadBrake` data from `graphql_handler`, or + /// the `task_read_brake` call from either resolver, and the exhausted-bucket + /// probes below answer normally instead of with the brake message. + #[sqlx::test] + async fn graphql_task_queries_share_the_task_read_ip_brake(pool: PgPool) { + use std::net::SocketAddr; + + let mut state = test_state(pool).await; + // Two slots, so the third task field from this IP is braked. + state.task_read_rate_limiter = + crate::rate_limit::RateLimiter::new(2, std::time::Duration::from_secs(3600)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let router = crate::server::build_router(state); + let peer: SocketAddr = "198.51.100.7:5000".parse().unwrap(); + let query = |peer: SocketAddr, q: &str| { + let mut req = Request::builder() + .method(Method::POST) + .uri("/graphql") + .header("content-type", "application/json") + .body(Body::from(serde_json::json!({ "query": q }).to_string())) + .unwrap(); + req.extensions_mut() + .insert(axum::extract::ConnectInfo(peer)); + req + }; + let run = |router: Router, peer: SocketAddr, q: &'static str| async move { + let resp = router.oneshot(query(peer, q)).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK, "GraphQL answers 200: {q}"); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + serde_json::from_slice::(&bytes).unwrap() + }; + let braked = |body: &serde_json::Value| { + body["errors"].as_array().is_some_and(|errs| { + errs.iter() + .any(|e| e["message"].as_str() == Some(crate::rate_limit::RATE_LIMIT_MESSAGE)) + }) + }; + + // Anonymous list, then anonymous single-id lookup: both run the gate, + // both spend a slot. + for q in ["{ tasks { items { id } } }", "{ task(id: \"t1\") { id } }"] { + let body = run(router.clone(), peer, q).await; + // Asserting the whole `errors` key is absent, not merely that the + // brake message is missing: a query that failed for some other + // reason would still spend its slot and leave this test vacuous. + assert!( + body.get("errors").is_none(), + "{q}: the first probes must pass the brake and resolve, got {body}" + ); + } + + let body = run(router.clone(), peer, "{ tasks { items { id } } }").await; + assert!( + braked(&body), + "an exhausted per-IP bucket must brake the GraphQL task query too, \ + got {body}" + ); + assert!( + body["data"]["tasks"].is_null(), + "a braked field must resolve to null, not answer with rows: {body}" + ); + + // The bucket is the one `task_read_routes` debits, not a second budget: + // the REST route is already exhausted by the GraphQL traffic above. + let mut rest = Request::builder() + .method(Method::GET) + .uri("/api/v1/tasks") + .body(Body::empty()) + .unwrap(); + rest.extensions_mut() + .insert(axum::extract::ConnectInfo(peer)); + let resp = router.clone().oneshot(rest).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::TOO_MANY_REQUESTS, + "GraphQL and REST task reads must share one per-IP bucket" + ); + + let other: SocketAddr = "198.51.100.8:5000".parse().unwrap(); + let body = run(router, other, "{ tasks { items { id } } }").await; + assert!( + !braked(&body), + "a different IP must not be braked by another IP's exhausted bucket" + ); + } + + /// #327 review: aliased GraphQL task queries execute within a single request, + /// so a request-scoped cap prevents an anonymous prober from exhausting the + /// rate limit or running excessive visibility scans in one POST. + #[sqlx::test] + async fn graphql_aliased_task_queries_are_capped_per_request(pool: PgPool) { + use std::net::SocketAddr; + + let mut state = test_state(pool).await; + // Large per-IP budget so per-request cap is what triggers first. + state.task_read_rate_limiter = + crate::rate_limit::RateLimiter::new(100, std::time::Duration::from_secs(3600)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let router = crate::server::build_router(state); + let peer: SocketAddr = "198.51.100.9:5000".parse().unwrap(); + let query = |peer: SocketAddr, q: &str| { + let mut req = Request::builder() + .method(Method::POST) + .uri("/graphql") + .header("content-type", "application/json") + .body(Body::from(serde_json::json!({ "query": q }).to_string())) + .unwrap(); + req.extensions_mut() + .insert(axum::extract::ConnectInfo(peer)); + req + }; + + let aliased_query = "{ \ + a1: tasks { items { id } } \ + a2: tasks { items { id } } \ + a3: tasks { items { id } } \ + a4: tasks { items { id } } \ + a5: tasks { items { id } } \ + a6: tasks { items { id } } \ + }"; + let resp = router.oneshot(query(peer, aliased_query)).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let body = serde_json::from_slice::(&bytes).unwrap(); + assert!( + body["errors"].as_array().is_some_and(|errs| { + errs.iter() + .any(|e| e["message"].as_str() == Some(crate::rate_limit::RATE_LIMIT_MESSAGE)) + }), + "excess aliased fields beyond per-request limit must be rejected, got {body}" + ); + } + + /// #327 review: trailing denied tasks must not advertise `has_more = true` + /// or leak the existence of private rows through pagination metadata. + #[sqlx::test] + async fn trailing_denied_tasks_do_not_set_has_more_or_leak_existence(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + state + .db + .create_repo(&repo("private-repo", DELEGATOR, "private", false)) + .await + .unwrap(); + + // 2 public tasks, followed by 3 private tasks + for (id, repo_id, ts) in [ + ("pub-2", "public-repo", "2026-01-05T00:00:00Z"), + ("pub-1", "public-repo", "2026-01-04T00:00:00Z"), + ("priv-3", "private-repo", "2026-01-03T00:00:00Z"), + ("priv-2", "private-repo", "2026-01-02T00:00:00Z"), + ("priv-1", "private-repo", "2026-01-01T00:00:00Z"), + ] { + let mut t = task(id, Some(repo_id), DELEGATOR); + t.created_at = ts.to_string(); + t.updated_at = ts.to_string(); + state.db.create_task(&t).await.unwrap(); + } + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks?limit=2")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + let tasks = body["tasks"].as_array().unwrap(); + assert_eq!(tasks.len(), 2); + assert_eq!(tasks[0]["id"], "pub-2"); + assert_eq!(tasks[1]["id"], "pub-1"); + assert_eq!( + body["has_more"], false, + "has_more must be false when only denied tasks trail the page, got {body}" + ); + assert_eq!( + body["incomplete"], false, + "incomplete must be false when all visible tasks have been delivered, got {body}" + ); + assert!( + body["next_cursor"].is_null(), + "next_cursor must be null when no more visible tasks exist, got {body}" + ); + } + + /// #327 review: a cursor records how far a scan got under *one* caller's + /// visibility, so presenting it as a different caller must fail rather + /// than resume. Here an anonymous page stops at a public task, having + /// already examined and denied the delegator's private one that sorts + /// ahead of it. Resuming that token as the delegator would start their + /// scan past their own task and drop it from the answer with nothing to + /// signal the loss. + #[sqlx::test] + async fn a_cursor_minted_anonymously_cannot_resume_an_authenticated_scan(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + // `created_at DESC, id DESC`: the delegator-only task sorts first, so + // it sits *before* the position the anonymous page stops at. + for (id, repo_id, ts) in [ + ("priv-1", None, "2026-01-03T00:00:00Z"), + ("pub-2", Some("public-repo"), "2026-01-02T00:00:00Z"), + ("pub-1", Some("public-repo"), "2026-01-01T00:00:00Z"), + ] { + let mut t = task(id, repo_id, DELEGATOR); + t.created_at = ts.to_string(); + t.updated_at = ts.to_string(); + state.db.create_task(&t).await.unwrap(); + } + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["tasks"][0]["id"], "pub-2"); + assert_eq!(body["has_more"], true); + let anon_cursor = body["next_cursor"] + .as_str() + .expect("a filled page hands back a continuation") + .to_string(); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + &format!("/api/v1/tasks?limit=50&cursor={anon_cursor}"), + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "a cursor bound to anonymous must not resume the delegator's scan" + ); + let body = body_json(resp).await; + assert_eq!(body["message"], "invalid or expired cursor"); + + // Load-bearing: the delegator really can read `priv-1`, so accepting + // that cursor would have silently dropped a row they are entitled to + // see rather than merely re-ordering their page. + let resp = list_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks?limit=50", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + let ids: Vec<&str> = body["tasks"] + .as_array() + .unwrap() + .iter() + .map(|t| t["id"].as_str().unwrap()) + .collect(); + assert_eq!(ids, ["priv-1", "pub-2", "pub-1"]); + } + + /// #327 review: `--limit 500` printed a successful but silently truncated + /// 200-row result. The page now fills, says so, and hands back a cursor + /// that reaches the rest. + #[sqlx::test] + async fn full_page_advertises_has_more_and_enumerates_past_the_row_cap(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let total = (MAX_VISIBLE_TASKS + 50) as usize; + for i in 0..total { + let mut t = task(&format!("visible-{i:04}"), Some("public-repo"), DELEGATOR); + // Descending ids so `created_at DESC, id DESC` yields visible-0249 + // first: order is asserted below, not assumed. + t.created_at = format!("2026-01-01T00:00:{:02}Z", i % 60); + t.updated_at = t.created_at.clone(); + state.db.create_task(&t).await.unwrap(); + } + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks?limit=500")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!( + body["count"], MAX_VISIBLE_TASKS, + "a request above the row cap is clamped, not served in full" + ); + assert_eq!( + body["limit"], MAX_VISIBLE_TASKS, + "the response must state the effective limit it applied" + ); + assert_eq!( + body["has_more"], true, + "a filled page with rows behind it must advertise a continuation" + ); + assert_eq!( + body["incomplete"], false, + "a filled page is not an interrupted scan" + ); + + let mut seen: Vec = body["tasks"] + .as_array() + .unwrap() + .iter() + .map(|t| t["id"].as_str().unwrap().to_string()) + .collect(); + let cursor = body["next_cursor"].as_str().unwrap().to_string(); + + let resp = list_router(state) + .oneshot(anon_get(&format!( + "/api/v1/tasks?limit=500&cursor={cursor}" + ))) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 50); + assert_eq!(body["has_more"], false); + assert!(body["next_cursor"].is_null()); + seen.extend( + body["tasks"] + .as_array() + .unwrap() + .iter() + .map(|t| t["id"].as_str().unwrap().to_string()), + ); + + let mut unique = seen.clone(); + unique.sort(); + unique.dedup(); + assert_eq!( + unique.len(), + total, + "paging must enumerate every visible row exactly once, no skips or repeats" + ); + } + + /// The minimal shape of a mid-batch page fill: fewer rows than one SQL + /// batch, and a limit smaller than that. A short batch means no rows exist + /// *past* it, not that every row *in* it was examined, so treating the two + /// as the same drops every row after the one that filled the page. + #[sqlx::test] + async fn short_batch_that_fills_the_page_still_offers_a_continuation(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + for (id, ts) in [ + ("t-3", "2026-01-03T00:00:00Z"), + ("t-2", "2026-01-02T00:00:00Z"), + ("t-1", "2026-01-01T00:00:00Z"), + ] { + let mut t = task(id, Some("public-repo"), DELEGATOR); + t.created_at = ts.into(); + t.updated_at = t.created_at.clone(); + state.db.create_task(&t).await.unwrap(); + } + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["tasks"][0]["id"], "t-3"); + assert_eq!( + body["has_more"], true, + "two rows remain in the same batch, so this is not the end of the stream: {body}" + ); + let cursor = body["next_cursor"] + .as_str() + .expect("a continuation must be offered") + .to_string(); + + let resp = list_router(state) + .oneshot(anon_get(&format!("/api/v1/tasks?limit=1&cursor={cursor}"))) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!( + body["tasks"][0]["id"], "t-2", + "the continuation must resume inside the batch, not past it: {body}" + ); + } + + /// Rows sharing a timestamp are the case a mis-ordered cursor skips or + /// repeats. The token carries the stored `created_at` verbatim, so the + /// `(created_at, id)` tie-break holds across a page boundary that lands + /// inside a group of equal timestamps. + #[sqlx::test] + async fn paging_across_equal_timestamps_neither_skips_nor_repeats(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + // Deliberately mixed spellings of the *same* instant, as a peer or an + // older writer could have stored them. + for (i, ts) in [ + "2026-01-01T00:00:00Z", + "2026-01-01T00:00:00+00:00", + "2026-01-01T00:00:00.000+00:00", + "2026-01-01T00:00:00Z", + "2026-01-01T00:00:00+00:00", + ] + .iter() + .enumerate() + { + let mut t = task(&format!("tie-{i}"), Some("public-repo"), DELEGATOR); + t.created_at = (*ts).into(); + t.updated_at = t.created_at.clone(); + state.db.create_task(&t).await.unwrap(); + } + + let mut seen: Vec = Vec::new(); + let mut cursor: Option = None; + for _ in 0..10 { + let uri = match &cursor { + Some(c) => format!("/api/v1/tasks?limit=2&cursor={}", c), + None => "/api/v1/tasks?limit=2".to_string(), + }; + let resp = list_router(state.clone()) + .oneshot(anon_get(&uri)) + .await + .unwrap(); + let body = body_json(resp).await; + for t in body["tasks"].as_array().unwrap() { + seen.push(t["id"].as_str().unwrap().to_string()); + } + match body["next_cursor"].as_str() { + Some(c) => cursor = Some(c.to_string()), + None => break, + } + } + + let mut unique = seen.clone(); + unique.sort(); + unique.dedup(); + assert_eq!( + seen.len(), + 5, + "five rows sharing an instant must be returned exactly once each: {seen:?}" + ); + assert_eq!(unique.len(), 5, "no row may repeat across pages: {seen:?}"); + } + + #[sqlx::test] + async fn list_tasks_closed_pool_returns_503_db_unavailable(pool: PgPool) { + let state = test_state(pool.clone()).await; + pool.close().await; + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks")) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::SERVICE_UNAVAILABLE, + "closed-pool outage must be retryable 503, not 500" + ); + let body = body_json(resp).await; + assert_eq!(body["error"], "db_unavailable"); + } + + #[sqlx::test] + async fn get_task_closed_pool_returns_503_db_unavailable(pool: PgPool) { + let state = test_state(pool.clone()).await; + pool.close().await; + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks/t1")) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::SERVICE_UNAVAILABLE, + "closed-pool outage must be retryable 503, not 500" + ); + let body = body_json(resp).await; + assert_eq!(body["error"], "db_unavailable"); + } + + /// Equal-timestamp siblings are where a cursor with the wrong ordering + /// domain skips or repeats a row. The token carries the served + /// `created_at` byte-for-byte, so the `(created_at, id)` tie-break + /// advances one row at a time through a fractional-zero sibling group. + #[sqlx::test] + async fn keyset_advances_across_trailing_zero_fraction_timestamps(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + + let ts_sibling = "2026-06-01T00:00:00.000000000+00:00"; + for (id, ts) in [ + ("task-2", ts_sibling), + ("task-1", ts_sibling), + ("task-0", "2026-05-01T00:00:00.000000000+00:00"), + ] { + let mut t = task(id, Some("public-repo"), DELEGATOR); + t.created_at = ts.into(); + t.updated_at = t.created_at.clone(); + state.db.create_task(&t).await.unwrap(); + } + + let mut cursor: Option = None; + let mut seen: Vec = Vec::new(); + for _ in 0..5 { + let uri = match &cursor { + Some(c) => format!("/api/v1/tasks?limit=1&cursor={c}"), + None => "/api/v1/tasks?limit=1".to_string(), + }; + let resp = list_router(state.clone()) + .oneshot(anon_get(&uri)) + .await + .unwrap(); + let body = body_json(resp).await; + for t in body["tasks"].as_array().unwrap() { + // The response must echo the stored spelling verbatim; that is + // the string the token compares against. + if t["id"] != "task-0" { + assert_eq!(t["created_at"], ts_sibling); + } + seen.push(t["id"].as_str().unwrap().to_string()); + } + match body["next_cursor"].as_str() { + Some(c) => cursor = Some(c.to_string()), + None => break, + } + } + + assert_eq!( + seen, + vec![ + "task-2".to_string(), + "task-1".to_string(), + "task-0".to_string() + ], + "paging must advance one row at a time through equal timestamps" + ); + } + + fn full_task_router(state: crate::state::AppState) -> Router { + Router::new() + .route("/api/v1/tasks", axum::routing::get(super::list_tasks)) + .route("/api/v1/tasks/{id}", axum::routing::get(super::get_task)) + .route( + "/api/v1/tasks/{id}/claim", + axum::routing::post(super::claim_task), ) - })?; - let _ = state.task_event_tx.send(TaskEventBroadcast { - task_id: id, - old_status: "claimed".to_string(), - new_status: "failed".to_string(), - by_did, - at: Utc::now().to_rfc3339(), - }); - Ok(Json(task_to_json(&task))) + .route( + "/api/v1/tasks/{id}/complete", + axum::routing::post(super::complete_task), + ) + .route( + "/api/v1/tasks/{id}/fail", + axum::routing::post(super::fail_task), + ) + .with_state(state) + } + + fn assert_not_found_envelope(body: &serde_json::Value) { + assert_eq!(body["error"], "not_found"); + assert_eq!(body["message"], "task not found"); + let serialized = body.to_string(); + assert!(!serialized.contains(SECRET_UCAN)); + assert!(!serialized.contains("payload-data")); + } + + #[sqlx::test] + async fn complete_and_fail_task_on_invisible_task_returns_404_not_403(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let complete_resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/t1/complete", + Body::from(r#"{"result":"done"}"#), + )) + .await + .unwrap(); + assert_eq!( + complete_resp.status(), + StatusCode::NOT_FOUND, + "completing an invisible task must 404, not leak existence via 403" + ); + assert_not_found_envelope(&body_json(complete_resp).await); + + let fail_resp = full_task_router(state) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/t1/fail", + Body::from(r#"{"reason":"error"}"#), + )) + .await + .unwrap(); + assert_eq!( + fail_resp.status(), + StatusCode::NOT_FOUND, + "failing an invisible task must 404, not leak existence via 403" + ); + assert_not_found_envelope(&body_json(fail_resp).await); + } + + #[sqlx::test] + async fn claim_task_on_invisible_task_returns_404_not_success_or_409(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let claim_resp = full_task_router(state) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/t1/claim", + Body::from(format!(r#"{{"assignee_did":"{STRANGER}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + claim_resp.status(), + StatusCode::NOT_FOUND, + "claiming an invisible task must 404, not succeed or leak via 409" + ); + assert_not_found_envelope(&body_json(claim_resp).await); + } + + /// Goes RED if `claim_task`'s `assignee_did IS NULL OR assignee_did = $2` + /// predicate is deleted: a public-repo pre-assigned task is visible to a + /// stranger, so only the SQL guard stops them from overwriting the + /// designated assignee. + #[sqlx::test] + async fn claim_task_does_not_steal_preassigned_assignee(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let mut assigned = task("preassigned", Some("public-repo"), DELEGATOR); + assigned.assignee_did = Some(ASSIGNEE.into()); + state.db.create_task(&assigned).await.unwrap(); + + let stranger_resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/preassigned/claim", + Body::from(format!(r#"{{"assignee_did":"{STRANGER}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + stranger_resp.status(), + StatusCode::CONFLICT, + "a stranger must not claim a task pre-assigned to someone else" + ); + let stranger_body = body_json(stranger_resp).await; + assert!(!stranger_body.to_string().contains(SECRET_UCAN)); + assert_eq!( + state + .db + .get_task("preassigned") + .await + .unwrap() + .unwrap() + .assignee_did + .as_deref(), + Some(ASSIGNEE), + "hostile claim must leave the designated assignee in place" + ); + + let assignee_resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + ASSIGNEE, + Method::POST, + "/api/v1/tasks/preassigned/claim", + Body::from(format!(r#"{{"assignee_did":"{ASSIGNEE}"}}"#)), + )) + .await + .unwrap(); + assert_eq!(assignee_resp.status(), StatusCode::OK); + let claimed = body_json(assignee_resp).await; + assert_eq!(claimed["status"], "claimed"); + assert_eq!(claimed["assignee_did"], ASSIGNEE); + + let second_resp = full_task_router(state) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/preassigned/claim", + Body::from(format!(r#"{{"assignee_did":"{STRANGER}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + second_resp.status(), + StatusCode::CONFLICT, + "a second claim after the assignee took the task must be refused" + ); + } + + /// `create_task` stores the supplied assignee form unchanged. Claim binds + /// the authenticated DID (typically `did:key:...`) and list filters pass + /// the query string through. Both SQL comparisons must collapse the + /// did:key short form, or a designated assignee stored as a bare key + /// cannot claim, and a `?assignee_did=` filter in the other form drops + /// the row. A `did:web:` assignee sharing the same residual must stay + /// unmatched. + #[sqlx::test] + async fn claim_and_list_match_bare_and_did_key_assignee_forms(pool: PgPool) { + let bare_assignee = crate::db::normalize_owner_key(ASSIGNEE); + assert_ne!( + bare_assignee, ASSIGNEE, + "test setup requires ASSIGNEE to be the full did:key form" + ); + + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + + let mut bare = task("bare-assignee", Some("public-repo"), DELEGATOR); + bare.assignee_did = Some(bare_assignee.into()); + state.db.create_task(&bare).await.unwrap(); + + let mut full = task("full-assignee", Some("public-repo"), DELEGATOR); + full.assignee_did = Some(ASSIGNEE.into()); + state.db.create_task(&full).await.unwrap(); + + let mut web = task("web-assignee", Some("public-repo"), DELEGATOR); + web.assignee_did = Some(format!("did:web:{bare_assignee}")); + state.db.create_task(&web).await.unwrap(); + + let listed_ids = |body: &serde_json::Value| -> Vec { + body["tasks"] + .as_array() + .unwrap() + .iter() + .map(|task| task["id"].as_str().unwrap().to_string()) + .collect() + }; + + let full_filter = list_router(state.clone()) + .oneshot(anon_get(&format!("/api/v1/tasks?assignee_did={ASSIGNEE}"))) + .await + .unwrap(); + assert_eq!(full_filter.status(), StatusCode::OK); + let full_ids = listed_ids(&body_json(full_filter).await); + assert!( + full_ids.contains(&"bare-assignee".to_string()), + "a did:key: filter must match a bare stored assignee" + ); + assert!(full_ids.contains(&"full-assignee".to_string())); + assert!( + !full_ids.contains(&"web-assignee".to_string()), + "did:key matching must not collapse a did:web assignee" + ); + + let bare_filter = list_router(state.clone()) + .oneshot(anon_get(&format!( + "/api/v1/tasks?assignee_did={bare_assignee}" + ))) + .await + .unwrap(); + assert_eq!(bare_filter.status(), StatusCode::OK); + let bare_ids = listed_ids(&body_json(bare_filter).await); + assert!( + bare_ids.contains(&"full-assignee".to_string()), + "a bare filter must match a did:key stored assignee" + ); + assert!(bare_ids.contains(&"bare-assignee".to_string())); + assert!( + !bare_ids.contains(&"web-assignee".to_string()), + "did:key matching must not collapse a did:web assignee" + ); + + let claim_full = full_task_router(state.clone()) + .oneshot(signed_request_as( + ASSIGNEE, + Method::POST, + "/api/v1/tasks/bare-assignee/claim", + Body::from(format!(r#"{{"assignee_did":"{ASSIGNEE}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + claim_full.status(), + StatusCode::OK, + "claim as did:key: form must match a bare stored assignee" + ); + + let claim_bare = full_task_router(state) + .oneshot(signed_request_as( + bare_assignee, + Method::POST, + "/api/v1/tasks/full-assignee/claim", + Body::from(format!(r#"{{"assignee_did":"{bare_assignee}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + claim_bare.status(), + StatusCode::OK, + "claim as a bare key must match a did:key stored assignee" + ); + } + + /// Goes RED if `announce_task_event` is replaced with a bare `tx.send`: + /// a repo-less claim would then reach an anonymous subscriber. + #[sqlx::test] + async fn announce_task_event_skips_tasks_invisible_to_anonymous(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + state + .db + .create_task(&task("pub-t", Some("public-repo"), DELEGATOR)) + .await + .unwrap(); + state + .db + .create_task(&task("priv-t", None, DELEGATOR)) + .await + .unwrap(); + + let mut events = state.task_event_tx.subscribe(); + + let pub_resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + ASSIGNEE, + Method::POST, + "/api/v1/tasks/pub-t/claim", + Body::from(format!(r#"{{"assignee_did":"{ASSIGNEE}"}}"#)), + )) + .await + .unwrap(); + assert_eq!(pub_resp.status(), StatusCode::OK); + let broadcast = events + .try_recv() + .expect("a publicly visible claim must broadcast"); + assert_eq!(broadcast.task_id, "pub-t"); + assert_eq!(broadcast.new_status, "claimed"); + + let priv_resp = full_task_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::POST, + "/api/v1/tasks/priv-t/claim", + Body::from(format!(r#"{{"assignee_did":"{DELEGATOR}"}}"#)), + )) + .await + .unwrap(); + assert_eq!(priv_resp.status(), StatusCode::OK); + assert!( + events.try_recv().is_err(), + "a repo-less claim must not reach an anonymous subscriber" + ); + } + + /// #327 review: the scan-ceiling probe in `collect_visible_tasks` is + /// un-gated, so a filled page that stops at the ceiling reports + /// `has_more = true` when *any* candidate row trails the scan position, + /// including rows the caller may not read. That bit is intentional and + /// bounded — this pins the bound end to end. + /// + /// One newest public task, then more than a full scan budget of denied + /// rows and nothing visible beyond them. The anonymous caller gets its + /// visible row plus a continuation, and following that continuation + /// returns the terminal page the probe predicted. What the caller learns + /// is therefore exactly what one more request would have told it anyway: + /// no denied row's id, payload or `ucan_token` is disclosed at any point, + /// and the walk ends instead of spinning. + #[sqlx::test] + async fn scan_ceiling_continuation_discloses_only_a_terminal_page(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let mut visible_newest = task("newest-visible", Some("public-repo"), DELEGATOR); + visible_newest.created_at = "2026-01-03T00:00:00Z".into(); + visible_newest.updated_at = visible_newest.created_at.clone(); + state.db.create_task(&visible_newest).await.unwrap(); + + // More than one scan budget of unreadable rows, with no visible row + // behind them, so the first request stops at the ceiling with rows + // still trailing it. + for i in 0..(MAX_TASK_SCAN_CANDIDATES + 5) { + let mut hidden = task(&format!("hidden-{i:05}"), None, DELEGATOR); + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + state.db.create_task(&hidden).await.unwrap(); + } + + let first = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + assert_eq!(first.status(), StatusCode::OK); + let body = body_json(first).await; + assert_eq!( + body["tasks"][0]["id"], "newest-visible", + "the visible row must still be served: {body}" + ); + assert_eq!(body["count"], 1); + assert_eq!( + body["incomplete"], false, + "a page that filled is not a paused scan: {body}" + ); + assert_eq!( + body["has_more"], true, + "the ceiling hands back a continuation so a longer denied window stays pageable: {body}" + ); + let mut cursor = body["next_cursor"] + .as_str() + .expect("a continuation must accompany has_more") + .to_string(); + assert!( + !body.to_string().contains("hidden-"), + "no response may disclose a denied row's id: {body}" + ); + assert!( + !body.to_string().contains(SECRET_UCAN), + "no response may disclose a ucan token: {body}" + ); + + // Following the server's own continuation reaches the end of the + // stream without ever surfacing a denied row. + let mut requests = 1; + let terminal = loop { + requests += 1; + assert!(requests <= 4, "the continuation must terminate, not spin"); + let resp = list_router(state.clone()) + .oneshot(anon_get(&format!("/api/v1/tasks?limit=1&cursor={cursor}"))) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let page = body_json(resp).await; + assert_eq!( + page["count"], 0, + "nothing visible trails the denied window: {page}" + ); + assert!( + !page.to_string().contains("hidden-"), + "no response may disclose a denied row's id: {page}" + ); + assert!( + !page.to_string().contains(SECRET_UCAN), + "no response may disclose a ucan token: {page}" + ); + match page["next_cursor"].as_str() { + Some(next) => cursor = next.to_string(), + None => break page, + } + }; + + assert_eq!( + terminal["has_more"], false, + "the walk must end at a terminal page: {terminal}" + ); + assert_eq!( + terminal["incomplete"], false, + "a terminal page is complete, not incomplete: {terminal}" + ); + } + + #[sqlx::test] + async fn exhausted_scan_of_exactly_ceiling_candidates_is_not_incomplete(pool: PgPool) { + let state = test_state(pool).await; + for i in 0..MAX_TASK_SCAN_CANDIDATES { + let mut hidden = task(&format!("hidden-{i:04}"), None, DELEGATOR); + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + state.db.create_task(&hidden).await.unwrap(); + } + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 0); + assert_eq!( + body["incomplete"], false, + "exactly {MAX_TASK_SCAN_CANDIDATES} denied rows with nothing beyond is a finished stream" + ); + } + + #[sqlx::test] + async fn task_mutations_closed_pool_returns_503_db_unavailable(pool: PgPool) { + let state = test_state(pool.clone()).await; + pool.close().await; + + for (uri, body) in [ + ( + "/api/v1/tasks/t1/claim", + format!(r#"{{"assignee_did":"{ASSIGNEE}"}}"#), + ), + ("/api/v1/tasks/t1/complete", r#"{"result":"done"}"#.into()), + ("/api/v1/tasks/t1/fail", r#"{"reason":"error"}"#.into()), + ] { + let resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + ASSIGNEE, + Method::POST, + uri, + Body::from(body), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::SERVICE_UNAVAILABLE, + "{uri}: closed-pool outage during visibility pre-check must be retryable 503" + ); + let json = body_json(resp).await; + assert_eq!(json["error"], "db_unavailable", "{uri}"); + } + } } diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 27b67786e..2890af259 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -497,10 +497,12 @@ mod tests { .connect_lazy("postgres://localhost/gitlawb_test_placeholder") .expect("lazy pool creation should not fail"); let db = Arc::new(crate::db::Db::for_testing(pool.clone())); + let task_cursor_key = crate::api::task_cursor::TaskCursorKey::derive(&keypair.to_seed()); let schema = Arc::new(graphql::build_schema( db.clone(), ref_tx.clone(), task_tx.clone(), + task_cursor_key.clone(), )); crate::state::AppState { config: Arc::new(Config::parse_from(["gitlawb-node"])), @@ -512,6 +514,7 @@ mod tests { ref_update_tx: ref_tx, task_event_tx: task_tx, graphql_schema: schema, + task_cursor_key, machine_id: None, repo_store: crate::git::repo_store::RepoStore::for_testing(PathBuf::from("/tmp"), pool), rate_limiter: RateLimiter::new(100, Duration::from_secs(60)), @@ -545,6 +548,7 @@ mod tests { git_ipfs_walk_semaphore: Arc::new(tokio::sync::Semaphore::new(64)), git_ipfs_walk_per_caller: crate::rate_limit::PerCallerConcurrency::with_default_max_keys(16), + task_read_rate_limiter: RateLimiter::new(1200, Duration::from_secs(3600)), git_bin: "git".to_string(), } } diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index 1fbf4376e..0494f2df1 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -702,6 +702,18 @@ pub struct Config { value_parser = clap::builder::RangedU64ValueParser::::new().range(0..=86_400) )] pub pin_repair_sweep_delay_secs: u64, + + /// Per-client-IP rate limit for the anonymous task read routes + /// (`GET /api/v1/tasks`, `GET /api/v1/tasks/{id}`), in requests per hour. + /// Both are publicly reachable (`optional_signature`), and each request runs + /// the visibility gate — a task lookup plus deduped-repo and visibility-rule + /// queries — before it can return the opaque 404, so an anonymous prober pays + /// nothing while the node pays for every probe. Keyed on the resolved client + /// IP via `GITLAWB_TRUSTED_PROXY`. `0` disables. Default: 1200 (a list page + /// followed by per-task reads is a normal client pattern, so this sits above + /// the `/ipfs` budget). + #[arg(long, env = "GITLAWB_TASK_READ_RATE_LIMIT", default_value_t = 1200)] + pub task_read_rate_limit: usize, } impl Config { diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index cc2cf0bd7..126de9774 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -1123,6 +1123,21 @@ const MIGRATIONS: &[Migration] = &[ "ALTER TABLE pin_repair_sweep ADD COLUMN IF NOT EXISTS discovery_cursor_id TEXT NOT NULL DEFAULT ''", ], }, + Migration { + version: 27, + name: "agent_tasks_assignee_key_didkey_aware", + stmts: &[ + // Filtering agent_tasks by assignee normalizes did:key values via + // ASSIGNEE_DID_CASE_SQL, making the raw idx_agent_tasks_assignee + // index unusable. Swap the index: drop the raw column index and + // build the matching expression index so list queries use an Index Cond. + // The CASE must stay byte-identical to ASSIGNEE_DID_CASE_SQL so + // Postgres matches it. + "DROP INDEX IF EXISTS idx_agent_tasks_assignee", + // Keep byte-identical to ASSIGNEE_DID_CASE_SQL so Postgres uses the index. + "CREATE INDEX IF NOT EXISTS idx_agent_tasks_assignee_key ON agent_tasks ((CASE WHEN assignee_did LIKE 'did:key:%' AND position(':' in substr(assignee_did, 9)) = 0 THEN substr(assignee_did, 9) ELSE assignee_did END))", + ], + }, ]; /// Max distinct source repos recorded per pinned object (F1, #173 jatmn round 8). @@ -1149,6 +1164,10 @@ const OWNER_KEY_CASE_SQL: &str = "CASE WHEN owner_did LIKE 'did:key:%' AND posit /// named `did` (like in agent_profiles) instead of `owner_did`. const PROFILE_DID_CASE_SQL: &str = "CASE WHEN did LIKE 'did:key:%' AND position(':' in substr(did, 9)) = 0 THEN substr(did, 9) ELSE did END"; +/// SQL CASE expression byte-identical to `normalize_owner_key`, but for the +/// `assignee_did` column on `agent_tasks`. +const ASSIGNEE_DID_CASE_SQL: &str = "CASE WHEN assignee_did LIKE 'did:key:%' AND position(':' in substr(assignee_did, 9)) = 0 THEN substr(assignee_did, 9) ELSE assignee_did END"; + #[cfg(test)] mod normalize_owner_key_tests { use super::normalize_owner_key; @@ -1181,6 +1200,11 @@ mod normalize_owner_key_tests { ); } + #[test] + fn leaves_single_residual_web_did_intact() { + assert_eq!(normalize_owner_key("did:web:z6Mkfoo"), "did:web:z6Mkfoo"); + } + #[test] fn does_not_strip_did_key_with_extra_colon() { // did:key:did:gitlawb:z6... — the remainder contains ':', so it's left whole. @@ -1426,11 +1450,13 @@ impl Db { /// Shared dedup CTE: collapses the mirror row and the canonical row of one /// logical repo into a single survivor. `$1` is an optional owner filter - /// (NULL = all rows). Grouping collapses on a did:key-aware owner key: strip a - /// `did:key:` prefix (8 chars, so `substr(owner_did, 9)`) only when the - /// remainder is a bare id with no `:`, otherwise keep the full DID. That is the - /// exact normalization in `crate::api::did_matches`, so `did:key:X` and a bare - /// `X` collapse while distinct DID methods (`did:gitlawb:X`) never merge. The + /// (NULL = all rows). `$2` optionally scopes the work to the logical groups + /// containing the supplied repo ids. Grouping collapses on a did:key-aware + /// owner key: strip a `did:key:` prefix (8 chars, so + /// `substr(owner_did, 9)`) only when the remainder is a bare id with no `:`, + /// otherwise keep the full DID. That is the exact normalization in + /// `crate::api::did_matches`, so `did:key:X` and a bare `X` collapse while + /// distinct DID methods (`did:gitlawb:X`) never merge. The /// CASE is repeated verbatim in `count_repos_deduped` and the v7 index and must /// stay byte-identical or Postgres stops using the index. /// The canonical row wins (mirror rows carry a slash-form `id` written only by @@ -1440,7 +1466,12 @@ impl Db { /// `crate::api::repos::dedupe_canonical_repos` must stay in sync. fn dedup_cte() -> String { format!( - "WITH deduped AS ( + "WITH requested_groups AS ( + SELECT DISTINCT {key} AS owner_key, name + FROM repos + WHERE $2::text[] IS NOT NULL AND id = ANY($2) + ), + deduped AS ( SELECT DISTINCT ON ({key}, name) id, name, owner_did, description, is_public, default_branch, created_at, @@ -1459,6 +1490,11 @@ impl Db { -- Quarantined mirrors (admitted but unvalidated by the iCaptcha -- propagation gate) are withheld from every listing surface. WHERE quarantined = FALSE AND ($1::text IS NULL OR ({key}) = $1) + AND ($2::text[] IS NULL OR EXISTS ( + SELECT 1 FROM requested_groups requested + WHERE requested.owner_key = ({key}) + AND requested.name = repos.name + )) ORDER BY {key}, name, -- mirror rows carry a slash-form id (\"{{owner_short}}/{{name}}\"), -- written only by upsert_mirror_repo; canonical ids are UUIDs. @@ -1506,6 +1542,7 @@ impl Db { ); let rows = sqlx::query(&sql) .bind(owner_key) + .bind(None::<&[String]>) .fetch_all(&self.pool) .await?; @@ -1534,6 +1571,32 @@ impl Db { ); let rows = sqlx::query(&sql) .bind(None::<&str>) + .bind(None::<&[String]>) + .fetch_all(&self.pool) + .await?; + + Ok(rows.into_iter().map(row_to_repo).collect()) + } + + /// Resolve only the requested repository ids through the same canonical + /// survivor and quarantine rules as `list_all_repos_deduped`. + pub async fn list_repos_deduped_by_ids(&self, repo_ids: &[String]) -> Result> { + if repo_ids.is_empty() { + return Ok(Vec::new()); + } + let sql = format!( + "{} + SELECT d.id, d.name, d.owner_did, d.description, d.is_public, + d.default_branch, d.created_at, d.updated_at, d.disk_path, + d.forked_from, d.machine_id + FROM deduped d + WHERE d.id = ANY($2) + ORDER BY d.updated_at DESC", + Self::dedup_cte() + ); + let rows = sqlx::query(&sql) + .bind(None::<&str>) + .bind(repo_ids) .fetch_all(&self.pool) .await?; @@ -3697,61 +3760,58 @@ impl Db { Ok(row.map(row_to_task)) } - pub async fn list_tasks( + pub async fn list_tasks_keyset( &self, status: Option<&str>, assignee_did: Option<&str>, limit: i64, + after: Option<(&str, &str)>, ) -> Result> { - let rows = match (status, assignee_did) { - (Some(s), Some(a)) => sqlx::query( - "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline - FROM agent_tasks WHERE status=$1 AND assignee_did=$2 ORDER BY created_at DESC LIMIT $3", - ) - .bind(s) - .bind(a) - .bind(limit) - .fetch_all(&self.pool) - .await?, - (Some(s), None) => sqlx::query( - "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline - FROM agent_tasks WHERE status=$1 ORDER BY created_at DESC LIMIT $2", - ) - .bind(s) - .bind(limit) - .fetch_all(&self.pool) - .await?, - (None, Some(a)) => sqlx::query( - "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline - FROM agent_tasks WHERE assignee_did=$1 ORDER BY created_at DESC LIMIT $2", - ) - .bind(a) - .bind(limit) - .fetch_all(&self.pool) - .await?, - (None, None) => sqlx::query( - "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline - FROM agent_tasks ORDER BY created_at DESC LIMIT $1", - ) + // create_task stores the supplied assignee form unchanged. Compare the + // did:key short form so a `did:key:z...` filter matches a bare `z...` + // row (and the reverse), matching `did_matches` on the read path. + let assignee_key = assignee_did.map(normalize_owner_key); + let sql = format!( + "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline + FROM agent_tasks + WHERE ($1::text IS NULL OR status = $1) + AND ($2::text IS NULL OR ({key}) = $2) + AND ($3::text IS NULL OR (created_at, id) < ($3, $4)) + ORDER BY created_at DESC, id DESC + LIMIT $5", + key = ASSIGNEE_DID_CASE_SQL + ); + let rows = sqlx::query(&sql) + .bind(status) + .bind(assignee_key) + .bind(after.map(|cursor| cursor.0)) + .bind(after.map(|cursor| cursor.1)) .bind(limit) .fetch_all(&self.pool) - .await?, - }; + .await?; Ok(rows.into_iter().map(row_to_task).collect()) } pub async fn claim_task(&self, id: &str, assignee_did: &str) -> Result { let now = Utc::now().to_rfc3339(); - let row = sqlx::query( + // Bind the presented DID for the write, and the normalized key for the + // pre-assignment guard. A designated assignee stored as a bare key + // must still be able to claim when the signer presents `did:key:...`. + let assignee_key = normalize_owner_key(assignee_did); + let sql = format!( "UPDATE agent_tasks SET status='claimed', assignee_did=$2, updated_at=$3 WHERE id=$1 AND status='pending' + AND (assignee_did IS NULL OR ({key}) = $4) RETURNING id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline", - ) - .bind(id) - .bind(assignee_did) - .bind(&now) - .fetch_optional(&self.pool) - .await?; + key = ASSIGNEE_DID_CASE_SQL + ); + let row = sqlx::query(&sql) + .bind(id) + .bind(assignee_did) + .bind(&now) + .bind(assignee_key) + .fetch_optional(&self.pool) + .await?; row.map(row_to_task) .ok_or_else(|| anyhow::anyhow!("task not claimable: not found or already claimed")) } @@ -4993,6 +5053,66 @@ mod migration_tests { db.migrate().await.unwrap(); } + #[sqlx::test] + async fn migration_v27_creates_assignee_expression_index(pool: sqlx::PgPool) { + let db = super::Db::for_testing(pool); + db.migrate().await.unwrap(); + + // Roll back: drop the expression index, restore the raw index, forget v27. + sqlx::query("DROP INDEX IF EXISTS idx_agent_tasks_assignee_key") + .execute(&db.pool) + .await + .unwrap(); + sqlx::query( + "CREATE INDEX IF NOT EXISTS idx_agent_tasks_assignee ON agent_tasks(assignee_did)", + ) + .execute(&db.pool) + .await + .unwrap(); + sqlx::query("DELETE FROM schema_migrations WHERE version = 27") + .execute(&db.pool) + .await + .unwrap(); + + // Re-run migration. + db.migrate().await.unwrap(); + + let idx_exists: (bool,) = sqlx::query_as( + "SELECT EXISTS( + SELECT 1 FROM pg_indexes + WHERE tablename = 'agent_tasks' AND indexname = 'idx_agent_tasks_assignee_key' + )", + ) + .fetch_one(&db.pool) + .await + .unwrap(); + assert!(idx_exists.0, "idx_agent_tasks_assignee_key must exist"); + + let old_idx_exists: (bool,) = sqlx::query_as( + "SELECT EXISTS( + SELECT 1 FROM pg_indexes + WHERE tablename = 'agent_tasks' AND indexname = 'idx_agent_tasks_assignee' + )", + ) + .fetch_one(&db.pool) + .await + .unwrap(); + assert!( + !old_idx_exists.0, + "idx_agent_tasks_assignee must be dropped" + ); + + let recorded: (i64,) = + sqlx::query_as("SELECT COUNT(*) FROM schema_migrations WHERE version = 27") + .fetch_one(&db.pool) + .await + .unwrap(); + assert_eq!(recorded.0, 1, "v27 must be recorded as applied"); + + // Idempotent re-run. + db.migrate().await.unwrap(); + } + #[sqlx::test] async fn dequeue_stamps_attempted_at_on_every_row_it_hands_out(pool: sqlx::PgPool) { // The stamp is what stops a deferred row from holding the window, and @@ -5261,6 +5381,36 @@ mod dedup_db_tests { ); } + #[sqlx::test] + async fn deduped_id_lookup_returns_only_requested_repo(pool: PgPool) { + let db = db(pool).await; + let requested = rec( + "requested", + "did:key:z6MkRequested", + "requested", + "requested", + "2026-01-01T00:00:00Z", + "2026-01-01T00:00:00Z", + ); + let unrelated = rec( + "unrelated", + "did:key:z6MkUnrelated", + "unrelated", + "unrelated", + "2026-01-02T00:00:00Z", + "2026-01-02T00:00:00Z", + ); + db.create_repo(&requested).await.unwrap(); + db.create_repo(&unrelated).await.unwrap(); + + let out = db + .list_repos_deduped_by_ids(std::slice::from_ref(&requested.id)) + .await + .unwrap(); + assert_eq!(out.len(), 1); + assert_eq!(out[0].id, requested.id); + } + /// A PRIVATE canonical repo and a PUBLIC mirror row for the same /// (owner, name) collapse to a single survivor whose `is_public` is the /// canonical `false`, not the mirror's `true`. `upsert_mirror_repo` always @@ -5954,6 +6104,7 @@ mod dedup_db_tests { "z6Mkfoo", "did:gitlawb:z6Mkfoo", "did:web:example.com:alice", + "did:web:z6Mkfoo", "did:key:did:gitlawb:z6Mkfoo", "", "did:key:", @@ -6000,6 +6151,7 @@ mod dedup_db_tests { "z6Mkfoo", "did:gitlawb:z6Mkfoo", "did:web:example.com:alice", + "did:web:z6Mkfoo", "did:key:did:gitlawb:z6Mkfoo", "", "did:key:", @@ -6033,6 +6185,50 @@ mod dedup_db_tests { ); } } + + /// Verify that `ASSIGNEE_DID_CASE_SQL` (which aliases `assignee_did`) also + /// agrees with Rust `normalize_owner_key` across the full boundary matrix. + #[sqlx::test] + async fn assignee_did_case_sql_matches_normalize_owner_key(pool: PgPool) { + let boundary_values = [ + "did:key:z6Mkfoo", + "z6Mkfoo", + "did:gitlawb:z6Mkfoo", + "did:web:example.com:alice", + "did:web:z6Mkfoo", + "did:key:did:gitlawb:z6Mkfoo", + "", + "did:key:", + "DID:KEY:z6Mkfoo", + ]; + + let values_sql: String = boundary_values + .iter() + .map(|v| format!("('{}'::text)", v)) + .collect::>() + .join(", "); + let sql = format!( + "WITH data(assignee_did) AS (VALUES {values_sql}) + SELECT assignee_did, ({key}) AS normalized FROM data ORDER BY assignee_did", + key = super::ASSIGNEE_DID_CASE_SQL + ); + + let rows: Vec<(String, String)> = sqlx::query_as(&sql).fetch_all(&pool).await.unwrap(); + + assert_eq!( + rows.len(), + boundary_values.len(), + "every boundary value must produce a row" + ); + + for (val, sql_result) in &rows { + let rust_result = super::normalize_owner_key(val); + assert_eq!( + sql_result, rust_result, + "ASSIGNEE_DID_CASE_SQL(\"{val}\") mismatch: Rust = \"{rust_result}\", SQL CASE = \"{sql_result}\"" + ); + } + } } /// Exercises the iCaptcha single-use proof ledger (`icaptcha_consumed_proofs`), diff --git a/crates/gitlawb-node/src/error.rs b/crates/gitlawb-node/src/error.rs index 474408e59..da1899b32 100644 --- a/crates/gitlawb-node/src/error.rs +++ b/crates/gitlawb-node/src/error.rs @@ -12,6 +12,9 @@ pub enum AppError { #[error("repo already exists: {0}")] RepoExists(String), + #[error("conflict: {0}")] + Conflict(String), + #[error("not found: {0}")] NotFound(String), @@ -156,6 +159,7 @@ impl IntoResponse for AppError { "repo_exists", format!("repository '{r}' already exists"), ), + AppError::Conflict(msg) => (StatusCode::CONFLICT, "conflict", msg.clone()), AppError::NotFound(msg) => (StatusCode::NOT_FOUND, "not_found", msg.clone()), AppError::Unauthorized(msg) => (StatusCode::UNAUTHORIZED, "not_an_agent", msg.clone()), AppError::Forbidden(msg) => (StatusCode::FORBIDDEN, "forbidden", msg.clone()), @@ -261,6 +265,16 @@ pub type Result = std::result::Result; mod tests { use super::*; + #[test] + fn conflict_maps_to_409() { + assert_eq!( + AppError::Conflict("task not claimable".into()) + .into_response() + .status(), + StatusCode::CONFLICT + ); + } + #[test] fn timeout_maps_to_504_distinct_from_git_500() { assert_eq!( diff --git a/crates/gitlawb-node/src/graphql/mod.rs b/crates/gitlawb-node/src/graphql/mod.rs index 181bcc22c..b9c74e830 100644 --- a/crates/gitlawb-node/src/graphql/mod.rs +++ b/crates/gitlawb-node/src/graphql/mod.rs @@ -60,6 +60,7 @@ pub(crate) fn graphql_app_err(e: crate::error::AppError) -> async_graphql::Error // Curated client-safe variants — `Display` is intentional API text. safe @ (crate::error::AppError::RepoNotFound(_) | crate::error::AppError::RepoExists(_) + | crate::error::AppError::Conflict(_) | crate::error::AppError::NotFound(_) | crate::error::AppError::Unauthorized(_) | crate::error::AppError::Forbidden(_) @@ -76,15 +77,38 @@ pub(crate) fn graphql_app_err(e: crate::error::AppError) -> async_graphql::Error } } +/// Classify a failed `claim_task` for GraphQL exactly as REST's claim handler +/// classifies it: a lost race is a client-safe conflict, a real sqlx fault +/// stays opaque. Lives here rather than at the three call sites so the +/// classification cannot drift between transports (#327 review), and so +/// `every_graphql_map_err_uses_opaque_helpers` can keep whitelisting by name. +pub(crate) fn graphql_claim_conflict(e: anyhow::Error) -> async_graphql::Error { + graphql_app_err(crate::api::tasks::task_write_conflict( + e, + "task not claimable: not found or already claimed", + )) +} + +/// The `finish_task` half of [`graphql_claim_conflict`], covering both +/// `completeTask` and `failTask`. +pub(crate) fn graphql_finish_conflict(e: anyhow::Error) -> async_graphql::Error { + graphql_app_err(crate::api::tasks::task_write_conflict( + e, + "task not found or not in claimed state", + )) +} + pub fn build_schema( db: Arc, ref_update_tx: tokio::sync::broadcast::Sender, task_event_tx: tokio::sync::broadcast::Sender, + task_cursor_key: crate::api::task_cursor::TaskCursorKey, ) -> GitlawbSchema { Schema::build(QueryRoot, MutationRoot, SubscriptionRoot) .data(db) .data(ref_update_tx) .data(task_event_tx) + .data(task_cursor_key) .finish() } @@ -161,8 +185,10 @@ mod tests { } /// Every `.map_err(` in the GraphQL query/mutation resolvers must route - /// through the opaque helpers, or discard the error (`|_|`). Same source- - /// scrape pattern as `api::authz_guard` (#255 review). + /// through one of the curated helpers in this module, or discard the error + /// (`|_|`). Same source-scrape pattern as `api::authz_guard` (#255 + /// review). The list is deliberately a whitelist of names rather than a + /// prefix match, so adding a new mapper is a decision a reviewer sees. #[test] fn every_graphql_map_err_uses_opaque_helpers() { for (file, src) in [ @@ -177,6 +203,8 @@ mod tests { let after = code[idx + ".map_err(".len()..].trim_start(); let ok = after.starts_with("crate::graphql::graphql_db_err") || after.starts_with("crate::graphql::graphql_app_err") + || after.starts_with("crate::graphql::graphql_claim_conflict") + || after.starts_with("crate::graphql::graphql_finish_conflict") || after.starts_with("|_|") || after.starts_with("|_ "); assert!( diff --git a/crates/gitlawb-node/src/graphql/mutation.rs b/crates/gitlawb-node/src/graphql/mutation.rs index 7fb7a1dce..f97396750 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -73,17 +73,31 @@ impl MutationRoot { let assignee_did = caller.to_string(); let db = ctx.data_unchecked::>(); let tx = ctx.data_unchecked::>(); + crate::api::tasks::get_visible_task(db, &id, Some(caller)) + .await + .map_err(crate::graphql::graphql_app_err)? + .ok_or_else(|| async_graphql::Error::new("task not found"))?; + // Same classification REST's claim handler applies: a claim race is a + // client-safe conflict, a genuine sqlx failure stays opaque. Routing + // both transports through `task_write_conflict` is what stops a normal + // race from reaching a GraphQL caller as a generic database error + // (#327 review). let task = db .claim_task(&id, &assignee_did) .await - .map_err(crate::graphql::graphql_db_err)?; - let _ = tx.send(TaskEventBroadcast { - task_id: id, - old_status: "pending".to_string(), - new_status: "claimed".to_string(), - by_did: assignee_did, - at: Utc::now().to_rfc3339(), - }); + .map_err(crate::graphql::graphql_claim_conflict)?; + crate::api::tasks::announce_task_event( + db, + tx, + TaskEventBroadcast { + task_id: id, + old_status: "pending".to_string(), + new_status: "claimed".to_string(), + by_did: assignee_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(AgentTaskType::from(task)) } @@ -103,12 +117,12 @@ impl MutationRoot { let by_did = caller.to_string(); let db = ctx.data_unchecked::>(); let tx = ctx.data_unchecked::>(); - // Authorize the actor: binding by_did to the signer is necessary but not - // sufficient — only the task's assignee may finish it. - let existing = db - .get_task(&id) + // Authorize the actor: the task must be visible to the caller (returning + // not found for invisible tasks so existence is not leaked), and only + // the task's assignee may finish it. + let existing = crate::api::tasks::get_visible_task(db, &id, Some(caller)) .await - .map_err(crate::graphql::graphql_db_err)? + .map_err(crate::graphql::graphql_app_err)? .ok_or_else(|| async_graphql::Error::new("task not found"))?; if !crate::api::did_matches(caller, existing.assignee_did.as_deref().unwrap_or_default()) { return Err(async_graphql::Error::new( @@ -118,14 +132,19 @@ impl MutationRoot { let task = db .finish_task(&id, "completed", input.result.as_deref()) .await - .map_err(crate::graphql::graphql_db_err)?; - let _ = tx.send(TaskEventBroadcast { - task_id: id, - old_status: "claimed".to_string(), - new_status: "completed".to_string(), - by_did, - at: Utc::now().to_rfc3339(), - }); + .map_err(crate::graphql::graphql_finish_conflict)?; + crate::api::tasks::announce_task_event( + db, + tx, + TaskEventBroadcast { + task_id: id, + old_status: "claimed".to_string(), + new_status: "completed".to_string(), + by_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(AgentTaskType::from(task)) } @@ -145,11 +164,12 @@ impl MutationRoot { let by_did = caller.to_string(); let db = ctx.data_unchecked::>(); let tx = ctx.data_unchecked::>(); - // Authorize the actor: only the task's assignee may fail it. - let existing = db - .get_task(&id) + // Authorize the actor: the task must be visible to the caller (returning + // not found for invisible tasks so existence is not leaked), and only + // the task's assignee may fail it. + let existing = crate::api::tasks::get_visible_task(db, &id, Some(caller)) .await - .map_err(crate::graphql::graphql_db_err)? + .map_err(crate::graphql::graphql_app_err)? .ok_or_else(|| async_graphql::Error::new("task not found"))?; if !crate::api::did_matches(caller, existing.assignee_did.as_deref().unwrap_or_default()) { return Err(async_graphql::Error::new( @@ -160,14 +180,19 @@ impl MutationRoot { let task = db .finish_task(&id, "failed", Some(&reason)) .await - .map_err(crate::graphql::graphql_db_err)?; - let _ = tx.send(TaskEventBroadcast { - task_id: id, - old_status: "claimed".to_string(), - new_status: "failed".to_string(), - by_did, - at: Utc::now().to_rfc3339(), - }); + .map_err(crate::graphql::graphql_finish_conflict)?; + crate::api::tasks::announce_task_event( + db, + tx, + TaskEventBroadcast { + task_id: id, + old_status: "claimed".to_string(), + new_status: "failed".to_string(), + by_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(AgentTaskType::from(task)) } } @@ -218,9 +243,10 @@ mod tests { errors(&resp) ); - // 3. Signed as the claimed assignee → passes the auth gate. The missing - // task is a business error from claim_task, not a sqlx fault, so the - // actionable message must survive (not the opaque DB string) (#250). + // 3. Signed as the claimed assignee → passes the auth gate. A missing + // task is gated by get_visible_task as "task not found" (same as a + // denied id) before claim_task runs, and must stay a business + // message rather than the opaque DB string (#250). let resp = schema .execute(Request::new(&q).data(AuthenticatedDid(assignee.into()))) .await; @@ -230,8 +256,8 @@ mod tests { "matching signer must pass the auth gate: {errs}" ); assert!( - errs.contains("task not claimable"), - "claim race / missing task must keep its business message: {errs}" + errs.contains("task not found"), + "missing task must keep its business message: {errs}" ); assert!( !errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE), @@ -272,6 +298,195 @@ mod tests { ); } + /// #327 review: `claimTask`/`completeTask`/`failTask` called + /// `graphql_db_err` directly on db-layer business failures, so an ordinary + /// claim race or stale finish reached GraphQL clients as a generic + /// database error while REST clients got an actionable conflict. All three + /// now route through the shared `task_write_conflict` classifier. + /// + /// Deleting either `map_err` from a mutation turns this red: the message + /// becomes the raw anyhow text instead of the fixed conflict form. + #[sqlx::test] + async fn task_write_races_surface_as_conflicts_not_db_errors(pool: PgPool) { + let state = crate::test_support::test_state(pool).await; + let schema = state.graphql_schema.as_ref(); + let assignee = "did:key:zGQLRACEASSIGNEEAAAAAAAAAAAAAAAAAAAAAAA"; + let rival = "did:key:zGQLRACERIVALBBBBBBBBBBBBBBBBBBBBBBBBBB"; + let delegator = "did:key:zGQLRACEDELEGATORCCCCCCCCCCCCCCCCCCCCC"; + let now = chrono::Utc::now().to_rfc3339(); + + state + .db + .create_repo(&crate::db::RepoRecord { + id: "race-repo".into(), + name: "race".into(), + owner_did: delegator.into(), + description: None, + is_public: true, + default_branch: "main".into(), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + disk_path: "/tmp/race-repo".into(), + forked_from: None, + machine_id: None, + }) + .await + .unwrap(); + state + .db + .create_task(&crate::db::AgentTask { + id: "race-task".into(), + repo_id: Some("race-repo".into()), + kind: "build".into(), + status: "pending".into(), + delegator_did: delegator.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: None, + payload: None, + result: None, + created_at: now.clone(), + updated_at: now, + deadline: None, + }) + .await + .unwrap(); + + // The assignee wins the claim. + let claim = |did: &str| { + format!(r#"mutation {{ claimTask(id: "race-task", assigneeDid: "{did}") {{ id }} }}"#) + }; + let resp = schema + .execute(Request::new(claim(assignee)).data(AuthenticatedDid(assignee.into()))) + .await; + assert!(resp.errors.is_empty(), "first claim: {}", errors(&resp)); + + // The rival loses the race: an expected conflict, not an outage. + let resp = schema + .execute(Request::new(claim(rival)).data(AuthenticatedDid(rival.into()))) + .await; + let errs = errors(&resp); + assert!( + errs.contains("task not claimable: not found or already claimed"), + "a lost claim race must render the fixed conflict message: {errs}" + ); + assert!( + !errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE), + "a claim race is not a database failure: {errs}" + ); + + // A stale finish on an already-completed task is the same class. + let complete = format!( + r#"mutation {{ completeTask(id: "race-task", byDid: "{assignee}", input: {{ result: "ok" }}) {{ id }} }}"# + ); + let resp = schema + .execute(Request::new(&complete).data(AuthenticatedDid(assignee.into()))) + .await; + assert!(resp.errors.is_empty(), "first complete: {}", errors(&resp)); + + let resp = schema + .execute(Request::new(&complete).data(AuthenticatedDid(assignee.into()))) + .await; + let errs = errors(&resp); + assert!( + errs.contains("task not found or not in claimed state"), + "a stale complete must render the fixed conflict message: {errs}" + ); + assert!( + !errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE), + "a stale complete is not a database failure: {errs}" + ); + + let fail = format!( + r#"mutation {{ failTask(id: "race-task", byDid: "{assignee}", input: {{ reason: "nope" }}) {{ id }} }}"# + ); + let resp = schema + .execute(Request::new(&fail).data(AuthenticatedDid(assignee.into()))) + .await; + let errs = errors(&resp); + assert!( + errs.contains("task not found or not in claimed state"), + "a stale fail must render the fixed conflict message: {errs}" + ); + assert!( + !errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE), + "a stale fail is not a database failure: {errs}" + ); + } + + /// The other half of the same classification: a genuine SQL fault on the + /// write itself must stay opaque. Without this, routing conflicts through + /// `task_write_conflict` could be "fixed" by making every db failure a + /// client-visible conflict message. + #[sqlx::test] + async fn task_write_sql_faults_stay_opaque(pool: PgPool) { + let state = crate::test_support::test_state(pool.clone()).await; + let assignee = "did:key:zGQLOPAQUEASSIGNEEAAAAAAAAAAAAAAAAAAAAA"; + let delegator = "did:key:zGQLOPAQUEDELEGATORBBBBBBBBBBBBBBBBBBB"; + let now = chrono::Utc::now().to_rfc3339(); + state + .db + .create_task(&crate::db::AgentTask { + id: "opaque-task".into(), + repo_id: None, + kind: "build".into(), + status: "pending".into(), + delegator_did: delegator.into(), + assignee_did: Some(assignee.into()), + capability: "repo:write".into(), + ucan_token: None, + payload: None, + result: None, + created_at: now.clone(), + updated_at: now, + deadline: None, + }) + .await + .unwrap(); + + // Fault the write itself, not the schema. Dropping a column the + // visibility pre-check also SELECTs would fail in `get_visible_task` + // and never reach `graphql_claim_conflict`, so this test would pass + // against a claim mapper that leaks (#327 review). A BEFORE UPDATE + // trigger keeps every read valid and faults only inside + // `Db::claim_task`. + sqlx::raw_sql( + "CREATE FUNCTION gl_test_fault_task_update() RETURNS trigger + LANGUAGE plpgsql AS $fn$ + BEGIN RAISE EXCEPTION 'gl_test_forced_update_fault'; END; + $fn$; + CREATE TRIGGER gl_test_fault_task_update + BEFORE UPDATE ON agent_tasks + FOR EACH ROW EXECUTE FUNCTION gl_test_fault_task_update();", + ) + .execute(&pool) + .await + .unwrap(); + + let resp = state + .graphql_schema + .execute( + Request::new(format!( + r#"mutation {{ claimTask(id: "opaque-task", assigneeDid: "{assignee}") {{ id }} }}"# + )) + .data(AuthenticatedDid(assignee.into())), + ) + .await; + let errs = errors(&resp); + assert!( + errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE), + "a real sqlx fault on the write must stay opaque: {errs}" + ); + assert!( + !errs.contains("task not claimable"), + "a write-time sqlx fault must not be reclassified as a claim race: {errs}" + ); + assert!( + !errs.contains("gl_test_forced_update_fault") && !errs.contains("trigger"), + "database text leaked through the conflict mapper: {errs}" + ); + } + /// Adversarial-review GATE-1 (GraphQL): completing a task requires being its /// assignee, not merely signing as the by_did you pass. A signer who is not /// the assignee is rejected even though the by_did binding passes; the @@ -294,7 +509,7 @@ mod tests { payload: None, result: None, created_at: now.clone(), - updated_at: now, + updated_at: now.clone(), deadline: None, }; state.db.create_task(&task).await.expect("seed task"); @@ -311,14 +526,69 @@ mod tests { ) }; - // Stranger signs as themselves and passes byDid=self (so the signer - // binding passes), but is not the assignee → rejected by authorization. + // Stranger signs as themselves on a repo-less task they cannot see: + // invisible task returns "task not found" so existence is not leaked. let resp = schema .execute(Request::new(q(stranger)).data(AuthenticatedDid(stranger.into()))) .await; + assert!( + errors(&resp).contains("task not found"), + "an invisible task must return not found, got: {}", + errors(&resp) + ); + + // Seed a task on a public repo that stranger CAN see, but is not assignee of: + let pub_repo = crate::db::RepoRecord { + id: "pub-r".into(), + name: "pub-r".into(), + owner_did: "did:key:zGQLDELEGATORCCCCCCCCCCCCCCCCCCCCCCCCCC".into(), + description: None, + is_public: true, + default_branch: "main".into(), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + disk_path: "/tmp/pub-r".into(), + forked_from: None, + machine_id: None, + }; + state.db.create_repo(&pub_repo).await.expect("create repo"); + let pub_task = crate::db::AgentTask { + id: "task-pub".into(), + repo_id: Some("pub-r".into()), + kind: "build".into(), + status: "pending".into(), + delegator_did: "did:key:zGQLDELEGATORCCCCCCCCCCCCCCCCCCCCCCCCCC".into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: None, + payload: None, + result: None, + created_at: now.clone(), + updated_at: now, + deadline: None, + }; + state + .db + .create_task(&pub_task) + .await + .expect("seed pub task"); + state + .db + .claim_task("task-pub", assignee) + .await + .expect("claim pub task"); + + let q_pub = |actor: &str| { + format!( + r#"mutation {{ completeTask(id: "task-pub", byDid: "{actor}", input: {{}}) {{ id status }} }}"# + ) + }; + let resp = schema + .execute(Request::new(q_pub(stranger)).data(AuthenticatedDid(stranger.into()))) + .await; assert!( errors(&resp).contains("assignee"), - "a non-assignee signer must be rejected: {}", + "a non-assignee signer on a visible task must be rejected: {}", errors(&resp) ); diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index 84d7540b7..ad866ee7b 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -3,10 +3,27 @@ use std::sync::Arc; use crate::db::Db; -use super::types::{AgentTaskType, RefUpdateType, RepoType}; +use super::types::{AgentTaskReadType, RefUpdateType, RepoType, TaskPageType}; pub struct QueryRoot; +/// Debit the per-IP task-read brake for one task field, mirroring the +/// `rate_limit_by_ip` layer on `task_read_routes`. Both surfaces run the same +/// #268 visibility gate and pay the same queries before answering, so an +/// anonymous prober must not get an unbraked lane by asking over GraphQL +/// (#327 review). +/// +/// A schema built without the brake (unit tests, subscriptions) is not braked, +/// exactly as `rate_limit_by_ip` is a no-op without its extension. +async fn task_read_brake(ctx: &Context<'_>, field: &str) -> Result<()> { + match ctx.data::() { + Ok(brake) if !brake.check(field).await => Err(async_graphql::Error::new( + crate::rate_limit::RATE_LIMIT_MESSAGE, + )), + _ => Ok(()), + } +} + #[Object] impl QueryRoot { async fn repos(&self, ctx: &Context<'_>) -> Result> { @@ -112,29 +129,73 @@ impl QueryRoot { assignee_did: Option, #[graphql( default = 50, - desc = "Max 200; larger requests are clamped to 200 (no error). Negative values clamp to 0." + desc = "Max 200; larger requests are clamped to 200 (no error). Negative values clamp to 0. Use `nextCursor` to read past the clamp." )] limit: i64, - ) -> Result> { + #[graphql( + desc = "Opaque continuation token from a previous page's `nextCursor`. Must be presented with the same `status`/`assigneeDid` filter that issued it." + )] + cursor: Option, + ) -> Result { + use crate::api::task_cursor::{self, TaskCursorKey, TaskFilter}; + + task_read_brake(ctx, "tasks").await?; let db = ctx.data_unchecked::>(); - // Clamp before SQL: a negative LIMIT is a client fault that Postgres - // rejects with 2201W, which would otherwise trip the opaque DB path - // and write an error-level log on every probe (#250 review). - let limit = limit.clamp(0, 200); - let tasks = db - .list_tasks(status.as_deref(), assignee_did.as_deref(), limit) - .await - .map_err(crate::graphql::graphql_db_err)?; - Ok(tasks.into_iter().map(AgentTaskType::from).collect()) + // #268: gate rows via the same collector the REST list route uses (like + // `ref_updates` shares `collect_visible_ref_updates` with its REST feed), + // so the two surfaces cannot drift. The collector clamps `limit` itself, + // including the negative-LIMIT case #250 called out for this resolver. + let caller = ctx + .data::() + .ok() + .map(|d| d.0.as_str()); + let key = ctx.data_unchecked::(); + let filter = TaskFilter { + status: status.as_deref(), + assignee_did: assignee_did.as_deref(), + }; + let resume = cursor + .as_deref() + .map(|token| task_cursor::decode(key, filter, caller, token)) + .transpose() + .map_err(crate::graphql::graphql_app_err)?; + let result = crate::api::tasks::collect_visible_tasks( + db, + status.as_deref(), + assignee_did.as_deref(), + limit, + resume.as_ref(), + caller, + ) + .await + .map_err(crate::graphql::graphql_app_err)?; + Ok(TaskPageType { + next_cursor: result + .next_position + .as_ref() + .map(|pos| task_cursor::encode(key, filter, caller, pos)), + items: result + .tasks + .into_iter() + .map(AgentTaskReadType::from) + .collect(), + has_more: result.has_more, + incomplete: result.incomplete, + }) } - async fn task(&self, ctx: &Context<'_>, id: String) -> Result> { + async fn task(&self, ctx: &Context<'_>, id: String) -> Result> { + task_read_brake(ctx, "task").await?; let db = ctx.data_unchecked::>(); - let t = db - .get_task(&id) + // #268: same gate as the REST get route, via the shared helper. + let caller = ctx + .data::() + .ok() + .map(|d| d.0.as_str()); + let t = crate::api::tasks::get_visible_task(db, &id, caller) .await - .map_err(crate::graphql::graphql_db_err)?; - Ok(t.map(AgentTaskType::from)) + .map_err(crate::graphql::graphql_app_err)?; + Ok(t.map(AgentTaskReadType::from)) } } @@ -153,10 +214,14 @@ mod tests { Arc::new(db) } + fn cursor_key() -> crate::api::task_cursor::TaskCursorKey { + crate::api::task_cursor::TaskCursorKey::derive(&[42u8; 32]) + } + fn schema(db: Arc) -> super::super::GitlawbSchema { let (ref_tx, _) = tokio::sync::broadcast::channel(16); let (task_tx, _) = tokio::sync::broadcast::channel(16); - super::super::build_schema(db, ref_tx, task_tx) + super::super::build_schema(db, ref_tx, task_tx, cursor_key()) } fn repo(id: &str, owner_did: &str, name: &str, is_public: bool) -> RepoRecord { @@ -466,7 +531,7 @@ mod tests { async fn tasks_negative_limit_clamped(pool: PgPool) { let db = db(pool).await; let schema = schema(db); - let resp = anon(&schema, "{ tasks(limit: -1) { id } }").await; + let resp = anon(&schema, "{ tasks(limit: -1) { items { id } } }").await; assert!( resp.errors.is_empty(), "negative limit must clamp, not fail: {:?}", @@ -500,18 +565,349 @@ mod tests { .unwrap(); } let schema = schema(db); - let resp = anon(&schema, "{ tasks(limit: 5000) { id } }").await; + // Queried as the delegator, not anonymously: since #268 the task read + // surface is visibility-gated, and an anonymous caller sees none of + // these repo-less tasks at all. The clamp is what this test pins, so it + // needs a caller who can legitimately see all 201 rows. + let resp = authed(&schema, "{ tasks(limit: 5000) { items { id } } }", OWNER).await; assert_eq!(count_tasks(&resp), 200, "limit above 200 must clamp to 200"); } + /// Seed one repo-less task carrying a `ucan_token`, so a leak on any read + /// surface is visible in the response body. + async fn seed_task(db: &Db, id: &str, delegator: &str) { + let now = Utc::now().to_rfc3339(); + db.create_task(&crate::db::AgentTask { + id: id.into(), + repo_id: None, + kind: "build".into(), + status: "pending".into(), + delegator_did: delegator.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: Some("SECRET-UCAN-TOKEN".into()), + payload: None, + result: None, + created_at: now.clone(), + updated_at: now, + deadline: None, + }) + .await + .unwrap(); + } + + /// #268: the `tasks` resolver must delegate to the gated collector, not + /// query the DB directly. A repo-less task belonging to someone else is + /// invisible to an anonymous caller. `tasks_negative_limit_clamped` cannot + /// catch a resolver that stops calling `collect_visible_tasks` because it + /// seeds no rows — this seeds one, so the gate is load-bearing here. + #[sqlx::test] + async fn tasks_repo_less_task_hidden_from_anon(pool: PgPool) { + let db = db(pool).await; + seed_task(&db, "t1", OWNER).await; + let schema = schema(db); + let resp = anon(&schema, "{ tasks { items { id } } }").await; + assert_eq!( + count_tasks(&resp), + 0, + "anon must not enumerate another party's repo-less task" + ); + assert!( + !format!("{:?}", resp.data).contains("SECRET-UCAN-TOKEN"), + "no ucan token may reach an anonymous caller" + ); + } + + /// #268 sibling for the single-task resolver: an invisible task reads as + /// `null`, indistinguishable from one that does not exist. + #[sqlx::test] + async fn task_by_id_is_null_for_anon(pool: PgPool) { + let db = db(pool).await; + seed_task(&db, "t1", OWNER).await; + let schema = schema(db); + let resp = anon(&schema, r#"{ task(id: "t1") { id } }"#).await; + assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); + let async_graphql::Value::Object(obj) = &resp.data else { + panic!("data not an object: {:?}", resp.data); + }; + assert_eq!( + obj.get("task"), + Some(&async_graphql::Value::Null), + "an invisible task must read as null, got {:?}", + obj.get("task") + ); + } + + /// #268: `ucanToken` is absent from the read type's schema entirely, so the + /// delegator cannot request it either. Asking for it is a validation error, + /// which pins the redaction at the schema level rather than per-resolver. + #[sqlx::test] + async fn task_read_schema_has_no_ucan_token_field(pool: PgPool) { + let db = db(pool).await; + seed_task(&db, "t1", OWNER).await; + let schema = schema(db); + let resp = authed(&schema, r#"{ task(id: "t1") { id ucanToken } }"#, OWNER).await; + assert!( + !resp.errors.is_empty(), + "ucanToken must not exist on the task read type" + ); + } + + #[sqlx::test] + async fn tasks_find_older_visible_row_behind_denied_window(pool: PgPool) { + let db = db(pool).await; + db.create_repo(&repo("public-repo", OWNER, "public", true)) + .await + .unwrap(); + let visible = crate::db::AgentTask { + id: "visible".into(), + repo_id: Some("public-repo".into()), + kind: "build".into(), + status: "pending".into(), + delegator_did: OWNER.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: None, + payload: None, + result: None, + created_at: "2026-01-01T00:00:00Z".into(), + updated_at: "2026-01-01T00:00:00Z".into(), + deadline: None, + }; + db.create_task(&visible).await.unwrap(); + for i in 0..200 { + let mut hidden = visible.clone(); + hidden.id = format!("hidden-{i:03}"); + hidden.repo_id = None; + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + db.create_task(&hidden).await.unwrap(); + } + + let schema = schema(db); + let resp = anon(&schema, "{ tasks(limit: 1) { items { id } incomplete } }").await; + assert_eq!(count_tasks(&resp), 1); + assert!(!task_incomplete(&resp)); + assert!(format!("{:?}", resp.data).contains("visible")); + } + + /// The GraphQL surface must page past a denied window on server-issued + /// cursors alone, exactly as REST does (#327 review). Before this, the + /// only cursor a caller could hold named the last row they saw, so the + /// query stalled at `incomplete: true` forever and never reached + /// `past-ceiling`. + #[sqlx::test] + async fn tasks_page_past_candidate_ceiling_on_server_cursors(pool: PgPool) { + let db = db(pool).await; + db.create_repo(&repo("public-repo", OWNER, "public", true)) + .await + .unwrap(); + let visible_newer = crate::db::AgentTask { + id: "newer-visible".into(), + repo_id: Some("public-repo".into()), + kind: "build".into(), + status: "pending".into(), + delegator_did: OWNER.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: None, + payload: None, + result: None, + created_at: "2026-01-03T00:00:00Z".into(), + updated_at: "2026-01-03T00:00:00Z".into(), + deadline: None, + }; + db.create_task(&visible_newer).await.unwrap(); + for i in 0..1500 { + let mut hidden = visible_newer.clone(); + hidden.id = format!("hidden-{i:04}"); + hidden.repo_id = None; + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + db.create_task(&hidden).await.unwrap(); + } + + let mut visible_older = visible_newer.clone(); + visible_older.id = "past-ceiling".into(); + visible_older.created_at = "2026-01-01T00:00:00Z".into(); + visible_older.updated_at = visible_older.created_at.clone(); + db.create_task(&visible_older).await.unwrap(); + + let schema = schema(db); + let mut seen: Vec = Vec::new(); + let mut cursor: Option = None; + for request in 0..10 { + let query = match &cursor { + Some(c) => format!( + r#"{{ tasks(limit: 1, cursor: "{c}") {{ items {{ id }} hasMore incomplete nextCursor }} }}"# + ), + None => { + "{ tasks(limit: 1) { items { id } hasMore incomplete nextCursor } }".to_string() + } + }; + let resp = anon(&schema, &query).await; + let rendered = format!("{:?}", resp.data); + assert!( + !rendered.contains("hidden-"), + "request {request} disclosed a denied row: {rendered}" + ); + for item in task_items(&resp) { + let async_graphql::Value::Object(row) = item else { + panic!("task item not an object"); + }; + seen.push(row.get("id").unwrap().to_string().replace('"', "")); + } + // Short page with more behind it is the scan wall, and says so. + if task_page_bool(&resp, "hasMore") && task_items(&resp).is_empty() { + assert!( + task_incomplete(&resp), + "request {request}: an empty page with rows behind it is a paused scan" + ); + } + match task_page_cursor(&resp) { + Some(c) => cursor = Some(c), + None => { + assert!(!task_page_bool(&resp, "hasMore")); + assert!(!task_incomplete(&resp)); + break; + } + } + } + + assert_eq!( + seen, + vec!["newer-visible".to_string(), "past-ceiling".to_string()], + "both visible rows must be reachable using only server-issued cursors" + ); + } + + /// REST and GraphQL mint the same tokens from the same node key, so a + /// cursor must not be usable against a filter it was not issued for, and + /// must render the same single rejection every other bad cursor renders. + #[sqlx::test] + async fn tasks_rejects_unusable_cursor(pool: PgPool) { + use crate::api::task_cursor::{self, TaskFilter, TaskPosition}; + + let db = db(pool).await; + let schema = schema(db); + + let wrong_filter = task_cursor::encode( + &cursor_key(), + TaskFilter { + status: Some("pending"), + assignee_did: None, + }, + None, + &TaskPosition::new("2026-01-01T00:00:00Z", "t1"), + ); + let foreign = task_cursor::encode( + &crate::api::task_cursor::TaskCursorKey::derive(&[9u8; 32]), + TaskFilter { + status: None, + assignee_did: None, + }, + None, + &TaskPosition::new("2026-01-01T00:00:00Z", "t1"), + ); + + for (label, query) in [ + ( + "garbage", + r#"{ tasks(cursor: "not-a-cursor") { items { id } } }"#.to_string(), + ), + ( + "another node's key", + format!(r#"{{ tasks(cursor: "{foreign}") {{ items {{ id }} }} }}"#), + ), + ( + "different filter", + format!(r#"{{ tasks(cursor: "{wrong_filter}") {{ items {{ id }} }} }}"#), + ), + ] { + let resp = anon(&schema, &query).await; + assert_eq!( + resp.errors.len(), + 1, + "{label}: must be rejected, not treated as page one" + ); + assert!( + resp.errors[0].message.contains("invalid or expired cursor"), + "{label}: unexpected message {:?}", + resp.errors[0].message + ); + } + + // Presented with the filter that issued it, the same token is accepted. + let resp = anon( + &schema, + &format!( + r#"{{ tasks(status: "pending", cursor: "{wrong_filter}") {{ items {{ id }} }} }}"# + ), + ) + .await; + assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); + } + + fn task_page_bool(resp: &async_graphql::Response, field: &str) -> bool { + assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); + let async_graphql::Value::Object(obj) = &resp.data else { + panic!("data not an object: {:?}", resp.data); + }; + let async_graphql::Value::Object(page) = obj.get("tasks").expect("tasks key") else { + panic!("tasks not an object"); + }; + let async_graphql::Value::Boolean(value) = page.get(field).expect("field present") else { + panic!("{field} not a bool"); + }; + *value + } + + fn task_page_cursor(resp: &async_graphql::Response) -> Option { + assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); + let async_graphql::Value::Object(obj) = &resp.data else { + panic!("data not an object: {:?}", resp.data); + }; + let async_graphql::Value::Object(page) = obj.get("tasks").expect("tasks key") else { + panic!("tasks not an object"); + }; + match page.get("nextCursor").expect("nextCursor key") { + async_graphql::Value::Null => None, + async_graphql::Value::String(c) => Some(c.clone()), + other => panic!("nextCursor not a string: {other:?}"), + } + } + + fn task_items(resp: &async_graphql::Response) -> &Vec { + assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); + let async_graphql::Value::Object(obj) = &resp.data else { + panic!("data not an object: {:?}", resp.data); + }; + let async_graphql::Value::Object(page) = obj.get("tasks").expect("tasks key") else { + panic!("tasks not an object"); + }; + let async_graphql::Value::List(rows) = page.get("items").expect("items key") else { + panic!("items not a list"); + }; + rows + } + fn count_tasks(resp: &async_graphql::Response) -> usize { + task_items(resp).len() + } + + fn task_incomplete(resp: &async_graphql::Response) -> bool { assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); let async_graphql::Value::Object(obj) = &resp.data else { panic!("data not an object: {:?}", resp.data); }; - let async_graphql::Value::List(rows) = obj.get("tasks").expect("tasks key") else { - panic!("tasks not a list"); + let async_graphql::Value::Object(page) = obj.get("tasks").expect("tasks key") else { + panic!("tasks not an object"); }; - rows.len() + let async_graphql::Value::Boolean(incomplete) = + page.get("incomplete").expect("incomplete key") + else { + panic!("incomplete not a bool"); + }; + *incomplete } } diff --git a/crates/gitlawb-node/src/graphql/types.rs b/crates/gitlawb-node/src/graphql/types.rs index 4264a581a..3f96fd6d9 100644 --- a/crates/gitlawb-node/src/graphql/types.rs +++ b/crates/gitlawb-node/src/graphql/types.rs @@ -48,6 +48,65 @@ impl From for AgentTaskType { } } +/// Read-only projection of `AgentTask` for the `tasks`/`task` queries, as +/// opposed to `AgentTaskType`, which the task mutations (`createTask`, +/// `claimTask`, `completeTask`, `failTask` — all `require_signer`-gated) +/// return. Identical except for the missing `ucan_token` (#268): a read +/// surface never needs to echo it back, since the assignee already received it +/// at delegation/claim time via the mutation response. +#[derive(SimpleObject, Clone)] +pub struct AgentTaskReadType { + pub id: String, + pub repo_id: Option, + pub kind: String, + pub status: String, + pub delegator_did: String, + pub assignee_did: Option, + pub capability: String, + pub payload: Option, + pub result: Option, + pub created_at: String, + pub updated_at: String, + pub deadline: Option, +} + +/// Wraps a `tasks` page with the same completion signals the REST list route +/// exposes, so the two surfaces answer pagination identically. +/// +/// `hasMore` and `incomplete` are separate because they are separate facts +/// (#327 review): `hasMore` says more candidates remain, `incomplete` says +/// this page is short *only* because the authorization scan hit its safety +/// wall. `nextCursor` is present exactly when `hasMore` is true and is an +/// opaque MAC'd token — it can name the last examined candidate, denied or +/// not, without disclosing it, which is what lets a caller page past a denied +/// window instead of stalling on it. +#[derive(SimpleObject, Clone)] +pub struct TaskPageType { + pub items: Vec, + pub has_more: bool, + pub incomplete: bool, + pub next_cursor: Option, +} + +impl From for AgentTaskReadType { + fn from(t: AgentTask) -> Self { + Self { + id: t.id, + repo_id: t.repo_id, + kind: t.kind, + status: t.status, + delegator_did: t.delegator_did, + assignee_did: t.assignee_did, + capability: t.capability, + payload: t.payload, + result: t.result, + created_at: t.created_at, + updated_at: t.updated_at, + deadline: t.deadline, + } + } +} + #[derive(SimpleObject, Clone)] pub struct RefUpdateType { pub repo: String, diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 66bfa0961..2596580f6 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -288,10 +288,15 @@ async fn main() -> Result<()> { let (ref_update_tx, _) = tokio::sync::broadcast::channel::(256); let (task_event_tx, _) = tokio::sync::broadcast::channel::(256); + // Node-keyed, so continuation tokens this node mints are only accepted by + // this node and survive its restarts without any configured secret. + let task_cursor_key = api::task_cursor::TaskCursorKey::derive(&keypair.to_seed()); + let graphql_schema = Arc::new(graphql::build_schema( Arc::clone(&db), ref_update_tx.clone(), task_event_tx.clone(), + task_cursor_key.clone(), )); let machine_id = std::env::var("FLY_MACHINE_ID").ok(); @@ -413,6 +418,7 @@ async fn main() -> Result<()> { db, node_did: node_did.clone(), node_keypair: Arc::new(keypair), + task_cursor_key, p2p: p2p_handle, http_client, ref_update_tx, @@ -522,11 +528,19 @@ async fn main() -> Result<()> { std::time::Duration::from_secs(3600), 200_000, ), + task_read_rate_limiter: rate_limit::RateLimiter::new_bounded( + config.task_read_rate_limit, + std::time::Duration::from_secs(3600), + 200_000, + ), git_bin: "git".to_string(), }; if config.ipfs_rate_limit == 0 { tracing::warn!("GITLAWB_IPFS_RATE_LIMIT=0 — per-IP /ipfs rate limiting disabled"); } + if config.task_read_rate_limit == 0 { + tracing::warn!("GITLAWB_TASK_READ_RATE_LIMIT=0 — per-IP task read rate limiting disabled"); + } // Periodic peer-count poll for the metrics gauge. If p2p is disabled // we still set the gauge to 0 so dashboards don't show "no data". @@ -1191,6 +1205,7 @@ mod rate_limiter_sweep_tests { state.peer_write_rate_limiter = RateLimiter::new(10, window); state.ipfs_rate_limiter = RateLimiter::new(10, window); state.ipfs_work_rate_limiter = RateLimiter::new(10, window); + state.task_read_rate_limiter = RateLimiter::new(10, window); let limiters = |s: &crate::state::AppState| { [ @@ -1201,6 +1216,7 @@ mod rate_limiter_sweep_tests { s.peer_write_rate_limiter.clone(), s.ipfs_rate_limiter.clone(), s.ipfs_work_rate_limiter.clone(), + s.task_read_rate_limiter.clone(), ] }; for l in limiters(&state) { diff --git a/crates/gitlawb-node/src/rate_limit.rs b/crates/gitlawb-node/src/rate_limit.rs index c36260898..773815835 100644 --- a/crates/gitlawb-node/src/rate_limit.rs +++ b/crates/gitlawb-node/src/rate_limit.rs @@ -279,7 +279,7 @@ pub async fn rate_limit_by_did(request: Request, next: Next) -> Response { return ( StatusCode::TOO_MANY_REQUESTS, [("retry-after", "60")], - "rate limit exceeded — try again later", + RATE_LIMIT_MESSAGE, ) .into_response(); } @@ -384,6 +384,11 @@ impl axum::extract::FromRequestParts for PeerAddr { } } +/// The message every per-IP brake answers with, shared so the GraphQL surface +/// (which cannot return a 429 status inside a 200 GraphQL envelope) says the +/// same thing as the REST routes. +pub const RATE_LIMIT_MESSAGE: &str = "rate limit exceeded — try again later"; + /// The shared 429 response for the per-IP flood brakes. Route-agnostic: this /// middleware now serves the push path AND the peer-sync routes, so the message /// stays generic (the offending path is recorded in the warn log below). @@ -391,11 +396,60 @@ pub fn too_many_requests() -> Response { ( StatusCode::TOO_MANY_REQUESTS, [("retry-after", "60")], - "rate limit exceeded — try again later", + RATE_LIMIT_MESSAGE, ) .into_response() } +/// The per-IP task-read brake carried as GraphQL request data: the limiter plus +/// the client key the transport already resolved. +/// +/// `/graphql` is a single POST endpoint, so this brake cannot sit in middleware +/// the way `task_read_routes` mounts `rate_limit_by_ip`: that would charge every +/// unrelated query and every mutation against the task-read bucket. The two task +/// resolvers debit it instead, which also prices an aliased query honestly — ten +/// aliased `tasks` fields run the visibility gate ten times and pay ten slots, +/// where one request-scoped debit would pay one (#327 review). +/// Default cap on the number of task read fields (across all aliases) permitted +/// in a single GraphQL request before rejecting further field executions (#327 review). +pub const MAX_GRAPHQL_TASK_READS_PER_REQUEST: usize = 5; + +#[derive(Clone)] +pub struct TaskReadBrake { + pub limiter: RateLimiter, + /// `None` when no key could be resolved at all (no trusted header and no + /// `ConnectInfo`, e.g. a synthetic test request). Never braked, matching + /// [`rate_limit_by_ip`]. + pub key: Option, + pub request_count: Arc, +} + +impl TaskReadBrake { + /// Debit one slot for one task-read field. `true` = proceed. + pub async fn check(&self, field: &str) -> bool { + let Some(key) = self.key.as_deref() else { + return true; + }; + let count = self + .request_count + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + if count >= MAX_GRAPHQL_TASK_READS_PER_REQUEST { + tracing::warn!( + key = %key, + field = %field, + count = count + 1, + "per-request GraphQL task field limit exceeded" + ); + return false; + } + if self.limiter.check(key).await { + return true; + } + tracing::warn!(key = %key, field = %field, "per-IP rate limit exceeded"); + false + } +} + /// Throttle the git push path by resolved client IP. The socket peer address is /// read from `ConnectInfo` (see `into_make_service_with_connect_info` in /// `main`). Only skips the limiter when no key can be resolved at all. diff --git a/crates/gitlawb-node/src/server.rs b/crates/gitlawb-node/src/server.rs index de61fcbe2..df7b0cf99 100644 --- a/crates/gitlawb-node/src/server.rs +++ b/crates/gitlawb-node/src/server.rs @@ -23,6 +23,8 @@ use crate::state::AppState; async fn graphql_handler( State(state): State, auth: Option>, + headers: axum::http::HeaderMap, + rate_limit::PeerAddr(peer): rate_limit::PeerAddr, req: GraphQLRequest, ) -> GraphQLResponse { // `optional_signature` attaches the verified DID when a signature is present. @@ -32,6 +34,18 @@ async fn graphql_handler( if let Some(axum::Extension(did)) = auth { inner = inner.data(did); } + // The anonymous `tasks`/`task` resolvers run the same #268 visibility gate + // as the REST read routes and cost the node the same queries, so they carry + // the same per-IP brake. It rides as request data rather than a router layer + // because /graphql is one endpoint for every operation — see `TaskReadBrake` + // (#327 review). It debits before the gate runs, but unlike the REST layer + // it sits inside `optional_signature`, so it brakes the gate's query cost + // and not signature verification. + inner = inner.data(rate_limit::TaskReadBrake { + limiter: state.task_read_rate_limiter.clone(), + key: rate_limit::client_key(&headers, peer, state.push_limiter_trust), + request_count: std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)), + }); state.graphql_schema.execute(inner).await.into() } @@ -76,10 +90,28 @@ pub fn build_router(state: AppState) -> Router { state.clone(), ); - // ── Task routes (read — open) ────────────────────────────────────────── + // ── Task routes (read — open, but scoped) ────────────────────────────── + // `optional_signature` attaches the verified DID when a signature is present + // so the handlers can identify the caller; the routes stay anonymous-reachable, + // but each task/row is gated to its delegator, its assignee, or (for a + // repo-scoped task) whoever can read that repo (#268 — these routes previously + // carried no gate and no identity at all). + // Both routes also carry a per-IP flood brake, mirroring `/ipfs/{cid}`: they are + // anon-reachable and the gate above costs a task lookup plus deduped-repo and + // visibility-rule queries *before* the opaque 404, so a prober pays nothing and + // the node pays per request. The limiter is the outermost layer so a flood is + // rejected before signature verification and the visibility queries run. The + // extension MUST be attached or `rate_limit_by_ip` is a silent no-op. + let task_read_limiter = rate_limit::IpRateLimiter { + limiter: state.task_read_rate_limiter.clone(), + trust: state.push_limiter_trust, + }; let task_read_routes = Router::new() .route("/api/v1/tasks", get(tasks::list_tasks)) - .route("/api/v1/tasks/{id}", get(tasks::get_task)); + .route("/api/v1/tasks/{id}", get(tasks::get_task)) + .layer(middleware::from_fn(auth::optional_signature)) + .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) + .layer(axum::Extension(task_read_limiter)); // ── Rate-limited creation routes — require HTTP Signature, plus a per-DID // throttle AND a per-IP flood brake. The per-DID limiter (inner) caps a diff --git a/crates/gitlawb-node/src/state.rs b/crates/gitlawb-node/src/state.rs index 24607e5ad..6e84a86e9 100644 --- a/crates/gitlawb-node/src/state.rs +++ b/crates/gitlawb-node/src/state.rs @@ -59,6 +59,10 @@ pub struct AppState { pub task_event_tx: tokio::sync::broadcast::Sender, /// GraphQL schema (queries + mutations + subscriptions) pub graphql_schema: Arc, + /// MAC key for task-list continuation tokens, derived from the node + /// keypair seed. Held here (and handed to the GraphQL schema) so REST and + /// GraphQL mint and accept the same tokens. + pub task_cursor_key: crate::api::task_cursor::TaskCursorKey, /// Fly.io machine ID — used for fly-replay routing in multi-machine deployments pub machine_id: Option, /// Centralized repo storage: local disk cache + optional Tigris backend @@ -319,6 +323,14 @@ pub struct AppState { /// (`with_default_max_keys`, reject-before-insert) so a source-key farm cannot grow /// it (INV-15). pub git_ipfs_walk_per_caller: crate::rate_limit::PerCallerConcurrency, + /// Per-client-IP rate limiter for the task read routes. `GET /api/v1/tasks` + /// and `GET /api/v1/tasks/{id}` are anonymously reachable and run the #268 + /// visibility gate (a task lookup plus deduped-repo and visibility-rule + /// queries) before returning the opaque 404, so an unauthenticated prober + /// costs the node real work per request whether or not anything is visible. + /// Keyed on the resolved client IP via `push_limiter_trust`. Layered on + /// `task_read_routes` via `rate_limit_by_ip`. + pub task_read_rate_limiter: RateLimiter, /// The `git` executable the served-git withheld-blob walk spawns. Production is /// `"git"` (resolved via PATH); injectable so a fake `git` can drive the walk's /// process-group teardown in handler tests without mutating the process-global @@ -346,6 +358,7 @@ impl AppState { self.ipfs_work_rate_limiter.cleanup().await; self.sync_trigger_rate_limiter.cleanup().await; self.peer_write_rate_limiter.cleanup().await; + self.task_read_rate_limiter.cleanup().await; } /// Trigger graceful shutdown. Idempotent — calling more than once diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 430c06002..81458238e 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -83,10 +83,12 @@ fn build_state(db: Arc, pool: PgPool) -> AppState { let node_did = keypair.did(); let (ref_tx, _) = tokio::sync::broadcast::channel(1); let (task_tx, _) = tokio::sync::broadcast::channel(1); + let task_cursor_key = crate::api::task_cursor::TaskCursorKey::derive(&keypair.to_seed()); let schema = Arc::new(graphql::build_schema( db.clone(), ref_tx.clone(), task_tx.clone(), + task_cursor_key.clone(), )); AppState { config: Arc::new(Config::parse_from(["gitlawb-node"])), @@ -98,6 +100,7 @@ fn build_state(db: Arc, pool: PgPool) -> AppState { ref_update_tx: ref_tx, task_event_tx: task_tx, graphql_schema: schema, + task_cursor_key, machine_id: None, repo_store: crate::git::repo_store::RepoStore::for_testing(PathBuf::from("/tmp"), pool), rate_limiter: RateLimiter::new(100, Duration::from_secs(60)), @@ -134,6 +137,7 @@ fn build_state(db: Arc, pool: PgPool) -> AppState { git_ipfs_walk_per_caller: crate::rate_limit::PerCallerConcurrency::with_default_max_keys( 16, ), + task_read_rate_limiter: RateLimiter::new(1200, Duration::from_secs(3600)), git_bin: "git".to_string(), } } @@ -681,11 +685,11 @@ mod tests { let assignee = "did:key:zTASKASSIGNEEBBBBBBBBBBBBBBBBBBBBBBBBBBBB"; let stranger = "did:key:zTASKSTRANGERCCCCCCCCCCCCCCCCCCCCCCCCCCC"; let state = test_state(pool).await; - state - .db - .create_task(&seed_task("task-1", delegator)) - .await - .expect("seed task"); + let repo = seed_repo(delegator, "task-pub-repo"); + state.db.create_repo(&repo).await.expect("seed repo"); + let mut t1 = seed_task("task-1", delegator); + t1.repo_id = Some(repo.id); + state.db.create_task(&t1).await.expect("seed task"); // Assignee claims it: pending -> claimed, assignee_did = assignee. state .db @@ -704,8 +708,25 @@ mod tests { let uri = "/api/v1/tasks/task-1/complete"; let body = || Body::from("{}"); - // Stranger (not the assignee) is rejected by the authorization gate, even - // with the empty body that previously bypassed the binding. Exact 403. + // Stranger on an invisible (repo-less) task receives opaque 404 (no existence leak). + let inv_task = seed_task("task-inv", delegator); + state.db.create_task(&inv_task).await.unwrap(); + let inv_resp = router() + .oneshot(signed_request_as( + stranger, + Method::POST, + "/api/v1/tasks/task-inv/complete", + body(), + )) + .await + .unwrap(); + assert_eq!( + inv_resp.status(), + StatusCode::NOT_FOUND, + "an invisible task must 404 so existence is not leaked" + ); + + // Stranger on a visible task is rejected by the authorization gate with exact 403. let resp = router() .oneshot(signed_request_as(stranger, Method::POST, uri, body())) .await diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index ae319c73a..ffe66a49b 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -512,7 +512,8 @@ fn tool_definitions() -> Value { "properties": { "status": { "type": "string", "description": "Filter by status: pending, claimed, completed, failed" }, "assignee_did": { "type": "string", "description": "Filter by assignee DID" }, - "limit": { "type": "integer", "description": "Max results (default: 50)", "default": 50 } + "limit": { "type": "integer", "minimum": 1, "description": "Total results to return (default: 50). Must be positive; a non-positive limit is rejected rather than answered with an empty list. Values above the node's 200-row page cap are gathered by following continuation tokens.", "default": 50 }, + "cursor": { "type": "string", "description": "Resume from a previous call's next_cursor. Must be paired with the same status/assignee_did filter that produced it." } } } }, @@ -1060,16 +1061,23 @@ async fn call_tool( // ── Task tools ──────────────────────────────────────────────────── "task_list" => { - let limit = args["limit"].as_i64().unwrap_or(50); - let mut path = format!("/api/v1/tasks?limit={limit}"); - if let Some(s) = args.get("status").and_then(|v| v.as_str()) { - path.push_str(&format!("&status={}", urlencoding::encode(s))); - } - if let Some(a) = args.get("assignee_did").and_then(|v| v.as_str()) { - path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); + // Follows the node's opaque `next_cursor` so a limit above the + // 200-row server page cap returns what was asked for instead of a + // silently truncated page, and reports an explicit incomplete + // result when a guard or the node's scan ceiling stops it (#327). + let result = crate::task::fetch_tasks( + &client, + args.get("status").and_then(|v| v.as_str()), + args.get("assignee_did").and_then(|v| v.as_str()), + args["limit"].as_i64().unwrap_or(50), + args.get("cursor").and_then(|v| v.as_str()), + ) + .await?; + let mut out = result.to_json(); + if let Some(warning) = result.truncation_warning() { + out["warning"] = Value::String(warning); } - let resp: Value = client.get(&path).await?.json().await?; - Ok(serde_json::to_string_pretty(&resp)?) + Ok(serde_json::to_string_pretty(&out)?) } "task_create" => { @@ -1085,7 +1093,12 @@ async fn call_tool( "deadline": args.get("deadline").and_then(|v| v.as_str()), "delegator_did": delegator_did, }))?; - let resp: Value = client.post("/api/v1/tasks", &body).await?.json().await?; + let resp: Value = client + .post("/api/v1/tasks", &body) + .await? + .error_for_status()? + .json() + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1097,6 +1110,7 @@ async fn call_tool( let resp: Value = client .post(&format!("/api/v1/tasks/{id}/claim"), &body) .await? + .error_for_status()? .json() .await?; Ok(serde_json::to_string_pretty(&resp)?) @@ -1113,6 +1127,7 @@ async fn call_tool( let resp: Value = client .post(&format!("/api/v1/tasks/{id}/complete"), &body) .await? + .error_for_status()? .json() .await?; Ok(serde_json::to_string_pretty(&resp)?) @@ -1590,7 +1605,7 @@ mod tests { ) .with_status(200) .with_header("content-type", "application/json") - .with_body(r#"{"tasks":[{"id":"t1","kind":"test","status":"pending"}]}"#) + .with_body(r#"{"tasks":[{"id":"t1","kind":"test","status":"pending"}],"has_more":false,"incomplete":false,"next_cursor":null}"#) .create_async() .await; @@ -1604,6 +1619,275 @@ mod tests { .unwrap(); let parsed: Value = serde_json::from_str(&result).unwrap(); assert_eq!(parsed["tasks"][0]["id"], "t1"); + assert_eq!(parsed["complete"], json!(true)); + } + + #[tokio::test] + async fn test_task_list_via_mcp_uses_loaded_identity() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + ) + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"tasks":[{"id":"t1"}],"has_more":false,"incomplete":false,"next_cursor":null}"#, + ) + .create_async() + .await; + + let result = call_tool("task_list", json!({}), &server.url(), Some(dir.path())) + .await + .unwrap(); + let parsed: Value = serde_json::from_str(&result).unwrap(); + assert_eq!(parsed["tasks"][0]["id"], "t1"); + assert_eq!(parsed["complete"], json!(true)); + } + + /// #327 review: MCP `task_list` issued one request and exposed no + /// continuation, so a limit above the node's 200-row page cap returned a + /// silently truncated result the model had no way to detect. It now + /// follows cursors and, when a guard stops it, says so in the payload. + #[tokio::test] + async fn test_task_list_via_mcp_follows_cursors() { + let mut server = mockito::Server::new_async().await; + let first = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=200$".to_string()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"tasks":[{"id":"t1"}],"has_more":true,"incomplete":false,"next_cursor":"c1"}"#, + ) + .create_async() + .await; + let second = server + .mock("GET", mockito::Matcher::Regex(r"cursor=c1".to_string())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"tasks":[{"id":"t2"}],"has_more":false,"next_cursor":null}"#) + .create_async() + .await; + + let result = call_tool("task_list", json!({"limit": 500}), &server.url(), None) + .await + .unwrap(); + first.assert_async().await; + second.assert_async().await; + let parsed: Value = serde_json::from_str(&result).unwrap(); + assert_eq!(parsed["count"], 2); + assert_eq!(parsed["tasks"][1]["id"], "t2"); + assert_eq!(parsed["complete"], json!(true)); + assert!(parsed.get("warning").is_none()); + } + + /// A truncated MCP result must carry an explicit warning and a resume + /// cursor, so the model cannot read it as the whole answer. + #[tokio::test] + async fn test_task_list_via_mcp_flags_incomplete_results() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"tasks":[{"id":"t1"}],"has_more":true,"incomplete":true,"next_cursor":"resume-me"}"#, + ) + .create_async() + .await; + + let result = call_tool("task_list", json!({"limit": 1}), &server.url(), None) + .await + .unwrap(); + let parsed: Value = serde_json::from_str(&result).unwrap(); + assert_eq!(parsed["complete"], json!(false)); + assert_eq!(parsed["incomplete"], json!(true)); + assert_eq!(parsed["next_cursor"], "resume-me"); + assert!( + parsed["warning"] + .as_str() + .unwrap() + .contains("result incomplete"), + "{parsed}" + ); + } + + /// A legacy response without pagination metadata is reported as incomplete via MCP. + #[tokio::test] + async fn test_task_list_via_mcp_legacy_response_is_incomplete() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"tasks":[{"id":"t1"}],"count":1}"#) + .create_async() + .await; + + let result = call_tool("task_list", json!({"limit": 50}), &server.url(), None) + .await + .unwrap(); + let parsed: Value = serde_json::from_str(&result).unwrap(); + assert_eq!(parsed["complete"], json!(false)); + assert_eq!(parsed["incomplete"], json!(true)); + assert!(parsed["next_cursor"].is_null()); + assert!( + parsed["warning"] + .as_str() + .unwrap() + .contains("node does not support pagination metadata"), + "{parsed}" + ); + } + + /// A legacy response with limit > 200 stops after 1 page and does not claim completeness via MCP. + #[tokio::test] + async fn test_task_list_via_mcp_legacy_limit_above_page_cap() { + let mut server = mockito::Server::new_async().await; + let ids: Vec = (0..200).map(|i| format!("t{i}")).collect(); + let tasks_json: Vec = ids.iter().map(|id| json!({ "id": id })).collect(); + let body = json!({ "tasks": tasks_json, "count": 200 }).to_string(); + + let m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=200$".to_string()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(body) + .expect(1) + .create_async() + .await; + + let result = call_tool("task_list", json!({"limit": 500}), &server.url(), None) + .await + .unwrap(); + m.assert_async().await; + let parsed: Value = serde_json::from_str(&result).unwrap(); + assert_eq!(parsed["count"], 200); + assert_eq!(parsed["complete"], json!(false)); + assert_eq!(parsed["incomplete"], json!(true)); + assert!(parsed["next_cursor"].is_null()); + assert!( + parsed["warning"] + .as_str() + .unwrap() + .contains("node does not support pagination metadata"), + "{parsed}" + ); + } + + /// A cursor the model passes back must reach the node. + #[tokio::test] + async fn test_task_list_via_mcp_forwards_cursor_argument() { + let mut server = mockito::Server::new_async().await; + let m = server + .mock( + "GET", + mockito::Matcher::Regex(r"cursor=given-cursor".to_string()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"tasks":[],"has_more":false,"incomplete":false,"next_cursor":null}"#) + .expect(1) + .create_async() + .await; + + call_tool( + "task_list", + json!({"cursor": "given-cursor"}), + &server.url(), + None, + ) + .await + .unwrap(); + m.assert_async().await; + } + + #[tokio::test] + async fn test_task_list_via_mcp_returns_http_errors() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + ) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"error":"failed"}"#) + .create_async() + .await; + + let err = call_tool("task_list", json!({}), &server.url(), None) + .await + .unwrap_err(); + assert!(err.to_string().contains("500")); + } + + /// Malformed server responses fail visibly in MCP. + #[tokio::test] + async fn test_task_list_via_mcp_malformed_response_errors() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"tasks":"not-an-array"}"#) + .create_async() + .await; + + let err = call_tool("task_list", json!({}), &server.url(), None) + .await + .unwrap_err(); + assert!( + err.to_string().contains("malformed") || err.to_string().contains("invalid JSON"), + "{err}" + ); + } + + /// #327 review: a model that sends `limit: 0` used to get an empty list + /// marked complete, which reads as "this node has no tasks". The shared + /// helper rejects it, so the model sees an invalid argument instead. + #[tokio::test] + async fn test_task_list_via_mcp_rejects_non_positive_limit() { + let mut server = mockito::Server::new_async().await; + let m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"tasks":[],"has_more":false,"incomplete":false,"next_cursor":null}"#) + .expect(0) + .create_async() + .await; + + let err = call_tool("task_list", json!({"limit": 0}), &server.url(), None) + .await + .unwrap_err(); + assert!( + err.to_string().contains("limit must be a positive"), + "{err}" + ); + m.assert_async().await; } #[tokio::test] diff --git a/crates/gl/src/task.rs b/crates/gl/src/task.rs index c26cb35f1..6ef3de0f8 100644 --- a/crates/gl/src/task.rs +++ b/crates/gl/src/task.rs @@ -3,6 +3,7 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; use serde_json::{json, Value}; +use std::collections::HashSet; use std::path::PathBuf; use crate::http::NodeClient; @@ -49,16 +50,26 @@ pub enum TaskCmd { status: Option, #[arg(long)] assignee_did: Option, + /// Total tasks to return. Must be positive. Values above the node's + /// 200-row page cap are gathered by following continuation tokens. #[arg(long, default_value = "50")] limit: i64, + /// Resume from a previous run's `next_cursor`. Must be used with the + /// same --status/--assignee-did filter that produced it. + #[arg(long)] + cursor: Option, #[arg(long, default_value = "https://node.gitlawb.com", env = "GITLAWB_NODE")] node: String, + #[arg(long)] + dir: Option, }, /// View a specific task View { id: String, #[arg(long, default_value = "https://node.gitlawb.com", env = "GITLAWB_NODE")] node: String, + #[arg(long)] + dir: Option, }, /// Claim a pending task Claim { @@ -120,9 +131,11 @@ pub async fn run(args: TaskArgs) -> Result<()> { status, assignee_did, limit, + cursor, node, - } => cmd_list(status, assignee_did, limit, node).await, - TaskCmd::View { id, node } => cmd_view(id, node).await, + dir, + } => cmd_list(status, assignee_did, limit, cursor, node, dir).await, + TaskCmd::View { id, node, dir } => cmd_view(id, node, dir).await, TaskCmd::Claim { id, node, dir } => cmd_claim(id, node, dir).await, TaskCmd::Complete { id, @@ -170,6 +183,8 @@ async fn cmd_create( .post("/api/v1/tasks", &body) .await .context("failed to create task")? + .error_for_status() + .context("failed to create task")? .json() .await .context("invalid JSON response")?; @@ -177,37 +192,336 @@ async fn cmd_create( Ok(()) } +/// Server-side ceiling on rows per response (`MAX_VISIBLE_TASKS` on the node). +/// A `--limit` above this needs more than one request, which is why the +/// clients follow `next_cursor` rather than printing a silently truncated page +/// (#327 review). +const SERVER_PAGE_CAP: i64 = 200; + +/// Requests one `gl`/MCP list call may issue while following continuations. +/// The node examines at most 1,000 candidate rows per request, so a long +/// window of tasks the caller cannot read returns empty pages that still carry +/// a cursor. Without this cap a single `task list` against such a window would +/// walk the whole table one request at a time. +const MAX_TASK_PAGES: usize = 25; + +/// Why page-following stopped before the requested limit was met. +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum TaskListStop { + /// The stream ended: every visible task was returned. + Exhausted, + /// The caller's limit was reached; more results remain. + LimitReached, + /// `MAX_TASK_PAGES` requests were issued and more results remain. + PageCap, + /// The node made no progress, returned an invalid/cyclic cursor, or repeated task rows. + NoProgress, + /// The node returned a legacy response without pagination metadata (`has_more`). + LegacyProtocol, +} + +#[derive(Debug)] +pub(crate) struct TaskList { + pub tasks: Vec, + /// True when the last response was short because the node's authorization + /// scan hit its ceiling, not because the stream ended. + pub incomplete: bool, + pub next_cursor: Option, + pub pages: usize, + pub stop: TaskListStop, +} + +impl TaskList { + /// Human-readable warning when the result is not the complete answer to + /// the request, so a truncated list can never read as an exhaustive one. + pub fn truncation_warning(&self) -> Option { + let reason = match self.stop { + TaskListStop::Exhausted | TaskListStop::LimitReached if !self.incomplete => { + return None; + } + TaskListStop::PageCap => "page limit reached", + TaskListStop::NoProgress => { + "node made no progress or returned an invalid/cyclic cursor" + } + TaskListStop::LegacyProtocol => "node does not support pagination metadata", + _ => "node's authorization scan ceiling reached", + }; + let resume = match &self.next_cursor { + Some(c) => format!("; continue with --cursor {c}"), + None => String::new(), + }; + Some(format!( + "result incomplete: {reason} after {} page(s), {} task(s) returned{resume}", + self.pages, + self.tasks.len() + )) + } + + pub fn to_json(&self) -> Value { + json!({ + "tasks": self.tasks, + "count": self.tasks.len(), + "incomplete": self.incomplete, + "has_more": self.next_cursor.is_some(), + "next_cursor": self.next_cursor, + "pages_fetched": self.pages, + "complete": self.truncation_warning().is_none(), + }) + } +} + +#[derive(Debug)] +pub(crate) enum TaskPage { + Paginated { + tasks: Vec, + has_more: bool, + incomplete: bool, + next_cursor: Option, + }, + Legacy { + tasks: Vec, + }, +} + +pub(crate) fn parse_task_page(val: Value) -> Result { + let obj = match val { + Value::Object(map) => map, + _ => anyhow::bail!("malformed task response: expected JSON object"), + }; + + let tasks_val = obj + .get("tasks") + .ok_or_else(|| anyhow::anyhow!("malformed task response: missing 'tasks' field"))?; + let tasks_arr = match tasks_val { + Value::Array(arr) => arr.clone(), + _ => anyhow::bail!("malformed task response: 'tasks' must be an array"), + }; + + let has_more_val = obj.get("has_more"); + let incomplete_val = obj.get("incomplete"); + let next_cursor_val = obj.get("next_cursor"); + + if let Some(hm) = has_more_val { + let has_more = match hm { + Value::Bool(b) => *b, + _ => anyhow::bail!("malformed task response: 'has_more' must be a boolean"), + }; + + let incomplete = match incomplete_val { + Some(Value::Bool(b)) => *b, + Some(_) => anyhow::bail!("malformed task response: 'incomplete' must be a boolean"), + None => false, + }; + + let next_cursor = match next_cursor_val { + Some(Value::String(s)) => { + if s.is_empty() { + None + } else { + Some(s.clone()) + } + } + Some(Value::Null) | None => None, + Some(_) => { + anyhow::bail!("malformed task response: 'next_cursor' must be a string or null") + } + }; + + Ok(TaskPage::Paginated { + tasks: tasks_arr, + has_more, + incomplete, + next_cursor, + }) + } else { + if incomplete_val.is_some() || next_cursor_val.is_some() { + anyhow::bail!("malformed task response: pagination fields present without 'has_more'"); + } + if let Some(count_val) = obj.get("count") { + if !count_val.is_number() { + anyhow::bail!("malformed task response: 'count' must be a number"); + } + } + Ok(TaskPage::Legacy { tasks: tasks_arr }) + } +} + +/// Fetch up to `limit` visible tasks, following the node's opaque +/// `next_cursor` across requests. +/// +/// Bounded on both axes so this cannot become an unbounded crawl: at most +/// `MAX_TASK_PAGES` requests, and it stops the moment a response reports more +/// results without a cursor to reach them. +/// +/// `limit` must be positive. The node clamps a non-positive limit to zero and +/// answers with an empty page marked complete, which reads as "no tasks exist" +/// rather than "your request was invalid", so both clients reject it here +/// instead of sending it (#327 review). +pub(crate) async fn fetch_tasks( + client: &NodeClient, + status: Option<&str>, + assignee_did: Option<&str>, + limit: i64, + cursor: Option<&str>, +) -> Result { + if limit < 1 { + anyhow::bail!("limit must be a positive number of tasks (got {limit})"); + } + let mut tasks: Vec = Vec::new(); + let mut seen_task_ids: HashSet = HashSet::new(); + let mut seen_cursors: HashSet = HashSet::new(); + let mut current_request_cursor: Option = cursor.map(str::to_string); + if let Some(ref c) = current_request_cursor { + seen_cursors.insert(c.clone()); + } + let mut safe_resume_cursor: Option = None; + let mut incomplete = false; + let mut pages = 0usize; + + let stop = loop { + if tasks.len() as i64 >= limit { + break TaskListStop::LimitReached; + } + let want = (limit - tasks.len() as i64).min(SERVER_PAGE_CAP); + let mut path = format!("/api/v1/tasks?limit={want}"); + if let Some(s) = status { + path.push_str(&format!("&status={}", urlencoding::encode(s))); + } + if let Some(a) = assignee_did { + path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); + } + if let Some(c) = ¤t_request_cursor { + path.push_str(&format!("&cursor={}", urlencoding::encode(c))); + } + let raw_val: Value = client + .get_maybe_signed(&path) + .await + .context("failed to list tasks")? + // No `context` here: the reqwest error already names the status, + // and MCP surfaces this message verbatim to the model. + .error_for_status()? + .json() + .await + .context("invalid JSON response")?; + pages += 1; + + let page = parse_task_page(raw_val)?; + match page { + TaskPage::Paginated { + tasks: page_tasks, + has_more, + incomplete: page_incomplete, + next_cursor, + } => { + let mut has_duplicate_row = false; + for t in &page_tasks { + if let Some(id) = t.get("id").and_then(|v| v.as_str()) { + if seen_task_ids.contains(id) { + has_duplicate_row = true; + break; + } + } + } + if has_duplicate_row { + incomplete = true; + safe_resume_cursor = None; + break TaskListStop::NoProgress; + } + + for t in page_tasks { + if let Some(id) = t.get("id").and_then(|v| v.as_str()) { + seen_task_ids.insert(id.to_string()); + } + tasks.push(t); + } + incomplete = page_incomplete; + + if tasks.len() as i64 >= limit { + safe_resume_cursor = if has_more { next_cursor } else { None }; + break TaskListStop::LimitReached; + } + + if !has_more { + safe_resume_cursor = None; + break TaskListStop::Exhausted; + } + + let Some(next) = next_cursor else { + incomplete = true; + safe_resume_cursor = None; + break TaskListStop::NoProgress; + }; + + if seen_cursors.contains(&next) { + incomplete = true; + safe_resume_cursor = None; + break TaskListStop::NoProgress; + } + + seen_cursors.insert(next.clone()); + current_request_cursor = Some(next.clone()); + safe_resume_cursor = Some(next); + + if pages >= MAX_TASK_PAGES { + break TaskListStop::PageCap; + } + } + TaskPage::Legacy { tasks: page_tasks } => { + for t in page_tasks { + if let Some(id) = t.get("id").and_then(|v| v.as_str()) { + seen_task_ids.insert(id.to_string()); + } + tasks.push(t); + } + incomplete = true; + safe_resume_cursor = None; + break TaskListStop::LegacyProtocol; + } + } + }; + + Ok(TaskList { + tasks, + incomplete, + next_cursor: safe_resume_cursor, + pages, + stop, + }) +} + async fn cmd_list( status: Option, assignee_did: Option, limit: i64, + cursor: Option, node: String, + dir: Option, ) -> Result<()> { - let client = NodeClient::new(&node, None); - let mut path = format!("/api/v1/tasks?limit={}", limit); - if let Some(s) = &status { - path.push_str(&format!("&status={}", urlencoding::encode(s))); - } - if let Some(a) = &assignee_did { - path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); + let client = NodeClient::new(&node, load_keypair_from_dir(dir.as_deref()).ok()); + let result = fetch_tasks( + &client, + status.as_deref(), + assignee_did.as_deref(), + limit, + cursor.as_deref(), + ) + .await?; + print_json(&result.to_json()); + // stderr so stdout stays a single parseable JSON document. + if let Some(warning) = result.truncation_warning() { + eprintln!("warning: {warning}"); } - let resp: Value = client - .get(&path) - .await - .context("failed to list tasks")? - .json() - .await - .context("invalid JSON response")?; - print_json(&resp); Ok(()) } -async fn cmd_view(id: String, node: String) -> Result<()> { - let client = NodeClient::new(&node, None); +async fn cmd_view(id: String, node: String, dir: Option) -> Result<()> { + let client = NodeClient::new(&node, load_keypair_from_dir(dir.as_deref()).ok()); let resp: Value = client - .get(&format!("/api/v1/tasks/{}", id)) + .get_maybe_signed(&format!("/api/v1/tasks/{}", id)) .await .context("failed to get task")? + .error_for_status() + .context("failed to get task")? .json() .await .context("invalid JSON response")?; @@ -225,6 +539,8 @@ async fn cmd_claim(id: String, node: String, dir: Option) -> Result<()> .post(&format!("/api/v1/tasks/{}/claim", id), &body) .await .context("failed to claim task")? + .error_for_status() + .context("claim request rejected")? .json() .await .context("invalid JSON response")?; @@ -247,6 +563,8 @@ async fn cmd_complete( .post(&format!("/api/v1/tasks/{}/complete", id), &body) .await .context("failed to complete task")? + .error_for_status() + .context("complete request rejected")? .json() .await .context("invalid JSON response")?; @@ -269,6 +587,8 @@ async fn cmd_fail( .post(&format!("/api/v1/tasks/{}/fail", id), &body) .await .context("failed to fail task")? + .error_for_status() + .context("fail request rejected")? .json() .await .context("invalid JSON response")?; @@ -358,8 +678,7 @@ mod tests { .create_async() .await; - // Should still succeed (prints JSON, doesn't check status code) - cmd_create( + let err = cmd_create( "deploy".to_string(), "agent:task".to_string(), None, @@ -371,7 +690,8 @@ mod tests { Some(dir.path().to_path_buf()), ) .await - .unwrap(); + .unwrap_err(); + assert!(err.to_string().contains("failed to create task")); } // ── list ───────────────────────────────────────────────────────── @@ -379,6 +699,7 @@ mod tests { #[tokio::test] async fn test_list_tasks_empty() { let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); let _m = server .mock( @@ -387,25 +708,43 @@ mod tests { ) .with_status(200) .with_header("content-type", "application/json") - .with_body(r#"{"tasks":[]}"#) + .with_body(r#"{"tasks":[],"has_more":false,"incomplete":false,"next_cursor":null}"#) .create_async() .await; - cmd_list(None, None, 50, server.url()).await.unwrap(); + cmd_list( + None, + None, + 50, + None, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap(); } #[tokio::test] - async fn test_list_tasks_with_filters() { + async fn test_delegator_list_tasks_is_signed() { let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); let _m = server .mock( "GET", mockito::Matcher::Regex(r"status=pending".to_string()), ) + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") - .with_body(r#"{"tasks":[{"id":"t1","kind":"test","status":"pending"}]}"#) + .with_body(r#"{"tasks":[{"id":"t1","kind":"test","status":"pending"}],"has_more":false,"incomplete":false,"next_cursor":null}"#) .create_async() .await; @@ -413,43 +752,642 @@ mod tests { Some("pending".to_string()), Some("did:key:z6Mk_test".to_string()), 10, + None, server.url(), + Some(dir.path().to_path_buf()), ) .await .unwrap(); } + // ── list paging ────────────────────────────────────────────────── + + fn page(ids: &[&str], has_more: bool, incomplete: bool, next: Option<&str>) -> String { + let tasks: Vec = ids + .iter() + .map(|id| json!({ "id": id, "kind": "test", "status": "pending" })) + .collect(); + json!({ + "tasks": tasks, + "count": tasks.len(), + "has_more": has_more, + "incomplete": incomplete, + "next_cursor": next, + }) + .to_string() + } + + fn client_for(server: &mockito::Server) -> NodeClient { + NodeClient::new(server.url(), None) + } + + /// #327 review: `--limit 500` used to print a successful but silently + /// truncated 200-row page, because the client issued exactly one request + /// and the server clamps to 200. It now follows `next_cursor`. + #[tokio::test] + async fn list_follows_cursors_until_the_limit_is_met() { + let mut server = mockito::Server::new_async().await; + let first = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=200$".into()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["a", "b"], true, false, Some("cursor-1"))) + .create_async() + .await; + let second = server + .mock("GET", mockito::Matcher::Regex(r"cursor=cursor-1".into())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["c"], false, false, None)) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 300, None) + .await + .unwrap(); + first.assert_async().await; + second.assert_async().await; + assert_eq!(result.tasks.len(), 3); + assert_eq!(result.pages, 2); + assert_eq!(result.stop, TaskListStop::Exhausted); + assert!(result.next_cursor.is_none()); + assert!( + result.truncation_warning().is_none(), + "an exhausted stream is a complete result" + ); + assert_eq!(result.to_json()["complete"], json!(true)); + } + + /// The node's authorization scan ceiling can end a run early. That must + /// reach the user as an explicit incomplete result with a way to resume, + /// never as a plain short list. + #[tokio::test] + async fn list_reports_incomplete_and_offers_a_resume_cursor() { + let mut server = mockito::Server::new_async().await; + // Distinct cursor per page, so the page cap is what stops this and not + // the no-progress guard. + let mut mocks = Vec::new(); + for step in 0..=MAX_TASK_PAGES { + let matcher = if step == 0 { + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=200$".into()) + } else { + mockito::Matcher::Regex(format!(r"cursor=step-{step}$")) + }; + let task_id = format!("task-{step}"); + mocks.push( + server + .mock("GET", matcher) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page( + &[&task_id], + true, + true, + Some(&format!("step-{}", step + 1)), + )) + .create_async() + .await, + ); + } + + let result = fetch_tasks(&client_for(&server), None, None, 10_000, None) + .await + .unwrap(); + assert_eq!(result.stop, TaskListStop::PageCap); + assert_eq!(result.pages, MAX_TASK_PAGES); + assert_eq!(result.tasks.len(), MAX_TASK_PAGES); + assert!(result.incomplete); + assert_eq!( + result.next_cursor.as_deref(), + Some(format!("step-{MAX_TASK_PAGES}").as_str()) + ); + let warning = result.truncation_warning().expect("must warn"); + assert!(warning.contains("page limit reached"), "{warning}"); + assert!( + warning.contains(&format!("--cursor step-{MAX_TASK_PAGES}")), + "{warning}" + ); + assert_eq!(result.to_json()["complete"], json!(false)); + } + + /// A node that claims more results but hands back no cursor would spin the + /// loop forever on the same request. The progress guard stops it. + #[tokio::test] + async fn list_stops_when_the_node_offers_no_way_forward() { + let mut server = mockito::Server::new_async().await; + let m = server + .mock("GET", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["a"], true, false, None)) + .expect(1) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 500, None) + .await + .unwrap(); + m.assert_async().await; + assert_eq!(result.stop, TaskListStop::NoProgress); + assert_eq!(result.pages, 1); + assert!(result.incomplete); + assert!(result.next_cursor.is_none()); + assert_eq!(result.to_json()["complete"], json!(false)); + let warning = result.truncation_warning().expect("must warn"); + assert!(warning.contains("result incomplete"), "{warning}"); + } + + /// A node that keeps returning the same cursor is the other shape of the + /// same fault, and must not loop either. + #[tokio::test] + async fn list_stops_when_the_cursor_does_not_advance() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["a"], true, false, Some("stuck"))) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 500, Some("stuck")) + .await + .unwrap(); + assert_eq!(result.stop, TaskListStop::NoProgress); + assert_eq!(result.pages, 1); + assert!(result.incomplete); + assert!(result.next_cursor.is_none()); + assert_eq!(result.to_json()["complete"], json!(false)); + } + + /// A legacy node returns `{tasks, count}` without pagination metadata. + /// The client must NOT interpret this as complete/exhausted, but instead + /// return an explicit incomplete result with `complete: false`. + #[tokio::test] + async fn list_legacy_response_is_incomplete_and_does_not_claim_exhaustion() { + let mut server = mockito::Server::new_async().await; + let m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=50$".into()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"tasks":[{"id":"t1","kind":"test","status":"pending"}],"count":1}"#) + .expect(1) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 50, None) + .await + .unwrap(); + m.assert_async().await; + assert_eq!(result.stop, TaskListStop::LegacyProtocol); + assert_eq!(result.pages, 1); + assert_eq!(result.tasks.len(), 1); + assert!(result.incomplete); + assert!(result.next_cursor.is_none()); + assert_eq!(result.to_json()["complete"], json!(false)); + let warning = result + .truncation_warning() + .expect("legacy response must warn"); + assert!( + warning.contains("node does not support pagination metadata"), + "{warning}" + ); + } + + /// When querying a legacy node with a limit above the 200-row page cap, + /// the client stops after the first page (since no cursor is returned) + /// and reports `complete: false` rather than claiming the 200 rows are the whole dataset. + #[tokio::test] + async fn list_legacy_response_with_limit_above_page_cap_stops_after_one_page() { + let mut server = mockito::Server::new_async().await; + let ids: Vec = (0..200).map(|i| format!("t{i}")).collect(); + let tasks_json: Vec = ids + .iter() + .map(|id| json!({ "id": id, "kind": "test" })) + .collect(); + let body = json!({ "tasks": tasks_json, "count": 200 }).to_string(); + + let m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=200$".into()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(body) + .expect(1) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 500, None) + .await + .unwrap(); + m.assert_async().await; + assert_eq!(result.stop, TaskListStop::LegacyProtocol); + assert_eq!(result.pages, 1); + assert_eq!(result.tasks.len(), 200); + assert!(result.incomplete); + assert!(result.next_cursor.is_none()); + assert_eq!(result.to_json()["complete"], json!(false)); + let warning = result + .truncation_warning() + .expect("legacy response must warn"); + assert!( + warning.contains("node does not support pagination metadata"), + "{warning}" + ); + } + + /// Malformed responses (missing fields, wrong types) must fail with an error + /// rather than silently succeeding. + #[tokio::test] + async fn list_malformed_responses_fail_visibly() { + let mut server = mockito::Server::new_async().await; + let bad_responses = [ + r#"{"tasks":"not-an-array"}"#, + r#"{"count":0}"#, + r#"{"tasks":[],"has_more":"true"}"#, + r#"{"tasks":[],"has_more":true,"incomplete":"no"}"#, + r#"{"tasks":[],"has_more":true,"next_cursor":123}"#, + r#"{"tasks":[],"next_cursor":"c1"}"#, + r#"[]"#, + ]; + + for bad in bad_responses { + let m = server + .mock("GET", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(bad) + .expect(1) + .create_async() + .await; + + let err = fetch_tasks(&client_for(&server), None, None, 50, None) + .await + .expect_err(&format!("expected error for malformed body: {bad}")); + assert!( + err.to_string().contains("malformed") || err.to_string().contains("invalid JSON"), + "{err}" + ); + m.assert_async().await; + } + } + + /// A node that loops cursors (c1 -> c2 -> c1) must be detected as a cycle, + /// terminating the loop without reaching the limit or page cap, and never + /// reporting duplicate rows as complete or recommending a stale cursor. + #[tokio::test] + async fn list_stops_on_cursor_cycle_c1_c2_c1() { + let mut server = mockito::Server::new_async().await; + let p1 = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=200$".into()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t1"], true, false, Some("c1"))) + .create_async() + .await; + let p2 = server + .mock("GET", mockito::Matcher::Regex(r"cursor=c1".into())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t2"], true, false, Some("c2"))) + .create_async() + .await; + let p3 = server + .mock("GET", mockito::Matcher::Regex(r"cursor=c2".into())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t3"], true, false, Some("c1"))) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 500, None) + .await + .unwrap(); + p1.assert_async().await; + p2.assert_async().await; + p3.assert_async().await; + assert_eq!(result.stop, TaskListStop::NoProgress); + assert_eq!(result.pages, 3); + assert_eq!(result.tasks.len(), 3); + assert!(result.incomplete); + assert!(result.next_cursor.is_none()); + assert_eq!(result.to_json()["complete"], json!(false)); + let warning = result.truncation_warning().expect("must warn on cycle"); + assert!( + !warning.contains("--cursor c1"), + "must not recommend stale cursor c1: {warning}" + ); + assert!( + !warning.contains("--cursor c2"), + "must not recommend stale cursor c2: {warning}" + ); + } + + /// A longer cursor cycle (c1 -> c2 -> c3 -> c1) must terminate boundedly. + #[tokio::test] + async fn list_stops_on_longer_cursor_cycle() { + let mut server = mockito::Server::new_async().await; + let p1 = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=200$".into()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t1"], true, false, Some("c1"))) + .create_async() + .await; + let p2 = server + .mock("GET", mockito::Matcher::Regex(r"cursor=c1".into())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t2"], true, false, Some("c2"))) + .create_async() + .await; + let p3 = server + .mock("GET", mockito::Matcher::Regex(r"cursor=c2".into())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t3"], true, false, Some("c3"))) + .create_async() + .await; + let p4 = server + .mock("GET", mockito::Matcher::Regex(r"cursor=c3".into())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t4"], true, false, Some("c1"))) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 500, None) + .await + .unwrap(); + p1.assert_async().await; + p2.assert_async().await; + p3.assert_async().await; + p4.assert_async().await; + assert_eq!(result.stop, TaskListStop::NoProgress); + assert_eq!(result.pages, 4); + assert!(result.incomplete); + assert!(result.next_cursor.is_none()); + assert_eq!(result.to_json()["complete"], json!(false)); + } + + /// A node that issues fresh cursors (c1 -> c2) but returns repeated task rows + /// must be caught by row progress validation, preventing duplicate rows and stopping boundedly. + #[tokio::test] + async fn list_stops_on_fresh_cursors_with_repeated_task_rows() { + let mut server = mockito::Server::new_async().await; + let p1 = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=200$".into()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t1"], true, false, Some("c1"))) + .create_async() + .await; + let p2 = server + .mock("GET", mockito::Matcher::Regex(r"cursor=c1".into())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t1"], true, false, Some("c2"))) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 500, None) + .await + .unwrap(); + p1.assert_async().await; + p2.assert_async().await; + assert_eq!(result.stop, TaskListStop::NoProgress); + assert_eq!(result.pages, 2); + assert_eq!(result.tasks.len(), 1, "duplicate row must not be appended"); + assert!(result.incomplete); + assert!(result.next_cursor.is_none()); + assert_eq!(result.to_json()["complete"], json!(false)); + let warning = result.truncation_warning().expect("must warn"); + assert!( + !warning.contains("--cursor c2"), + "must not recommend c2: {warning}" + ); + } + + /// If the caller supplies cursor `c1` and the node returns `next_cursor: c1` on the first request, + /// the client stops immediately as NoProgress and does NOT recommend `--cursor c1`. + #[tokio::test] + async fn list_stops_on_immediate_repeat_from_caller_supplied_cursor() { + let mut server = mockito::Server::new_async().await; + let m = server + .mock("GET", mockito::Matcher::Regex(r"cursor=c1".into())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["t1"], true, false, Some("c1"))) + .expect(1) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 500, Some("c1")) + .await + .unwrap(); + m.assert_async().await; + assert_eq!(result.stop, TaskListStop::NoProgress); + assert_eq!(result.pages, 1); + assert_eq!(result.tasks.len(), 1); + assert!(result.incomplete); + assert!( + result.next_cursor.is_none(), + "must not return stale input cursor c1" + ); + assert_eq!(result.to_json()["complete"], json!(false)); + let warning = result.truncation_warning().expect("must warn"); + assert!( + !warning.contains("--cursor c1"), + "must not recommend stale cursor c1: {warning}" + ); + } + + /// A caller-supplied resume cursor must reach the node, and each request + /// must ask only for the rows still outstanding. + #[tokio::test] + async fn list_resumes_from_a_supplied_cursor_and_narrows_each_request() { + let mut server = mockito::Server::new_async().await; + let first = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=3&cursor=given$".into()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["a"], true, false, Some("next"))) + .create_async() + .await; + let second = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/tasks\?limit=2&cursor=next$".into()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["b"], false, false, None)) + .create_async() + .await; + + let result = fetch_tasks(&client_for(&server), None, None, 3, Some("given")) + .await + .unwrap(); + first.assert_async().await; + second.assert_async().await; + assert_eq!(result.tasks.len(), 2); + } + + /// A per-request ask must never exceed the node's page cap, so the client + /// cannot rely on a server that forgets to clamp. + #[tokio::test] + async fn list_never_asks_for_more_than_the_server_page_cap() { + let mut server = mockito::Server::new_async().await; + let m = server + .mock( + "GET", + mockito::Matcher::Regex(format!(r"^/api/v1/tasks\?limit={SERVER_PAGE_CAP}$")), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&["a"], false, false, None)) + .expect(1) + .create_async() + .await; + + fetch_tasks(&client_for(&server), None, None, 5_000, None) + .await + .unwrap(); + m.assert_async().await; + } + + /// #327 review: `--limit 0` (or a negative one) reached the node, which + /// clamped it to zero and answered with an empty page marked complete. A + /// caller could read an invalid request as proof that no tasks exist, so + /// the shared helper both clients use rejects it before the first request. + #[tokio::test] + async fn non_positive_limit_is_rejected_without_a_request() { + let mut server = mockito::Server::new_async().await; + let m = server + .mock("GET", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(page(&[], false, false, None)) + .expect(0) + .create_async() + .await; + + for limit in [0, -1] { + let err = fetch_tasks(&client_for(&server), None, None, limit, None) + .await + .expect_err("a non-positive limit must not be answered with an empty list"); + assert!( + err.to_string().contains("limit must be a positive"), + "{err}" + ); + } + m.assert_async().await; + } + // ── view ───────────────────────────────────────────────────────── #[tokio::test] - async fn test_view_task_success() { + async fn test_assignee_view_task_is_signed() { let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); let _m = server .mock("GET", "/api/v1/tasks/task-42") + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"id":"task-42","kind":"deploy","status":"completed","result":"ok"}"#) .create_async() .await; - cmd_view("task-42".to_string(), server.url()).await.unwrap(); + cmd_view( + "task-42".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap(); + } + + #[tokio::test] + async fn test_private_repo_task_view_is_signed() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + let _m = server + .mock("GET", "/api/v1/tasks/private-task") + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"id":"private-task","repo_id":"private-repo"}"#) + .create_async() + .await; + + cmd_view( + "private-task".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap(); } #[tokio::test] async fn test_view_task_not_found() { let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); let _m = server .mock("GET", "/api/v1/tasks/nope") + .match_header("signature", mockito::Matcher::Missing) .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) .create_async() .await; - // cmd_view doesn't check status — it prints the JSON - cmd_view("nope".to_string(), server.url()).await.unwrap(); + let err = cmd_view( + "nope".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("failed to get task")); } // ── claim ────────────────────────────────────────────────────────