From 4bc9b33c7ab7d6d6ed5222fee4f363489b13925f Mon Sep 17 00:00:00 2001 From: Pedram Jahangiry Date: Thu, 6 Aug 2026 14:57:37 -0600 Subject: [PATCH] fix(buzz-cli): stop aged projects becoming permanently unmodifiable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `next_timestamp` advanced `created_at` off the observed head only (`head.created_at + 1`) and never consulted the wall clock. The signed timestamp therefore stayed frozen near the project's creation and drifted further from real time as the project aged. Once it fell outside the relay's accepted timestamp window, every mutation was rejected with relay error 400: invalid: event timestamp too far from server time and the rejection was permanent rather than transient: the stored head only ages further, so `update`, `add-repo`, `remove-repo` and `delete` were all refused alike. An affected project could no longer be modified *or* removed by anyone, including its author. Advance to `max(now, head.created_at + 1)` instead. The result is still strictly greater than the observed head — including when the head sits in the future, which the existing ordering test covers — while tracking the wall clock in the ordinary case. Trade-off, recorded in the doc comment: `head + 1` also gave free lost-update detection, since a writer working from a stale read landed at or below an intervening writer and was refused as dominated. Tracking wall clock removes that backstop. Closing it properly needs a compare-and-swap on the observed head rather than a timestamp rule. Also add `buzz repos delete --id `, which had no equivalent at all — a kind:30617 announcement could be replaced but never retracted. It fetches the caller's own head, emits a NIP-09 kind:5 addressable tombstone for `30617::`, and re-queries to confirm no head survived, mirroring `buzz projects delete`. Scoped to signer-self, so another author's announcement at the same `d` tag is never touched. Both delete commands now include the tombstone `event_id` in their output, per the documented CLI contract that all writes return `{event_id, accepted, message}`. Note for reviewers: `repos bind` and `repos protect` still carry the same `+1` timestamp pattern in `build_updated_repo_announcement`. Left untouched here to keep this change reviewable; happy to follow up. Co-authored-by: Fizz (Buzz agent) Signed-off-by: Pedram Jahangiry --- crates/buzz-cli/src/commands/projects.rs | 132 +++++++++++++++--- crates/buzz-cli/src/commands/repos.rs | 167 ++++++++++++++++++++++- crates/buzz-cli/src/lib.rs | 17 ++- 3 files changed, 295 insertions(+), 21 deletions(-) diff --git a/crates/buzz-cli/src/commands/projects.rs b/crates/buzz-cli/src/commands/projects.rs index e6798dbfc4..91f72c6018 100644 --- a/crates/buzz-cli/src/commands/projects.rs +++ b/crates/buzz-cli/src/commands/projects.rs @@ -4,8 +4,9 @@ //! 1. Fetch the caller's own live head via `kinds:[30621] + authors:[self] + #d:[slug]`. //! 2. Mutate the tag set (strip `auth`, apply change). //! 3. Re-validate the full envelope through Layer A before submitting. -//! 4. Set `created_at = head.created_at + 1` (never wall-clock) to avoid -//! overwriting a concurrently advancing head. +//! 4. Set `created_at = max(now, head.created_at + 1)` to avoid overwriting +//! a concurrently advancing head without stranding the write outside the +//! relay's accepted timestamp window (see `next_timestamp`). //! //! Limitations recorded in this phase: //! - Relay hints are read-preserved but not authored (`--repo` carries @@ -121,12 +122,40 @@ async fn submit_project(client: &BuzzClient, builder: EventBuilder) -> Result<() // ── Build helpers ───────────────────────────────────────────────────────────── /// Advance the `created_at` counter off an observed head. +/// +/// Returns `max(now, head.created_at + 1)`. +/// +/// `head + 1` on its own preserves monotonicity — the write always lands +/// strictly above the head it read, so it cannot silently erase a +/// concurrently advancing writer. But it never tracks wall clock, so on a +/// head older than the relay's accepted timestamp window it signs a stale +/// `created_at` and the relay rejects the write with "event timestamp too far +/// from server time". That failure is permanent rather than transient: the +/// stored head only ages further, so every subsequent mutation — `update`, +/// `add-repo`, `remove-repo` and `delete` alike — is rejected too, leaving the +/// project impossible to modify or remove. +/// +/// Taking the max keeps the monotonicity guarantee (the result is always +/// greater than the head, including when the head sits in the future) while +/// following wall clock in the ordinary case where wall clock is ahead. +/// +/// Trade-off, deliberately accepted: `head + 1` also gave free lost-update +/// detection. A writer working from a stale read would land at or below an +/// intervening writer's timestamp and be refused as dominated. Tracking wall +/// clock removes that backstop — a delayed writer now lands above the +/// intervening write and silently replaces it. Closing that hole properly +/// needs a compare-and-swap on the observed head, not a timestamp rule. Until +/// then this is the better failure mode: the old behaviour made aged projects +/// permanently unmodifiable and undeletable, which is a certainty, against a +/// narrow concurrent-writer race, which is not. fn next_timestamp(head: &Event) -> Result { - head.created_at + let after_head = head + .created_at .as_secs() .checked_add(1) - .map(Timestamp::from) - .ok_or_else(|| CliError::Other("project timestamp cannot be advanced".into())) + .ok_or_else(|| CliError::Other("project timestamp cannot be advanced".into()))?; + + Ok(Timestamp::from(after_head.max(Timestamp::now().as_secs()))) } /// Strip `auth` from a tag list and pass the resulting envelope through @@ -491,7 +520,7 @@ pub async fn cmd_update( /// /// Head-based and verified: /// 1. Fetch own live head — `NotFound` if absent. -/// 2. Build tombstone at `head.created_at + 1`. +/// 2. Build tombstone at `max(now, head.created_at + 1)`. /// 3. Submit. /// 4. Re-query the coordinate; if a newer head survived → `Conflict`. pub async fn cmd_delete(client: &BuzzClient, slug: &str) -> Result<(), CliError> { @@ -508,6 +537,9 @@ pub async fn cmd_delete(client: &BuzzClient, slug: &str) -> Result<(), CliError> .custom_created_at(next_ts); let event = client.sign_event(tombstone)?; + // Captured before submit: `submit_event` consumes the event, and the + // tombstone id is what an operator needs to audit the retraction. + let tombstone_id = event.id.to_hex(); let raw = client.submit_event(event).await?; parse_write_response(&raw, "delete event was dominated; a newer head exists")?; @@ -520,7 +552,14 @@ pub async fn cmd_delete(client: &BuzzClient, slug: &str) -> Result<(), CliError> ))); } - println!("{}", serde_json::json!({ "deleted": slug, "status": "ok" })); + println!( + "{}", + serde_json::json!({ + "deleted": slug, + "event_id": tombstone_id, + "status": "ok", + }) + ); Ok(()) } @@ -979,22 +1018,27 @@ mod tests { // ── next_timestamp ordering ─────────────────────────────────────────────── - /// `next_timestamp` must return `head.created_at + 1` regardless of the wall - /// clock. NIP-MP Deletion rule: a tombstone older than the live head does - /// NOT remove it, so we must advance strictly off the observed head — never - /// use wall-clock time, which could be behind a head that was bumped - /// multiple times in the same second. - #[test] - fn next_timestamp_returns_head_plus_one_when_head_is_ahead_of_wall_clock() { - // Build a minimal signed event with a created_at far in the future. + /// Build a signed kind:30621 head carrying an exact `created_at`. + fn signed_head_at(created_at: Timestamp) -> Event { let keys = nostr::Keys::generate(); - let far_future_ts = Timestamp::from(9_999_999_999u64); // year 2286 let tags = vec![ make_test_tag(&["d", "platform"]), make_test_tag(&["a", &format!("30617:{OWNER_HEX}:buzz")]), ]; - let builder = rebuild_project("", tags, far_future_ts).expect("valid head envelope"); - let head = builder.sign_with_keys(&keys).expect("sign"); + let builder = rebuild_project("", tags, created_at).expect("valid head envelope"); + builder.sign_with_keys(&keys).expect("sign") + } + + /// When the head is ahead of the wall clock, `next_timestamp` must return + /// `head.created_at + 1`. NIP-MP Deletion rule: a tombstone older than the + /// live head does NOT remove it, so the result must advance strictly off the + /// observed head — wall-clock time alone could land behind a head that was + /// bumped multiple times in the same second. + #[test] + fn next_timestamp_returns_head_plus_one_when_head_is_ahead_of_wall_clock() { + // Build a minimal signed event with a created_at far in the future. + let far_future_ts = Timestamp::from(9_999_999_999u64); // year 2286 + let head = signed_head_at(far_future_ts); // Verify the event actually has our future timestamp. assert_eq!(head.created_at, far_future_ts); @@ -1007,6 +1051,58 @@ mod tests { ); } + /// An aged head must NOT produce an aged timestamp. `head + 1` alone would + /// sign a `created_at` an hour in the past here, which the relay rejects as + /// "event timestamp too far from server time" — permanently, because the + /// stored head only ages further. The result must track the wall clock. + #[test] + fn next_timestamp_uses_wall_clock_when_head_is_aged() { + let now = Timestamp::now().as_secs(); + let hour_ago = Timestamp::from(now - 3_600); + let head = signed_head_at(hour_ago); + + let next = next_timestamp(&head).expect("no overflow").as_secs(); + + assert!( + next >= now, + "aged head must advance to wall clock, got {next} against now {now}" + ); + assert!( + next > hour_ago.as_secs(), + "result must still be strictly after the observed head" + ); + } + + /// A head written moments ago is already inside the relay's window, so the + /// result must stay at wall clock rather than jumping ahead of it. + #[test] + fn next_timestamp_stays_at_wall_clock_for_a_current_head() { + let now = Timestamp::now().as_secs(); + let head = signed_head_at(Timestamp::from(now - 1)); + + let next = next_timestamp(&head).expect("no overflow").as_secs(); + + assert!( + next >= now && next <= now + 1, + "current head must land at wall clock, got {next} against now {now}" + ); + } + + /// `u64::MAX` cannot be advanced — the checked add must surface an error + /// rather than wrapping to zero and signing a 1970 timestamp. + #[test] + fn next_timestamp_rejects_overflow_at_u64_max() { + let head = signed_head_at(Timestamp::from(u64::MAX)); + + let error = next_timestamp(&head).expect_err("u64::MAX must not advance"); + + assert!( + matches!(error, CliError::Other(ref message) + if message.contains("timestamp cannot be advanced")), + "expected a timestamp-advance error, got {error:?}" + ); + } + // ── empty update guard ──────────────────────────────────────────────────── /// `cmd_update` with no setters or clearers must return `CliError::Usage` diff --git a/crates/buzz-cli/src/commands/repos.rs b/crates/buzz-cli/src/commands/repos.rs index e54b95ef20..1a261970ae 100644 --- a/crates/buzz-cli/src/commands/repos.rs +++ b/crates/buzz-cli/src/commands/repos.rs @@ -30,6 +30,22 @@ async fn fetch_own_repo_announcement( Ok(events.into_iter().next()) } +/// Advance the `created_at` counter off an observed announcement head. +/// +/// Returns `max(now, head.created_at + 1)` — strictly after the head, so the +/// write cannot be dominated by the announcement it read, but never stale +/// enough for the relay to reject it as "event timestamp too far from server +/// time" once the head has aged past the accepted window. +fn next_announcement_timestamp(head: &Event) -> Result { + let after_head = head + .created_at + .as_secs() + .checked_add(1) + .ok_or_else(|| CliError::Other("repository timestamp cannot be advanced".into()))?; + + Ok(Timestamp::from(after_head.max(Timestamp::now().as_secs()))) +} + fn repo_id_from_event(event: &Event) -> Result<&str, CliError> { event .tags @@ -270,6 +286,53 @@ pub async fn cmd_create_repo( Ok(()) } +/// `buzz repos delete` +/// +/// Head-based and verified, mirroring `buzz projects delete`: +/// 1. Fetch the caller's own live announcement head — `NotFound` if absent. +/// 2. Build a NIP-09 kind:5 addressable tombstone for +/// `30617::` at `max(now, head.created_at + 1)`. +/// 3. Submit. +/// 4. Re-query the coordinate; if a head survived → `Conflict`. +/// +/// Targets signer-self only: the filter is scoped to the caller's pubkey, so +/// another author's announcement at the same `d` tag is never touched. +pub async fn cmd_delete_repo(client: &BuzzClient, repo_id: &str) -> Result<(), CliError> { + let head = current_repo(client, repo_id).await?; + let next_ts = next_announcement_timestamp(&head)?; + + let pubkey_hex = client.keys().public_key().to_hex(); + let tombstone = + buzz_sdk::build_delete_addressable(KIND_GIT_REPO_ANNOUNCEMENT, &pubkey_hex, repo_id) + .map_err(|error| CliError::Other(format!("failed to build delete event: {error}")))? + .custom_created_at(next_ts); + + let event = client.sign_event(tombstone)?; + // Captured before submit: `submit_event` consumes the event, and the + // tombstone id is what an operator needs to audit the retraction. + let tombstone_id = event.id.to_hex(); + let raw = client.submit_event(event).await?; + parse_write_response(&raw, "delete event was dominated; a newer head exists")?; + + // Post-submit verification: re-query to confirm the head is gone. + if let Some(survivor) = fetch_own_repo_announcement(client, repo_id).await? { + return Err(CliError::Conflict(format!( + "repository {repo_id:?} still exists (head at {}); a concurrent write raced the delete", + survivor.created_at.as_secs() + ))); + } + + println!( + "{}", + serde_json::json!({ + "deleted": repo_id, + "event_id": tombstone_id, + "status": "ok", + }) + ); + Ok(()) +} + pub async fn cmd_get_repo( client: &BuzzClient, repo_id: &str, @@ -434,6 +497,7 @@ pub async fn dispatch(cmd: crate::ReposCmd, client: &BuzzClient) -> Result<(), C ReposCmd::Get { id, owner } => cmd_get_repo(client, &id, owner.as_deref()).await, ReposCmd::List { owner, limit } => cmd_list_repos(client, owner.as_deref(), limit).await, ReposCmd::Bind { id, channel } => cmd_bind_repo(client, &id, &channel).await, + ReposCmd::Delete { id } => cmd_delete_repo(client, &id).await, ReposCmd::Protect(command) => match command { ReposProtectCmd::List { id } => cmd_protect_list(client, &id).await, ReposProtectCmd::Set { @@ -468,7 +532,7 @@ mod tests { use super::{ build_create_announcement, build_protection_tag, build_updated_repo_announcement, - protection_rules_json, validate_write_response, RepoChange, + next_announcement_timestamp, protection_rules_json, validate_write_response, RepoChange, }; fn signed_repo(tags: Vec, content: &str, created_at: u64) -> nostr::Event { @@ -844,4 +908,105 @@ mod tests { }) ); } + + // ── delete tombstone timestamp ──────────────────────────────────────────── + + /// An aged announcement must not produce an aged tombstone. `head + 1` + /// alone would sign an hour in the past here, which the relay rejects as + /// "event timestamp too far from server time" — and permanently, since the + /// stored head only ages further. + #[test] + fn next_announcement_timestamp_uses_wall_clock_when_head_is_aged() { + let now = Timestamp::now().as_secs(); + let head = signed_repo(vec![tag(&["d", "demo"])], "", now - 3_600); + + let next = next_announcement_timestamp(&head) + .expect("no overflow") + .as_secs(); + + assert!( + next >= now, + "aged head must advance to wall clock, got {next} against now {now}" + ); + } + + /// A head already inside the relay's window must land at wall clock rather + /// than jumping past it. + #[test] + fn next_announcement_timestamp_stays_at_wall_clock_for_a_current_head() { + let now = Timestamp::now().as_secs(); + let head = signed_repo(vec![tag(&["d", "demo"])], "", now - 1); + + let next = next_announcement_timestamp(&head) + .expect("no overflow") + .as_secs(); + + assert!( + next >= now && next <= now + 1, + "current head must land at wall clock, got {next} against now {now}" + ); + } + + /// A head in the future must still be superseded strictly, so a tombstone + /// can never be dominated by the announcement it is retracting. + #[test] + fn next_announcement_timestamp_returns_head_plus_one_when_head_is_ahead() { + let far_future = 9_999_999_999u64; // year 2286 + let head = signed_repo(vec![tag(&["d", "demo"])], "", far_future); + + let next = next_announcement_timestamp(&head) + .expect("no overflow") + .as_secs(); + + assert_eq!( + next, + far_future + 1, + "tombstone must be strictly after a future head" + ); + } + + /// `u64::MAX` cannot be advanced — the checked add must surface an error + /// rather than wrapping to zero and signing a 1970 timestamp. + #[test] + fn next_announcement_timestamp_rejects_overflow_at_u64_max() { + let head = signed_repo(vec![tag(&["d", "demo"])], "", u64::MAX); + + let error = next_announcement_timestamp(&head).expect_err("u64::MAX must not advance"); + + assert!( + matches!(error, crate::error::CliError::Other(ref message) + if message.contains("timestamp cannot be advanced")), + "expected a timestamp-advance error, got {error:?}" + ); + } + + /// The tombstone must be a NIP-09 kind:5 carrying exactly the addressable + /// coordinate `30617::` — that coordinate is what the + /// relay matches to retract the announcement. + #[test] + fn delete_tombstone_targets_the_repo_announcement_coordinate() { + let pubkey = "a".repeat(64); + let tombstone = buzz_sdk::build_delete_addressable( + buzz_core::kind::KIND_GIT_REPO_ANNOUNCEMENT, + &pubkey, + "demo", + ) + .expect("valid tombstone") + .custom_created_at(Timestamp::from(1_700_000_000u64)) + .sign_with_keys(&Keys::generate()) + .expect("sign tombstone"); + + assert_eq!(tombstone.kind, Kind::Custom(5)); + let coords: Vec<&str> = tombstone + .tags + .iter() + .filter_map(|t| { + let values = t.as_slice(); + (values.first().map(String::as_str) == Some("a")) + .then(|| values.get(1).map(String::as_str)) + .flatten() + }) + .collect(); + assert_eq!(coords, vec![format!("30617:{pubkey}:demo").as_str()]); + } } diff --git a/crates/buzz-cli/src/lib.rs b/crates/buzz-cli/src/lib.rs index 8a8bb053b0..fed701ce19 100644 --- a/crates/buzz-cli/src/lib.rs +++ b/crates/buzz-cli/src/lib.rs @@ -1174,6 +1174,19 @@ pub enum ReposCmd { #[arg(long)] channel: String, }, + /// Delete one of your repository announcements (NIP-09 kind:5). + /// + /// Retracts the kind:30617 announcement at `30617::` so it stops + /// appearing in repository and project listings. Scoped to the caller's own + /// pubkey — another author's announcement at the same id is never touched. + /// + /// This removes the announcement only. Any relay-hosted bare repository + /// storage behind it is an operator concern and is not reclaimed here. + Delete { + /// Repository identifier (d-tag). + #[arg(long)] + id: String, + }, /// Manage branch and tag protection rules on one of your repositories. #[command(subcommand)] Protect(ReposProtectCmd), @@ -2226,7 +2239,7 @@ mod tests { ); assert_eq!( names(&cmd, "repos"), - vec!["bind", "create", "get", "list", "protect"] + vec!["bind", "create", "delete", "get", "list", "protect"] ); let repos = cmd .get_subcommands() @@ -2302,7 +2315,7 @@ mod tests { ("pr", 5), ("projects", 7), ("reactions", 3), - ("repos", 5), + ("repos", 6), ("social", 7), ("upload", 1), ("users", 5),