graft batch 21: 3 lands from backlog-1

changes/WI-2616.md

@@ -0,0 +1 @@
+- graft-attester: in post-land mode, a check that fails and then passes on a retry (a flake) now posts its pass with a `#graft-attempts=N` fragment on the url. The server keeps a record of which check on which land needed N runs, and it survives an attester restart. The land still reads good, with no fail attestation. `post_land::flake_attempts` reads the count back.

changes/WI-2639.md

@@ -0,0 +1 @@
+- Add `graft_proto::forward_workspace_api!`, which hands every `WorkspaceApi` method (with all its arguments) to a wrapper's forwarding macro, so a forwarding wrapper cannot skip `overlapping_drafts` or any later method with a trait default. graft-mirror's `Server` and graft-client's `Backend` use it. A graft-proto test pins the macro's list to the trait. No behaviour change.

changes/WI-2716.md

@@ -0,0 +1 @@
+- graft-mirror: per-land commit trailers now join a message's final trailer block (as `git interpret-trailers` does) instead of starting a new paragraph, so GitHub credits `Co-authored-by` lines from the original message. CRLF and trailing whitespace are normalized.

crates/graft-attester/src/post_land.rs

@@ -24,7 +24,9 @@
 //!   it are attested good; it is attested failed.
 //! - **Retry.** A failed check is run again, up to `retries` more times.
 //!   One that then passes is a flake: the land is good, only the pass is
-//!   attested, and [`PostLand::flakes`] counts it. A check that keeps
+//!   attested, and [`PostLand::flakes`] counts it. The pass's url carries
+//!   `#graft-attempts=N` ([`flake_url`]), so the server keeps which check
+//!   on which land needed N runs across a restart. A check that keeps
 //!   failing is attested failed once per run, so the server's row reads
 //!   `bad`.
 //! - **A posted failure is final.** To the server a fail followed by a
@@ -72,8 +74,8 @@ use graft_client::workcopy::{self, WcConfig};
 use graft_model::ChangesetId;
 use graft_proto::api::{ObjectApi, RepoApi};
 use graft_proto::types::{
-    AttestKind, AttestStatus, AttestV2, GetChangesets, JournalEntry, Land, Landed, MergeMode,
-    ResolveRef, Revert, Watch,
+    AttestKind, AttestStatus, AttestV2, GetChangesets, JournalEntry, Land, Landed, MAX_ATTEST_URL,
+    MergeMode, ResolveRef, Revert, Watch,
 };
 use graft_proto::{ApiError, CallContext};
 
@@ -383,6 +385,35 @@ enum Verdict {
     },
 }
 
+/// The fragment key that marks a flake's pass attestation.
+const ATTEMPTS_KEY: &str = "graft-attempts=";
+
+/// `url` marked as a pass that took `attempts` runs: a
+/// `#graft-attempts=N` fragment (`;`-joined to a fragment the url already
+/// has). The pass attestation is the server's record of the flake, so it
+/// outlives a restart without a second report next to the pass. A url too
+/// long for the marker is cut, at a char boundary, so the marker fits in
+/// [`MAX_ATTEST_URL`].
+pub fn flake_url(url: &str, attempts: u32) -> String {
+    let sep = if url.contains('#') { ';' } else { '#' };
+    let marker = format!("{sep}{ATTEMPTS_KEY}{attempts}");
+    let mut end = url.len().min(MAX_ATTEST_URL.saturating_sub(marker.len()));
+    while !url.is_char_boundary(end) {
+        end -= 1;
+    }
+    format!("{}{marker}", &url[..end])
+}
+
+/// The runs a pass attestation's `url` says the check took, from the
+/// marker [`flake_url`] adds: `Some(n)` for a flake, `None` for a pass on
+/// the first run.
+pub fn flake_attempts(url: &str) -> Option<u32> {
+    let (_, frag) = url.split_once('#')?;
+    frag.split(';')
+        .find_map(|p| p.strip_prefix(ATTEMPTS_KEY))
+        .and_then(|n| n.parse().ok())
+}
+
 /// The post-land checker over an API client and a check runner.
 pub struct PostLand<A, R> {
     api: A,
@@ -854,6 +885,11 @@ impl<A: ObjectApi + RepoApi, R: Runner<A>> PostLand<A, R> {
             }
             match pass {
                 Some(url) => {
+                    let url = if urls.is_empty() {
+                        url
+                    } else {
+                        flake_url(&url, urls.len() as u32 + 1)
+                    };
                     if !urls.is_empty() {
                         let n = self.flakes.entry(check.name.clone()).or_insert(0);
                         *n += 1;

crates/graft-attester/tests/post_land.rs

@@ -10,7 +10,8 @@ use std::sync::{Arc, Mutex};
 use std::time::Duration;
 
 use graft_attester::post_land::{
-    BlockReason, PostLand, PostLandReport, REVERT_CHECK, REVERTS, Run, Runner,
+    BlockReason, PostLand, PostLandReport, REVERT_CHECK, REVERTS, Run, Runner, flake_attempts,
+    flake_url,
 };
 use graft_attester::{Attester, Check, Config, Error, Outcome, call_context};
 use graft_client::{Endpoint, Remote};
@@ -687,6 +688,87 @@ async fn a_flaky_check_is_retried_and_counted() {
     assert!(entries(&rig.engine, "LandReverted").await.is_empty());
 }
 
+/// Pass urls per land and check, from the server's journal.
+async fn pass_urls(engine: &Eng) -> BTreeMap<(String, String), Vec<String>> {
+    let mut out: BTreeMap<(String, String), Vec<String>> = BTreeMap::new();
+    for (_, v) in entries(engine, "Attested").await {
+        let passed = match v.get("status").and_then(|s| s.as_str()) {
+            Some(s) => s == "pass",
+            None => v["passed"].as_bool().expect("passed"),
+        };
+        if passed {
+            out.entry((
+                v["changeset"].as_str().expect("changeset").to_owned(),
+                v["check"].as_str().expect("check").to_owned(),
+            ))
+            .or_default()
+            .push(v["url"].as_str().expect("url").to_owned());
+        }
+    }
+    out
+}
+
+/// WI-2616: a flake leaves a record on the server. Each land's pass of
+/// `ci/test` says it took 2 runs; `ci/build`, which passed first time,
+/// carries no marker. The record is read after the checker is gone and a
+/// restarted checker leaves it as it is (accept 2); the land reads good
+/// with no failure attested (accept 3).
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn a_flake_is_recorded_on_the_server_and_survives_a_restart() {
+    let rig = Rig::new(2).await;
+    let landed = lands(&rig.engine, 2, 0).await;
+    let mut fake = Fake::default();
+    fake.flaky.insert(TEST.to_owned(), 1);
+    let mut c = rig.checker(fake).await;
+    drain(&mut c).await;
+    drop(c);
+
+    // Restarted, with nothing new to check.
+    let mut c = rig.checker(Fake::default()).await;
+    drain(&mut c).await;
+    assert_eq!(c.runner().runs.len(), 0, "nothing re-run");
+    assert!(c.flakes().is_empty(), "the counter is gone");
+    drop(c);
+
+    let urls = pass_urls(&rig.engine).await;
+    for id in &landed {
+        let test = &urls[&(id.to_string(), TEST.to_owned())];
+        assert_eq!(test.len(), 1, "{test:?}");
+        assert_eq!(flake_attempts(&test[0]), Some(2), "{test:?}");
+        let build = &urls[&(id.to_string(), BUILD.to_owned())];
+        assert_eq!(build.len(), 1, "{build:?}");
+        assert_eq!(flake_attempts(&build[0]), None, "{build:?}");
+    }
+    let got = attested(&rig.engine).await;
+    for id in &landed {
+        assert!(got[&id.to_string()].iter().all(|(_, _, p)| *p));
+    }
+    let states = land_states(&rig.engine).await;
+    assert!(
+        landed
+            .iter()
+            .all(|id| states.get(id) == Some(&LandCheckState::Good)),
+        "{states:?}"
+    );
+}
+
+/// The marker joins an existing fragment, reads back, and still fits the
+/// server's url limit when the run url is already at it.
+#[test]
+fn the_flake_marker_reads_back_and_fits() {
+    let u = flake_url("https://ci.example/run/7", 3);
+    assert_eq!(u, "https://ci.example/run/7#graft-attempts=3");
+    assert_eq!(flake_attempts(&u), Some(3));
+    let u = flake_url("https://ci.example/run/7#log", 2);
+    assert_eq!(u, "https://ci.example/run/7#log;graft-attempts=2");
+    assert_eq!(flake_attempts(&u), Some(2));
+    assert_eq!(flake_attempts("https://ci.example/run/7#log"), None);
+    let long = format!("https://ci.example/{}", "é".repeat(MAX_ATTEST_URL));
+    let u = flake_url(&long, 4);
+    assert!(u.len() <= MAX_ATTEST_URL, "{}", u.len());
+    assert_eq!(flake_attempts(&u), Some(4));
+}
+
 /// Accept 3, the other side: a check that fails every retry makes the land
 /// bad, one failure attested per run.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]

crates/graft-client/src/backend.rs

@@ -2,7 +2,7 @@
 
 use std::sync::Arc;
 
-use graft_model::{ChangesetId, ObjectId};
+use graft_model::ObjectId;
 use graft_proto::api::{ObjectApi, RepoApi, WorkspaceApi};
 use graft_proto::context::CallContext;
 use graft_proto::error::ApiResult;
@@ -89,71 +89,15 @@ impl RepoApi for Backend {
     graft_proto::forward_repo_api!(forward_repo);
 }
 
+/// One `WorkspaceApi` method of [`Backend`], through `forward!`.
+macro_rules! forward_workspace {
+    ($($name:ident($($arg:ident: $ty:ty),*) -> $out:ty;)*) => {$(
+        async fn $name(&self, cx: &CallContext, $($arg: $ty),*) -> ApiResult<$out> {
+            forward!(self.$name(cx $(, $arg)*))
+        }
+    )*};
+}
+
 impl WorkspaceApi for Backend {
-    async fn open_workspace(
-        &self,
-        cx: &CallContext,
-        req: OpenWorkspace,
-    ) -> ApiResult<WorkspaceView> {
-        forward!(self.open_workspace(cx, req))
-    }
-    async fn get_workspace(&self, cx: &CallContext, req: WorkspaceRef) -> ApiResult<WorkspaceView> {
-        forward!(self.get_workspace(cx, req))
-    }
-    async fn list_workspaces(
-        &self,
-        cx: &CallContext,
-        repo: String,
-        page: Page,
-    ) -> ApiResult<Vec<WorkspaceView>> {
-        forward!(self.list_workspaces(cx, repo, page))
-    }
-    async fn renew_lease(
-        &self,
-        cx: &CallContext,
-        req: WorkspaceRef,
-        lease_ms: u64,
-    ) -> ApiResult<WorkspaceView> {
-        forward!(self.renew_lease(cx, req, lease_ms))
-    }
-    async fn set_head(&self, cx: &CallContext, req: SetHead) -> ApiResult<WorkspaceView> {
-        forward!(self.set_head(cx, req))
-    }
-    async fn sync(
-        &self,
-        cx: &CallContext,
-        req: WorkspaceRef,
-        onto: Option<ChangesetId>,
-    ) -> ApiResult<Synced> {
-        forward!(self.sync(cx, req, onto))
-    }
-    async fn stack(
-        &self,
-        cx: &CallContext,
-        req: WorkspaceRef,
-        op: StackOp,
-    ) -> ApiResult<WorkspaceView> {
-        forward!(self.stack(cx, req, op))
-    }
-    async fn close_workspace(
-        &self,
-        cx: &CallContext,
-        req: WorkspaceRef,
-        keep_drafts: bool,
-    ) -> ApiResult<()> {
-        forward!(self.close_workspace(cx, req, keep_drafts))
-    }
-    async fn get_conflicts(&self, cx: &CallContext, req: GetConflicts) -> ApiResult<Conflicts> {
-        forward!(self.get_conflicts(cx, req))
-    }
-    async fn resolve(&self, cx: &CallContext, req: Resolve) -> ApiResult<Resolved> {
-        forward!(self.resolve(cx, req))
-    }
-    async fn overlapping_drafts(
-        &self,
-        cx: &CallContext,
-        req: OverlappingDrafts,
-    ) -> ApiResult<DraftOverlap> {
-        forward!(self.overlapping_drafts(cx, req))
-    }
+    graft_proto::forward_workspace_api!(forward_workspace);
 }

crates/graft-mirror/src/batch/render.rs

@@ -163,16 +163,50 @@ pub(crate) fn land_row(land: &LandLine, appended: bool) -> String {
     )
 }
 
+/// A message with CRLF made LF, trailing whitespace cut from every line,
+/// and leading and trailing blank lines dropped.
+fn normalize_message(message: &str) -> String {
+    let lines: Vec<&str> = message.lines().map(str::trim_end).collect();
+    lines.join("\n").trim_matches('\n').to_owned()
+}
+
+/// Whether a line is a git trailer: `Token: value`, the token made of
+/// letters, digits and `-`.
+fn is_trailer_line(line: &str) -> bool {
+    line.split_once(':').is_some_and(|(token, _)| {
+        !token.is_empty() && token.chars().all(|c| c.is_ascii_alphanumeric() || c == '-')
+    })
+}
+
+/// Whether a normalized message ends in a trailer block: a last paragraph,
+/// not the subject, whose lines are all trailers (or their indented
+/// continuations).
+pub(crate) fn ends_in_trailers(message: &str) -> bool {
+    let Some((_, last)) = message.rsplit_once("\n\n") else {
+        return false;
+    };
+    let mut lines = last.lines();
+    lines.next().is_some_and(is_trailer_line)
+        && lines.all(|l| is_trailer_line(l) || l.starts_with([' ', '\t']))
+}
+
 /// A land's per-land commit message (D26): its own message (or a stand-in
 /// when it has none), then the `Graft-*` trailers.
 pub(crate) fn land_message(number: u64, land: &LandLine) -> String {
-    let text = land.message.trim();
+    let text = normalize_message(&land.message);
     let mut m = if text.is_empty() {
         format!("graft land {}", short(land.id))
     } else {
-        text.to_owned()
+        text
     };
-    m.push_str(&format!("\n\n{CHANGESET_TRAILER} {}\n", land.id));
+    // Join a final trailer block (as `git interpret-trailers` does), so a
+    // forge that parses only the last block still sees `Co-authored-by`.
+    if ends_in_trailers(&m) {
+        m.push('\n');
+    } else {
+        m.push_str("\n\n");
+    }
+    m.push_str(&format!("{CHANGESET_TRAILER} {}\n", land.id));
     if let Some(s) = &land.trailer_session {
         m.push_str(&format!("Graft-Session: {s}\n"));
     }

crates/graft-mirror/src/batch/tests.rs

@@ -519,3 +519,67 @@ fn the_land_cap_applies_within_the_settled_prefix_and_never_splits_a_revert() {
     let rows = rows_of(&[Some(Bad), Some(Good), Some(Good)]);
     assert_eq!(settled_keep(&lands, &rows, 1).0, 2);
 }
+
+fn message_for(text: &str) -> String {
+    let mut l = land(1, false);
+    l.message = text.to_owned();
+    l.trailer_session = None;
+    l.trailer_work_item = None;
+    land_message(4, &l)
+}
+
+#[test]
+fn trailers_join_a_final_trailer_block() {
+    let msg = message_for("fix: thing\n\nbody text\n\nCo-authored-by: X <x@example.com>\n");
+    let id = land(1, false).id;
+    assert_eq!(
+        msg,
+        format!(
+            "fix: thing\n\nbody text\n\nCo-authored-by: X <x@example.com>\n\
+             Graft-Changeset: {id}\nGraft-Batch: 4\n"
+        )
+    );
+}
+
+#[test]
+fn trailers_start_a_new_block_after_a_plain_body() {
+    let id = land(1, false).id;
+    assert_eq!(
+        message_for("fix: thing\n\nbody text"),
+        format!("fix: thing\n\nbody text\n\nGraft-Changeset: {id}\nGraft-Batch: 4\n")
+    );
+    // A subject alone is never a trailer block, even with a colon.
+    assert_eq!(
+        message_for("fix: thing"),
+        format!("fix: thing\n\nGraft-Changeset: {id}\nGraft-Batch: 4\n")
+    );
+}
+
+#[test]
+fn a_body_line_with_a_colon_is_not_a_trailer() {
+    let id = land(1, false).id;
+    assert_eq!(
+        message_for("subject\n\nThis fixes the parser: it now stops."),
+        format!(
+            "subject\n\nThis fixes the parser: it now stops.\n\n\
+             Graft-Changeset: {id}\nGraft-Batch: 4\n"
+        )
+    );
+    // One prose line spoils the block, as in git.
+    assert!(
+        message_for("subject\n\nSigned-off-by: A <a@example.com>\nand some prose")
+            .contains("prose\n\nGraft-Changeset:")
+    );
+}
+
+#[test]
+fn crlf_and_trailing_whitespace_still_join_the_block() {
+    let id = land(1, false).id;
+    assert_eq!(
+        message_for("subject  \r\n\r\nCo-authored-by: X <x@example.com>  \r\n\r\n  "),
+        format!(
+            "subject\n\nCo-authored-by: X <x@example.com>\n\
+             Graft-Changeset: {id}\nGraft-Batch: 4\n"
+        )
+    );
+}