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).
This commit is contained in:
parent
edad6f863c
commit
156ca70b1e
6 changed files with 201 additions and 5 deletions
|
|
@ -229,6 +229,22 @@ impl Client {
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// PATCH `<api>/<path>` 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 `<api>/<path>` for an endpoint that returns a
|
/// POST a JSON body to `<api>/<path>` for an endpoint that returns a
|
||||||
/// 2xx with an empty body (so there is nothing to decode). Used by
|
/// 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
|
/// `pr-merge` — Forgejo's merge endpoint answers `200 OK` with no body
|
||||||
|
|
|
||||||
|
|
@ -18,6 +18,7 @@
|
||||||
|
|
||||||
mod body;
|
mod body;
|
||||||
mod client;
|
mod client;
|
||||||
|
mod notify;
|
||||||
mod verbs;
|
mod verbs;
|
||||||
|
|
||||||
use anyhow::{Context, Result};
|
use anyhow::{Context, Result};
|
||||||
|
|
|
||||||
138
hive-forge/src/notify.rs
Normal file
138
hive-forge/src/notify.rs
Normal file
|
|
@ -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
|
||||||
|
/// `<repo>#<number>`, 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<Option<u64>> {
|
||||||
|
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::<u64>().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
|
||||||
|
));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -1,12 +1,21 @@
|
||||||
//! `comment <number> [body sources] [repo]` — post a comment on an
|
//! `comment <number> [body sources] [repo]` — post a comment on an
|
||||||
//! issue or PR.
|
//! 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 <n>` / `view <n>`) 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 clap::Args as ClapArgs;
|
||||||
use serde_json::json;
|
use serde_json::json;
|
||||||
|
|
||||||
use crate::body;
|
use crate::body;
|
||||||
use crate::client::Client;
|
use crate::client::Client;
|
||||||
|
use crate::notify;
|
||||||
use crate::verbs::print_json;
|
use crate::verbs::print_json;
|
||||||
|
|
||||||
#[derive(ClapArgs)]
|
#[derive(ClapArgs)]
|
||||||
|
|
@ -19,17 +28,41 @@ pub struct Args {
|
||||||
/// Read body from a file. `-` means stdin.
|
/// Read body from a file. `-` means stdin.
|
||||||
#[arg(long = "body-file")]
|
#[arg(long = "body-file")]
|
||||||
body_file: Option<String>,
|
body_file: Option<String>,
|
||||||
|
/// Post even when the thread has unread activity (skips the
|
||||||
|
/// read-before-comment guard).
|
||||||
|
#[arg(long)]
|
||||||
|
force: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
/// # Errors
|
/// # Errors
|
||||||
///
|
///
|
||||||
/// Propagates any I/O error from the body input (`--body-file`,
|
/// Returns an error when the read-before-comment guard fires (the thread
|
||||||
/// stdin), any transport error from the Forgejo REST call (network
|
/// has an unread notification and `--force` was not passed), and
|
||||||
/// unreachable, 4xx/5xx response, token missing/invalid), and any
|
/// propagates any I/O error from the body input (`--body-file`, stdin),
|
||||||
/// I/O error from writing the response to stdout.
|
/// 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<()> {
|
pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||||
let body = body::resolve_required(args.body.as_deref(), args.body_file.as_deref(), "comment")?;
|
let body = body::resolve_required(args.body.as_deref(), args.body_file.as_deref(), "comment")?;
|
||||||
let repo = client.repo();
|
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(
|
let resp = client.post_json(
|
||||||
&format!("/repos/{repo}/issues/{}/comments", args.number),
|
&format!("/repos/{repo}/issues/{}/comments", args.number),
|
||||||
&json!({ "body": body }),
|
&json!({ "body": body }),
|
||||||
|
|
|
||||||
|
|
@ -21,6 +21,7 @@ use clap::Args as ClapArgs;
|
||||||
use serde_json::{Value, json};
|
use serde_json::{Value, json};
|
||||||
|
|
||||||
use crate::client::Client;
|
use crate::client::Client;
|
||||||
|
use crate::notify;
|
||||||
use crate::verbs::print_json;
|
use crate::verbs::print_json;
|
||||||
|
|
||||||
/// Forgejo's per-page comment cap. The API caps `limit` at 50 even
|
/// 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)?,
|
Some(n) => fetch_tail(client, repo, args.number, n)?,
|
||||||
None => fetch_head(client, repo, args.number, args.limit)?,
|
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() {
|
if client.json_mode() {
|
||||||
let trimmed: Vec<Value> = comments
|
let trimmed: Vec<Value> = comments
|
||||||
.iter()
|
.iter()
|
||||||
|
|
|
||||||
|
|
@ -6,6 +6,7 @@ use clap::Args as ClapArgs;
|
||||||
use serde_json::Value;
|
use serde_json::Value;
|
||||||
|
|
||||||
use crate::client::Client;
|
use crate::client::Client;
|
||||||
|
use crate::notify;
|
||||||
|
|
||||||
#[derive(ClapArgs)]
|
#[derive(ClapArgs)]
|
||||||
pub struct Args {
|
pub struct Args {
|
||||||
|
|
@ -15,6 +16,9 @@ pub struct Args {
|
||||||
|
|
||||||
pub fn run(client: &Client, args: Args) -> Result<()> {
|
pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||||
let repo = client.repo();
|
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 issue = client.get_json(&format!("/repos/{repo}/issues/{}", args.number))?;
|
||||||
let title = issue.get("title").and_then(Value::as_str).unwrap_or("");
|
let title = issue.get("title").and_then(Value::as_str).unwrap_or("");
|
||||||
let body = issue.get("body").and_then(Value::as_str).unwrap_or("");
|
let body = issue.get("body").and_then(Value::as_str).unwrap_or("");
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue