only the controller's own comments carry the marker, and a cleared comment says resolved

docs/swarm/builder.md

@@ -230,9 +230,10 @@ its deadline, the controller also comments once on that PR, with its forge
 account: the agent, the `internal/meta` commit of the first red build, `nix`'s last lines (at most 4000
 bytes), and that the deploy waits and the swarm builds it again every round. A comment
 carries a marker line with the agent, and the controller posts only if the PR
-holds no comment with that marker. The retry rounds stay quiet whatever commit
+holds no comment of its own with that marker; a copy of the marker in someone else's comment is neither counted nor edited. The retry rounds stay quiet whatever commit
 they build, and the latest error stays on the Jobs page. A green build of the
-agent for that PR rewrites the marker, so a later red comments again. A `no verdict` failure, and a build that isn't a
+agent for that PR rewrites the marker and appends the build's commit to the comment, so a later red comments again.
+A `no verdict` failure, and a build that isn't a
 merged PR's (a new agent, the pin fan-out), leave no comment. When the forge
 refuses the comment, the controller logs a warning and the node ends as before.
 

swarm-controller/src/forge/pr_comment.rs

@@ -8,6 +8,25 @@ use forgejo_api::structs::{
 use super::Client;
 
 impl Client {
+    /// Every comment on pull request `pr` of `owner/repo` written by this
+    /// client's own forge login. A comment by anyone else can carry a marker
+    /// (a quote-reply copies it) and is neither counted nor edited.
+    async fn own_comments(&self, owner: &str, repo: &str, pr: u64) -> Result<Vec<Comment>> {
+        let me = self
+            .api
+            .call(|api| api.user_get_current().into_future())
+            .await
+            .context("reading this client's own forge login")?
+            .login
+            .context("the forge returned no login for this client")?;
+        Ok(self
+            .comments(owner, repo, pr)
+            .await?
+            .into_iter()
+            .filter(|c| c.user.as_ref().and_then(|u| u.login.as_deref()) == Some(me.as_str()))
+            .collect())
+    }
+
     /// Every comment on pull request `pr` of `owner/repo`.
     async fn comments(&self, owner: &str, repo: &str, pr: u64) -> Result<Vec<Comment>> {
         let index = i64::try_from(pr).context("PR number out of range")?;
@@ -45,7 +64,7 @@ impl Client {
         body: &str,
     ) -> Result<bool> {
         let index = i64::try_from(pr).context("PR number out of range")?;
-        let existing = self.comments(owner, repo, pr).await?;
+        let existing = self.own_comments(owner, repo, pr).await?;
         if existing
             .iter()
             .any(|c| c.body.as_deref().is_some_and(|b| b.contains(marker)))
@@ -66,10 +85,16 @@ impl Client {
         Ok(true)
     }
 
-    /// Rewrite every comment on pull request `pr` of `owner/repo` that
+    /// Rewrite every comment of this client's own on pull request `pr` of
-    /// contains `marker` so it contains `cleared` instead, which lets
+    /// `owner/repo` that contains `marker`: the marker becomes `cleared`, which
-    /// [`Self::comment_once`] post that marker again. How many comments were
+    /// lets [`Self::comment_once`] post that marker again, and `resolved` is
-    /// rewritten.
+    /// appended on a line of its own. A comment the forge refuses to edit is
+    /// logged and skipped, so one refusal leaves the rest cleared. How many
+    /// comments were rewritten.
+    ///
+    /// # Errors
+    ///
+    /// The comments could not be listed.
     pub(crate) async fn clear_marker(
         &self,
         owner: &str,
@@ -77,9 +102,10 @@ impl Client {
         pr: u64,
         marker: &str,
         cleared: &str,
+        resolved: &str,
     ) -> Result<usize> {
         let mut rewritten = 0;
-        for comment in self.comments(owner, repo, pr).await? {
+        for comment in self.own_comments(owner, repo, pr).await? {
             let (Some(id), Some(body)) = (comment.id, comment.body.as_deref()) else {
                 continue;
             };
@@ -87,17 +113,23 @@ impl Client {
                 continue;
             }
             let option = EditIssueCommentOption {
-                body: body.replace(marker, cleared),
+                body: format!("{}\n\n{resolved}", body.replace(marker, cleared)),
                 updated_at: None,
             };
-            self.api
+            let edited = self
+                .api
                 .call(|api| {
                     api.issue_edit_comment(owner, repo, id, option.clone())
                         .into_future()
                 })
-                .await
+                .await;
-                .with_context(|| format!("clearing comment {id} on {owner}/{repo}#{pr}"))?;
+            match edited {
-            rewritten += 1;
+                Ok(_) => rewritten += 1,
+                Err(e) => tracing::warn!(
+                    id, %owner, %repo, pr, error = %format!("{e:#}"),
+                    "swarm forge: could not rewrite a resolved build comment"
+                ),
+            }
         }
         Ok(rewritten)
     }
@@ -115,6 +147,13 @@ pub(crate) mod tests {
     /// posted to and their body.
     pub(crate) type Posted = Arc<Mutex<Vec<(String, String)>>>;
 
+    /// A comment whose body starts with this is written by someone other than
+    /// the client's own login.
+    pub(crate) const HUMAN: &str = "HUMAN:";
+
+    /// A comment whose body contains this is one the forge refuses to edit.
+    pub(crate) const REFUSED: &str = "REFUSED";
+
     /// A stand-in forge that stores the comments posted to any issue path and
     /// lists them back.
     pub(crate) async fn stand_in() -> (Client, Posted) {
@@ -127,14 +166,20 @@ pub(crate) mod tests {
                     let path = uri.path().to_owned();
                     let mut held = state.lock().unwrap();
                     match method.as_str() {
+                        "GET" if path == "/api/v1/user" => (
+                            StatusCode::OK,
+                            [("x-total-count", String::new())],
+                            r#"{"login":"bot"}"#.to_owned(),
+                        ),
                         "GET" => {
                             let list: Vec<_> = held
                                 .iter()
                                 .enumerate()
                                 .filter(|(_, (p, _))| *p == path)
-                                .map(
+                                .map(|(id, (_, body))| {
-                                    |(id, (_, body))| serde_json::json!({ "id": id, "body": body }),
+                                    let author = if body.starts_with(HUMAN) { "human" } else { "bot" };
-                                )
+                                    serde_json::json!({ "id": id, "body": body, "user": { "login": author } })
+                                })
                                 .collect();
                             let count = list.len().to_string();
                             (
@@ -146,6 +191,13 @@ pub(crate) mod tests {
                         "PATCH" => {
                             let sent: serde_json::Value = serde_json::from_slice(&body).unwrap();
                             let id: usize = path.rsplit('/').next().unwrap().parse().unwrap();
+                            if held[id].1.contains(REFUSED) {
+                                return (
+                                    StatusCode::FORBIDDEN,
+                                    [("x-total-count", String::new())],
+                                    r#"{"message":"refused"}"#.to_owned(),
+                                );
+                            }
                             held[id].1 = sent["body"].as_str().unwrap().to_owned();
                             (
                                 StatusCode::OK,
@@ -202,8 +254,12 @@ pub(crate) mod tests {
         );
     }
 
+    fn issue_path() -> String {
+        format!("/api/v1/repos/o/r/{}/7/comments", "issues")
+    }
+
     #[tokio::test]
-    async fn a_cleared_marker_can_be_posted_again() {
+    async fn a_cleared_marker_can_be_posted_again_and_the_old_comment_says_resolved() {
         let (client, held) = stand_in().await;
         let (marker, cleared) = ("<!-- a -->", "<!-- a cleared -->");
         assert!(
@@ -212,20 +268,9 @@ pub(crate) mod tests {
                 .await
                 .unwrap()
         );
-        assert_eq!(
+        let clear = || client.clear_marker("o", "r", 7, marker, cleared, "Resolved.");
-            client
+        assert_eq!(clear().await.unwrap(), 1);
-                .clear_marker("o", "r", 7, marker, cleared)
+        assert_eq!(clear().await.unwrap(), 0);
-                .await
-                .unwrap(),
-            1
-        );
-        assert_eq!(
-            client
-                .clear_marker("o", "r", 7, marker, cleared)
-                .await
-                .unwrap(),
-            0
-        );
         assert!(
             client
                 .comment_once("o", "r", 7, marker, marker)
@@ -233,7 +278,53 @@ pub(crate) mod tests {
                 .unwrap()
         );
         let held = held.lock().unwrap();
-        assert_eq!(held[0].1, cleared);
+        assert_eq!(held[0].1, format!("{cleared}\n\nResolved."));
         assert_eq!(held[1].1, marker);
     }
+
+    /// A human's comment can carry the marker (a quote-reply copies it): it
+    /// neither counts as the bot's report nor is rewritten.
+    #[tokio::test]
+    async fn a_comment_by_someone_else_with_the_marker_is_neither_counted_nor_edited() {
+        let (client, held) = stand_in().await;
+        let marker = "<!-- a -->";
+        let quoted = format!("{HUMAN} > {marker}");
+        held.lock().unwrap().push((issue_path(), quoted.clone()));
+        assert!(
+            client
+                .comment_once("o", "r", 7, marker, marker)
+                .await
+                .unwrap(),
+            "the human's copy does not silence the bot"
+        );
+        let cleared = client
+            .clear_marker("o", "r", 7, marker, "<!-- c -->", "Resolved.")
+            .await
+            .unwrap();
+        assert_eq!(cleared, 1, "only the bot's own comment");
+        let held = held.lock().unwrap();
+        assert_eq!(held[0].1, quoted, "the human's comment is untouched");
+        assert!(held[1].1.contains("<!-- c -->"), "{:?}", held[1]);
+    }
+
+    /// The forge refusing one edit does not stop the next comment's clear.
+    #[tokio::test]
+    async fn a_refused_edit_still_clears_the_next_comment() {
+        let (client, held) = stand_in().await;
+        let marker = "<!-- a -->";
+        let refused = format!("{marker} {REFUSED}");
+        {
+            let mut held = held.lock().unwrap();
+            held.push((issue_path(), refused.clone()));
+            held.push((issue_path(), marker.to_owned()));
+        }
+        let cleared = client
+            .clear_marker("o", "r", 7, marker, "<!-- c -->", "Resolved.")
+            .await
+            .unwrap();
+        assert_eq!(cleared, 1);
+        let held = held.lock().unwrap();
+        assert_eq!(held[0].1, refused, "control: the refused one is as it was");
+        assert!(held[1].1.contains("<!-- c -->"), "{:?}", held[1]);
+    }
 }

swarm-controller/src/meta_build.rs

@@ -67,13 +67,15 @@ pub(crate) trait MetaRepo {
         body: &str,
     ) -> impl Future<Output = Result<bool>> + Send;
     /// Rewrite the comments on `pr` of `agent`'s config repo that contain
-    /// `marker` so they contain `cleared`, which lets `marker` be posted again.
+    /// `marker` so they contain `cleared` and end with `resolved`, which lets
+    /// `marker` be posted again. Only the controller's own comments count.
     fn clear_marker(
         &self,
         agent: &str,
         pr: u64,
         marker: &str,
         cleared: &str,
+        resolved: &str,
     ) -> impl Future<Output = Result<usize>> + Send;
 }
 
@@ -106,10 +108,19 @@ impl MetaRepo for forge::Client {
         pr: u64,
         marker: &str,
         cleared: &str,
+        resolved: &str,
     ) -> Result<usize> {
         crate::jobq::forge_node(
             "build_meta",
-            forge::Client::clear_marker(self, forge::CONFIG_ORG, agent, pr, marker, cleared),
+            forge::Client::clear_marker(
+                self,
+                forge::CONFIG_ORG,
+                agent,
+                pr,
+                marker,
+                cleared,
+                resolved,
+            ),
         )
         .await
     }
@@ -227,9 +238,19 @@ async fn report_failure(
 /// A green build ends the red spell: clear `pr`'s failure marker for `agent`,
 /// so a later red comments again. A failure is logged and changes nothing
 /// about the node.
-async fn clear_failure(head: &impl MetaRepo, pr: u64, agent: &str) {
+async fn clear_failure(head: &impl MetaRepo, pr: u64, agent: &str, meta_rev: &str) {
+    let resolved = format!(
+        "Resolved: built at `{}`.",
+        meta_rev.get(..12).unwrap_or(meta_rev)
+    );
     match head
-        .clear_marker(agent, pr, &failure_marker(agent), &cleared_marker(agent))
+        .clear_marker(
+            agent,
+            pr,
+            &failure_marker(agent),
+            &cleared_marker(agent),
+            &resolved,
+        )
         .await
     {
         Ok(cleared) => {
@@ -286,6 +307,7 @@ pub(crate) async fn run(
             Outcome::Failed(error_tail)
         }
         Ok(FromMetaReply::Built { out_path, system }) => {
+            let built_rev = meta_rev.clone();
             let closure = AgentClosure {
                 meta_rev,
                 store_path: out_path.clone(),
@@ -295,7 +317,7 @@ pub(crate) async fn run(
             match queue.record(&agent, &closure).await {
                 Ok(()) => {
                     if let Some(pr) = pr {
-                        clear_failure(head, pr, &agent).await;
+                        clear_failure(head, pr, &agent, &built_rev).await;
                     }
                     Outcome::Done
                 }
@@ -364,10 +386,11 @@ mod tests {
             pr: u64,
             marker: &str,
             cleared: &str,
+            resolved: &str,
         ) -> Result<usize> {
             let client = self.comments.as_ref().expect("no clear expected");
             client
-                .clear_marker(forge::CONFIG_ORG, agent, pr, marker, cleared)
+                .clear_marker(forge::CONFIG_ORG, agent, pr, marker, cleared, resolved)
                 .await
         }
     }
@@ -635,6 +658,11 @@ mod tests {
             posted[0]
         );
         assert!(posted[0].1.contains(&cleared_marker("atlas")));
+        assert!(
+            posted[0].1.contains("Resolved: built at `0123456789ab`"),
+            "{:?}",
+            posted[0]
+        );
     }
 
     /// A build that got no verdict says nothing about the PR's config, so it