From 156ca70b1e8d76676196b6a2744a0cd984f93b6d Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 22 Jun 2026 18:54:40 +0200 Subject: [PATCH] feat(#1888): read-before-comment guard via forge notification read-state Rebuild on forge's own notification read-state instead of the local seen-cursor mirror (operator-nacked: mirrors state forge owns + drifts on restart). - comment refuses to post when forge has an unread notification for the thread (someone commented since you last read it); --force overrides. Degrades open if the notification check itself fails. - comments / view mark the thread's notification read (the "I've seen it" signal), clearing the guard for a subsequent reply. - new crate::notify (no local file): unread_thread_id pages repo-scoped unread notifications (newest-first, capped) + subject-url number match; mark_thread_read via the new client.patch_no_content. Pairs with the forge_notify harness change (#1895, merged) that leaves delivered notifications unread until the agent actually reads. Covers pr/issue comment too (they delegate to comment::run). --- hive-forge/src/client.rs | 16 ++++ hive-forge/src/main.rs | 1 + hive-forge/src/notify.rs | 138 +++++++++++++++++++++++++++++++ hive-forge/src/verbs/comment.rs | 43 ++++++++-- hive-forge/src/verbs/comments.rs | 4 + hive-forge/src/verbs/view.rs | 4 + 6 files changed, 201 insertions(+), 5 deletions(-) create mode 100644 hive-forge/src/notify.rs diff --git a/hive-forge/src/client.rs b/hive-forge/src/client.rs index 16a81223..c6ffa3d6 100644 --- a/hive-forge/src/client.rs +++ b/hive-forge/src/client.rs @@ -229,6 +229,22 @@ impl Client { Ok(()) } + /// PATCH `/` for an endpoint that returns a 2xx with an + /// empty body (nothing to decode). Used to mark a notification thread + /// read (`/notifications/threads/{id}` answers `205 Reset Content`). + /// + /// # Errors + /// + /// Returns an error if the request fails to send (transport/network + /// error) or the server responds with a non-2xx status (the response + /// body is included in the error). + pub fn patch_no_content(&self, path: &str) -> Result<()> { + let url = format!("{}{}", self.api(), path); + let resp = self.http.patch(&url).send().context("PATCH")?; + check_status(resp, &format!("PATCH {url}"))?; + Ok(()) + } + /// POST a JSON body to `/` for an endpoint that returns a /// 2xx with an empty body (so there is nothing to decode). Used by /// `pr-merge` — Forgejo's merge endpoint answers `200 OK` with no body diff --git a/hive-forge/src/main.rs b/hive-forge/src/main.rs index bc0f26a9..0e82b6ed 100644 --- a/hive-forge/src/main.rs +++ b/hive-forge/src/main.rs @@ -18,6 +18,7 @@ mod body; mod client; +mod notify; mod verbs; use anyhow::{Context, Result}; diff --git a/hive-forge/src/notify.rs b/hive-forge/src/notify.rs new file mode 100644 index 00000000..d29179dd --- /dev/null +++ b/hive-forge/src/notify.rs @@ -0,0 +1,138 @@ +//! Read-before-comment guard backed by forge's own notification +//! read-state — no local mirror (which drifts across restarts). +//! +//! When someone comments on an issue/PR the caller participates in, +//! forge raises an unread notification for that thread; once the agent +//! actually reads the thread (`comments` / `view`) we mark it read via +//! the notifications API. So "is there unread activity here?" is just +//! "does forge still have an unread notification for this thread?" — +//! forge is the single source of truth, nothing to lose on restart. +//! +//! (This pairs with the `forge_notify` harness change that stops +//! marking notifications read on wake *delivery* — issue tracker +//! `forge cli: agents keep commenting without reading prev comments`.) + +use anyhow::Result; +use serde_json::Value; + +use crate::client::Client; + +/// Forgejo's per-page notification cap. +const PAGE_SIZE: u64 = 50; +/// How many pages of unread notifications to scan for the thread. +/// Notifications come newest-first and a thread the caller is about to +/// comment on was just active, so it sits near the top; this cap keeps +/// the guard cheap even when the unread list is large (an uncontrolled +/// firehose), at the cost of not detecting a match buried past +/// `MAX_PAGES * PAGE_SIZE` unread items (degrade-open — acceptable for +/// a courtesy guard). +const MAX_PAGES: u64 = 5; + +/// The notification thread id of an UNREAD notification on +/// `#`, or `None` when the thread has no unread +/// notification (nothing to read / already read). Repo-scoped so a bare +/// number can't false-match another repo. Errors only on transport / +/// non-2xx — callers degrade open on `Err`. +pub fn unread_thread_id(client: &Client, repo: &str, number: u64) -> Result> { + for page in 1..=MAX_PAGES { + let v = client.get_json(&format!( + "/repos/{repo}/notifications?all=false&page={page}&limit={PAGE_SIZE}" + ))?; + let arr = v.as_array().cloned().unwrap_or_default(); + let len = arr.len() as u64; + for n in &arr { + let subject_url = n + .get("subject") + .and_then(|s| s.get("url")) + .and_then(Value::as_str) + .unwrap_or(""); + if subject_matches(subject_url, number) { + return Ok(n.get("id").and_then(Value::as_u64)); + } + } + // Last (short) page reached — stop. + if len < PAGE_SIZE { + break; + } + } + Ok(None) +} + +/// Mark a notification thread read (best-effort; the caller ignores the +/// result). Clears the unread signal so a subsequent comment isn't +/// blocked after the agent has read the thread. +pub fn mark_thread_read(client: &Client, thread_id: u64) -> Result<()> { + client.patch_no_content(&format!("/notifications/threads/{thread_id}")) +} + +/// Reading a thread (`comments` / `view`) is the "I've seen it" signal: +/// clear its unread notification so the read-before-comment guard lets a +/// subsequent comment through. Fully best-effort — never fails or noises +/// up the read verb (a thread with no unread notification is a no-op). +pub fn mark_read_best_effort(client: &Client, repo: &str, number: u64) { + if let Ok(Some(id)) = unread_thread_id(client, repo, number) { + let _ = mark_thread_read(client, id); + } +} + +/// True when a notification `subject.url` refers to issue/PR `number` in +/// the (already repo-scoped) query. Forgejo subject URLs end in the +/// issue/PR number (`…/repos/o/r/issues/42`, or `…/pulls/42`); match the +/// trailing path segment. Tolerates a trailing slash. +#[must_use] +pub fn subject_matches(subject_url: &str, number: u64) -> bool { + subject_url + .trim_end_matches('/') + .rsplit('/') + .next() + .and_then(|seg| seg.parse::().ok()) + == Some(number) +} + +#[cfg(test)] +mod tests { + use super::subject_matches; + + #[test] + fn matches_issue_subject_url() { + assert!(subject_matches( + "https://forge/api/v1/repos/o/r/issues/42", + 42 + )); + } + + #[test] + fn matches_pull_subject_url() { + assert!(subject_matches("https://forge/api/v1/repos/o/r/pulls/7", 7)); + } + + #[test] + fn tolerates_trailing_slash() { + assert!(subject_matches( + "https://forge/api/v1/repos/o/r/issues/9/", + 9 + )); + } + + #[test] + fn rejects_different_number() { + assert!(!subject_matches( + "https://forge/api/v1/repos/o/r/issues/42", + 7 + )); + } + + #[test] + fn rejects_non_numeric_tail() { + assert!(!subject_matches("https://forge/api/v1/repos/o/r", 42)); + } + + #[test] + fn rejects_substring_number() { + // 142 must not match 42 — full-segment parse, not substring. + assert!(!subject_matches( + "https://forge/api/v1/repos/o/r/issues/142", + 42 + )); + } +} diff --git a/hive-forge/src/verbs/comment.rs b/hive-forge/src/verbs/comment.rs index f5bd774c..c7d24d03 100644 --- a/hive-forge/src/verbs/comment.rs +++ b/hive-forge/src/verbs/comment.rs @@ -1,12 +1,21 @@ //! `comment [body sources] [repo]` — post a comment on an //! issue or PR. +//! +//! Read-before-comment guard: before posting, refuse when forge still +//! has an unread notification for the thread — i.e. someone commented +//! since the caller last read it. Reading the thread +//! (`hive-forge comments ` / `view `) marks the notification read +//! and clears the block; `--force` overrides. The unread signal is +//! forge's own notification read-state, not a local mirror — see +//! `crate::notify`. -use anyhow::Result; +use anyhow::{Result, bail}; use clap::Args as ClapArgs; use serde_json::json; use crate::body; use crate::client::Client; +use crate::notify; use crate::verbs::print_json; #[derive(ClapArgs)] @@ -19,17 +28,41 @@ pub struct Args { /// Read body from a file. `-` means stdin. #[arg(long = "body-file")] body_file: Option, + /// Post even when the thread has unread activity (skips the + /// read-before-comment guard). + #[arg(long)] + force: bool, } /// # Errors /// -/// Propagates any I/O error from the body input (`--body-file`, -/// stdin), any transport error from the Forgejo REST call (network -/// unreachable, 4xx/5xx response, token missing/invalid), and any -/// I/O error from writing the response to stdout. +/// Returns an error when the read-before-comment guard fires (the thread +/// has an unread notification and `--force` was not passed), and +/// propagates any I/O error from the body input (`--body-file`, stdin), +/// any transport error from the Forgejo REST call (network unreachable, +/// 4xx/5xx response, token missing/invalid), and any I/O error from +/// writing the response to stdout. pub fn run(client: &Client, args: Args) -> Result<()> { let body = body::resolve_required(args.body.as_deref(), args.body_file.as_deref(), "comment")?; let repo = client.repo(); + + if !args.force { + // Degrade open: a transport failure checking notifications must + // not block a legitimate comment — only a *confirmed* unread + // thread refuses. + match notify::unread_thread_id(client, repo, args.number) { + Ok(Some(_)) => bail!( + "hive-forge: {repo}#{0} has unread activity — someone commented since you last \ + read it. Read it first (`hive-forge comments {0}`), then retry — or pass --force.", + args.number + ), + Ok(None) => {} + Err(e) => eprintln!( + "hive-forge: warning: could not check notifications ({e}); posting anyway" + ), + } + } + let resp = client.post_json( &format!("/repos/{repo}/issues/{}/comments", args.number), &json!({ "body": body }), diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index 36783665..610dc1c6 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -21,6 +21,7 @@ use clap::Args as ClapArgs; use serde_json::{Value, json}; use crate::client::Client; +use crate::notify; use crate::verbs::print_json; /// Forgejo's per-page comment cap. The API caps `limit` at 50 even @@ -51,6 +52,9 @@ pub fn run(client: &Client, args: Args) -> Result<()> { Some(n) => fetch_tail(client, repo, args.number, n)?, None => fetch_head(client, repo, args.number, args.limit)?, }; + // Reading the thread clears its unread notification so the + // read-before-comment guard (in `comment`) lets a reply through. + notify::mark_read_best_effort(client, repo, args.number); if client.json_mode() { let trimmed: Vec = comments .iter() diff --git a/hive-forge/src/verbs/view.rs b/hive-forge/src/verbs/view.rs index b0ab2474..23207cbc 100644 --- a/hive-forge/src/verbs/view.rs +++ b/hive-forge/src/verbs/view.rs @@ -6,6 +6,7 @@ use clap::Args as ClapArgs; use serde_json::Value; use crate::client::Client; +use crate::notify; #[derive(ClapArgs)] pub struct Args { @@ -15,6 +16,9 @@ pub struct Args { pub fn run(client: &Client, args: Args) -> Result<()> { let repo = client.repo(); + // Reading the thread clears its unread notification so the + // read-before-comment guard (in `comment`) lets a reply through. + notify::mark_read_best_effort(client, repo, args.number); let issue = client.get_json(&format!("/repos/{repo}/issues/{}", args.number))?; let title = issue.get("title").and_then(Value::as_str).unwrap_or(""); let body = issue.get("body").and_then(Value::as_str).unwrap_or("");