Merge remote-tracking branch 'origin/main' into feat/files-through-the-store

modules/mailu/automx.ts

@@ -7,14 +7,25 @@ import type { Exec } from "./health.js";
 
 export const AUTOMX_DATABASE = "/data/db.sqlite";
 
-/** The tables read, each with the one query that reads it. Fixed text: the tool takes no query from its caller. */
-export const AUTOMX_TABLES: Record<string, string> = {
-  provider: "SELECT id, name, short_name FROM provider ORDER BY id",
-  server: "SELECT id, prio, name, port, type, socket_type, user_name, authentication FROM server ORDER BY id",
-  domain: "SELECT id, name, provider_id, ldapserver_id FROM domain ORDER BY id",
-  server_domain: "SELECT server_id, domain_id FROM server_domain ORDER BY domain_id, server_id",
+/** The tables read, each with its columns and its ordering. Fixed text: the tool takes no query from its caller. */
+export const AUTOMX_TABLES: Record<string, { columns: string[]; order: string }> = {
+  provider: { columns: ["id", "name", "short_name"], order: "id" },
+  server: { columns: ["id", "prio", "name", "port", "type", "socket_type", "user_name", "authentication"], order: "id" },
+  domain: { columns: ["id", "name", "provider_id", "ldapserver_id"], order: "id" },
+  server_domain: { columns: ["server_id", "domain_id"], order: "domain_id, server_id" },
 };
 
+/**
+ * The one statement that reads a table. The sqlite3 in the mailu-automx image does not know `-json` (it answers
+ * "sqlite3: Error: unknown option: -json", issue 1630), so each column is printed as `<type>:<hex of its bytes>`, which
+ * the oldest sqlite3 prints in its default list mode: no quoting, no separator or newline inside a cell, and a NULL
+ * (`null:`) apart from an empty text (`text:`).
+ */
+export function selectOf(table: string): string {
+  const t = AUTOMX_TABLES[table];
+  return `SELECT ${t.columns.map((c) => `typeof(${c}) || ':' || hex(${c})`).join(", ")} FROM ${table} ORDER BY ${t.order}`;
+}
+
 export type Row = Record<string, string | number | null>;
 
 export type AutomxRows = {
@@ -26,26 +37,43 @@ export type AutomxRows = {
   unreadable: Record<string, string>;
 };
 
-/** sqlite3 -json prints one JSON array, or nothing at all for a table without rows. */
-export function parseRows(output: string): Row[] {
-  const text = output.trim();
-  if (text === "") return [];
-  const rows = JSON.parse(text) as unknown;
-  if (!Array.isArray(rows)) throw new Error("sqlite3 did not print a list of rows");
-  return rows as Row[];
+/** One cell as selectOf prints it: `<type>:<hex>`; text is the bytes read as UTF-8, a number is read from its text. */
+function cellOf(cell: string): string | number | null {
+  const at = cell.indexOf(":");
+  if (at < 0 || cell.length - at - 1 > 0 && !/^([0-9A-Fa-f]{2})+$/.test(cell.slice(at + 1))) throw new Error(`sqlite3 printed ${JSON.stringify(cell.slice(0, 40))}, not a cell`);
+  const type = cell.slice(0, at);
+  const bytes = Buffer.from(cell.slice(at + 1), "hex").toString("utf8");
+  switch (type) {
+    case "null": return null;
+    case "integer": case "real": return Number(bytes);
+    default: return bytes;
+  }
 }
 
-/** The command that reads one table: read-only, JSON, one fixed statement. */
-export function readCommand(sql: string): string[] {
-  return ["sqlite3", "-readonly", "-json", AUTOMX_DATABASE, sql];
+/** sqlite3 prints one line per row, cells apart by `|`, and nothing at all for a table without rows. */
+export function parseRows(table: string, output: string): Row[] {
+  const columns = AUTOMX_TABLES[table].columns;
+  const rows: Row[] = [];
+  for (const line of output.split("\n")) {
+    if (line.trim() === "") continue;
+    const cells = line.replace(/\r$/, "").split("|");
+    if (cells.length !== columns.length) throw new Error(`sqlite3 printed ${cells.length} cells for ${columns.length} columns`);
+    rows.push(Object.fromEntries(columns.map((c, i) => [c, cellOf(cells[i])])));
+  }
+  return rows;
+}
+
+/** The command that reads one table: read-only, one fixed statement. */
+export function readCommand(table: string): string[] {
+  return ["sqlite3", "-readonly", AUTOMX_DATABASE, selectOf(table)];
 }
 
 /** Read every table; one that fails is said in `unreadable` and does not hide the others. */
 export async function readAutomx(exec: Exec, container: string): Promise<AutomxRows> {
   const answer: AutomxRows = { database: AUTOMX_DATABASE, tables: {}, empty_names: [], unreadable: {} };
-  for (const [table, sql] of Object.entries(AUTOMX_TABLES)) {
+  for (const table of Object.keys(AUTOMX_TABLES)) {
     try {
-      answer.tables[table] = parseRows(await exec(container, readCommand(sql)));
+      answer.tables[table] = parseRows(table, await exec(container, readCommand(table)));
     } catch (e) {
       const err = e as { stderr?: string; message?: string };
       const text = (err?.stderr ?? "").trim() || (err?.message ?? String(e)).trim();

modules/mailu/test/automx-rows.test.ts

@@ -2,13 +2,14 @@
 // sqlite3 inside its container. The database here is a real SQLite file made by the test with the tables
 // automx/files/setup-db creates and the rows add-domains writes — with the IMAP server's name empty, the case
 // the live instance is in. The fake container runs each command's statement against that database and prints
-// it the way `sqlite3 -json` does (one JSON array, nothing for no rows). Run against the compiled module.
+// it the way the image's sqlite3 does in list mode (one line per row, cells apart by "|", nothing for no rows); that sqlite3
+// refuses `-json` ("sqlite3: Error: unknown option: -json", captured from novox/mailu.automx_rows, issue 1630). Run against the compiled module.
 
 import { test } from "node:test";
 import assert from "node:assert/strict";
 import { DatabaseSync } from "node:sqlite";
 
-import { readAutomx, readCommand, AUTOMX_DATABASE } from "../dist/automx.js";
+import { readAutomx, readCommand, selectOf, parseRows, AUTOMX_DATABASE } from "../dist/automx.js";
 
 function database(imapName: string): DatabaseSync {
   const db = new DatabaseSync(":memory:");
@@ -32,8 +33,9 @@ function database(imapName: string): DatabaseSync {
 function container(db: DatabaseSync, asked: string[][] = []) {
   return async (_name: string, command: string[]) => {
     asked.push(command);
-    const rows = db.prepare(command[command.length - 1]).all();
-    return rows.length === 0 ? "" : JSON.stringify(rows) + "\n";
+    if (command.includes("-json")) throw Object.assign(new Error("Command failed: docker exec"), { stderr: "sqlite3: Error: unknown option: -json\n" });
+    const rows = db.prepare(command[command.length - 1]).all() as unknown as Record<string, string>[];
+    return rows.length === 0 ? "" : rows.map((r) => Object.values(r).join("|")).join("\n") + "\n";
   };
 }
 
@@ -61,17 +63,18 @@ test("every command opens the file read-only and runs one fixed SELECT", async (
   await readAutomx(container(database("x"), asked), "mailu-automx");
   assert.equal(asked.length, 4);
   for (const c of asked) {
-    assert.deepEqual(c.slice(0, 4), ["sqlite3", "-readonly", "-json", AUTOMX_DATABASE]);
-    assert.match(c[4], /^SELECT /);
-    assert.equal(c.length, 5);
+    assert.deepEqual(c.slice(0, 3), ["sqlite3", "-readonly", AUTOMX_DATABASE]);
+    assert.match(c[3], /^SELECT /);
+    assert.equal(c.length, 4);
+    assert.ok(!c.includes("-json"));
   }
-  assert.deepEqual(readCommand("SELECT 1").length, 5);
+  assert.deepEqual(readCommand("provider"), ["sqlite3", "-readonly", AUTOMX_DATABASE, selectOf("provider")]);
 });
 
 test("a table that cannot be read is said and does not hide the others", async () => {
   const db = database("x");
   const exec = async (name: string, command: string[]) => {
-    if (command[4].includes("FROM domain ")) throw Object.assign(new Error("Command failed: docker exec"), { stderr: "Error: unable to open database file\nmore" });
+    if (command[3].includes("FROM domain ")) throw Object.assign(new Error("Command failed: docker exec"), { stderr: "Error: unable to open database file\nmore" });
     return container(db)(name, command);
   };
   const answer = await readAutomx(exec, "mailu-automx");
@@ -79,3 +82,22 @@ test("a table that cannot be read is said and does not hide the others", async (
   assert.equal(answer.tables.provider.length, 1);
   assert.equal(answer.tables.domain, undefined);
 });
+
+test("a NULL, an empty text, a number and a text holding a bar or a newline are read apart", () => {
+  const hex = (t: string) => Buffer.from(t).toString("hex").toUpperCase();
+  const line = [`integer:${hex("5")}`, `text:${hex("a|b\nc")}`, "text:", "null:"].join("|");
+  const rows = parseRows("domain", line + "\n");
+  assert.deepEqual(rows, [{ id: 5, name: "a|b\nc", provider_id: "", ldapserver_id: null }]);
+});
+
+test("output that is not cells is an unreadable table, not a guess", async () => {
+  const exec = async () => "Error: something else\n";
+  const answer = await readAutomx(exec, "mailu-automx");
+  assert.equal(Object.keys(answer.unreadable).length, 4);
+});
+
+test("the image's refusal of -json is said as the table's reason", async () => {
+  const exec = async (_n: string, command: string[]) => container(database("x"))("", ["sqlite3", "-json", ...command.slice(1)]);
+  const answer = await readAutomx(exec, "mailu-automx");
+  assert.deepEqual(answer.unreadable.domain, "sqlite3: Error: unknown option: -json");
+});

modules/mesh-delivery-gate/.gitignore

@@ -0,0 +1 @@
+/cmd/mesh-delivery-gate/mesh-delivery-gate

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go

@@ -366,6 +366,9 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
 				d.BaseSHA = p.BaseSHA
 				h.keep(d)
 			}
+			// A delivery made from its verdict before its head was heard (issue 1726) holds none of what an announcement
+			// carries: filled from this one, so its base is known and the operator is asked.
+			h.fillFromAnnouncement(d, p)
 			// The review record, corrected in the description without a push, is read again (ADR 0421 §5).
 			if by, at := ReviewIn(p.Body); p.Body != "" && (by != d.ReviewedBy || at != d.ReviewedAt) {
 				d.ReviewedBy, d.ReviewedAt = by, at
@@ -398,18 +401,7 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
 			h.leaveGroup(old)
 		}
 	}
-	replaced := ""
-	for _, old := range h.deliveries {
-		if old.Repository == p.repository() && old.Number == p.Number && !sameCommit(old.Commit, p.HeadSHA) &&
-			!old.State.Final() && old.MergedAs == "" {
-			old.NewerHead = p.HeadSHA
-			h.settleAndKeep(old)
-			if old.Group != "" {
-				h.leaveGroup(old)
-				replaced = old.ID
-			}
-		}
-	}
+	replaced := h.supersedeOlder(p)
 	d, t, err := h.freshAnnouncement(p, now, startedOver)
 	if err != nil {
 		h.refuse(err)
@@ -429,6 +421,49 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
 	return isNew
 }
 
+// supersedeOlder ends the older heads of p's pull request: each is told its newer head, and one in a group leaves it. It
+// returns the id of the one that left a group, whose place the new head takes (takePlaceOf). Under the lock.
+func (h *Holder) supersedeOlder(p PullEvent) (replaced string) {
+	for _, old := range h.deliveries {
+		if old.Repository == p.repository() && old.Number == p.Number && !sameCommit(old.Commit, p.HeadSHA) &&
+			!old.State.Final() && old.MergedAs == "" {
+			old.NewerHead = p.HeadSHA
+			h.settleAndKeep(old)
+			if old.Group != "" {
+				h.leaveGroup(old)
+				replaced = old.ID
+			}
+		}
+	}
+	return replaced
+}
+
+// fillFromAnnouncement fills a delivery made from its check's verdict before its pull request's head was heard (issue
+// 1726: no base, branch, title, URLs or review record) from an announcement or an edit of that pull request, and then
+// does what a first announcement does: the older heads of the pull request are superseded, and the delivery takes
+// their place in a group or joins the group its branch makes. Nothing for a delivery that holds its base. Under the lock.
+func (h *Holder) fillFromAnnouncement(d *Delivery, p PullEvent) {
+	if d.Base != "" || p.Base == "" || d.State.Final() || d.MergedAs != "" {
+		return
+	}
+	d.Title, d.Base, d.Branch, d.HTMLURL, d.CloneURL = p.Title, p.Base, p.Head, p.HTMLURL, p.CloneURL
+	d.Paths, d.PathsTruncated, d.Removed = p.Paths, p.PathsTruncated, p.Removed
+	d.ModuleDirs, d.ModuleDirsSaid = p.ModuleDirs, p.ModuleDirsSaid
+	d.Issues, d.After = IssuesIn(p.Title+"\n"+p.Body), AfterIn(p.Body)
+	d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body)
+	if p.BaseSHA != "" {
+		d.BaseSHA = p.BaseSHA
+	}
+	d.Number = p.Number
+	h.Logf("[mesh-delivery] %s: its pull request's announcement filled its base (%s), branch and title", d.ID, p.Base)
+	h.keep(d)
+	replaced := h.supersedeOlder(p)
+	if d.Group == "" && !h.takePlaceOf(replaced, d) {
+		h.joinGroup(d)
+	}
+	h.Poke()
+}
+
 // PullEdited takes a pull request's edit (novox/hq issue 1079): its description is read again for the review record, the
 // `after:` lines and the issues it names, exactly as an announcement reads them. An Approve ask still unanswered whose words
 // the new record changes is withdrawn and asked again with it read, without a word on the pull request: nothing was
@@ -458,6 +493,9 @@ func (h *Holder) PullEdited(p PullEvent) {
 		h.pullUpdated(p)
 		return
 	}
+	if d != nil && d.Number == p.Number {
+		h.fillFromAnnouncement(d, p) // a delivery made from its verdict, its head never heard (issue 1726)
+	}
 	if p.Body == "" {
 		return
 	}

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go

@@ -0,0 +1,126 @@
+package main
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+// novox/hq issue 1726: a delivery made from a check's verdict that arrived before its pull request's head was heard
+// (transition "its verdict arrived before its head was heard", as mesh-controller#419 at a47476252772 was) holds no base,
+// branch, title, URLs or review record. The ready pass skipped it in silence, so no Approve was ever asked, and the
+// announcement of the same head, heard later, filled none of it.
+
+const verdictFirstBody = "Fixes a bug.\n\nReviewed-by: reviewer@shanks at " + readyHead + "\n"
+
+// verdictFirst is the live shape: the verdict heard, the head not, and the delivery ready.
+func verdictFirst(t *testing.T) (*world, string) {
+	w := newWorld(t)
+	w.op.auto = false
+	id := IDOf("novox/app", readyHead)
+	w.h.Checked(judgedAt("novox/app", 419, readyHead, "pass", "t1t1t1t1t1t1"))
+	w.settleAll()
+	d := w.delivery(id)
+	if d.State != Ready || d.Base != "" || d.Branch != "" || d.Title != "" || d.HTMLURL != "" {
+		t.Fatalf("not the live shape: %s base %q branch %q title %q", d.State, d.Base, d.Branch, d.Title)
+	}
+	return w, id
+}
+
+func TestADeliveryWithoutItsHeadSaysWhatItWaitsForAndAsksNoOne(t *testing.T) {
+	w, id := verdictFirst(t)
+	if n := len(w.op.open()); n != 0 {
+		t.Fatalf("an ask was made for a delivery with no base: %d", n)
+	}
+	d := w.delivery(id)
+	if d.Ready == nil || !strings.Contains(d.Ready.Waits, "announcement") {
+		t.Fatalf("it does not say what it waits for: %+v", d.Ready)
+	}
+	shown, err := w.h.Show(id)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if waits, _ := shown.(map[string]any)["waits"].(string); !strings.Contains(waits, "announcement") {
+		t.Fatalf("show waits %q", waits)
+	}
+}
+
+func TestAnAnnouncementFillsADeliveryMadeFromItsVerdictAndTheOperatorIsAsked(t *testing.T) {
+	w, id := verdictFirst(t)
+	p := pr("novox/app", 419, readyHead, "fix/x", "src/a.go")
+	p.Title, p.Body, p.CloneURL = "Fix x", verdictFirstBody, "https://forge.invalid/novox/app.git"
+	w.h.PullUpdated(p)
+	w.settleAll()
+	d := w.delivery(id)
+	if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.HTMLURL == "" || d.CloneURL == "" ||
+		d.ReviewedBy != "reviewer@shanks" || len(d.Paths) != 1 {
+		t.Fatalf("the announcement did not fill the delivery: %+v", d)
+	}
+	if n := len(w.op.open()); n != 1 {
+		t.Fatalf("the operator was asked %d times; wanted once", n)
+	}
+	if d.Ready.Waits != "" && !strings.Contains(d.Ready.Waits, "Approve") {
+		t.Fatalf("still waits: %q", d.Ready.Waits)
+	}
+}
+
+func TestADeliveryWithoutItsHeadPastItsBoundIsStalled(t *testing.T) {
+	w, id := verdictFirst(t)
+	if s := w.h.Stalled(); len(s) != 0 {
+		t.Fatalf("stalled before its bound: %+v", s)
+	}
+	w.later(headHeardBound + time.Minute)
+	w.settleAll()
+	var found bool
+	for _, l := range w.h.Stalled() {
+		if l.ID == id && strings.Contains(l.Says, "announcement") {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("not stalled past its bound: %+v", w.h.Stalled())
+	}
+}
+
+// A head asked, then a push whose verdict is heard before its head: the announcement of the new head supersedes the old
+// one exactly as a first announcement does, so one Approve stands, for the new head.
+func TestAnAnnouncementFillingAVerdictFirstHeadSupersedesTheOlderHeadAndItsAsk(t *testing.T) {
+	w := newWorld(t)
+	w.op.auto = false
+	a, b := IDOf("novox/app", readyHead), IDOf("novox/app", pushed)
+	w.h.PullUpdated(pr("novox/app", 419, readyHead, "fix/x", "src/a.go"))
+	w.h.Checked(judgedAt("novox/app", 419, readyHead, "pass", "t1t1t1t1t1t1"))
+	w.settleAll()
+	if n := len(w.op.open()); n != 1 {
+		t.Fatalf("head A: %d asks open; wanted 1", n)
+	}
+	w.h.Checked(judgedAt("novox/app", 419, pushed, "pass", "t1t1t1t1t1t1"))
+	w.settleAll()
+	w.h.PullUpdated(pr("novox/app", 419, pushed, "fix/x", "src/a.go"))
+	w.settleAll()
+	if got := w.delivery(a); got.NewerHead != pushed || got.State != Superseded {
+		t.Fatalf("head A is %s with newer head %q; wanted superseded by %s", got.State, got.NewerHead, pushed)
+	}
+	if got := w.delivery(b); got.Base != "main" || got.Branch != "fix/x" {
+		t.Fatalf("head B not filled: %+v", got)
+	}
+	if n := len(w.op.open()); n != 1 {
+		t.Fatalf("%d asks open; wanted one, for the new head", n)
+	}
+}
+
+// A description edit is a natural trigger too: it fills a delivery made from its verdict.
+func TestAnEditOfThePullRequestFillsADeliveryMadeFromItsVerdict(t *testing.T) {
+	w, id := verdictFirst(t)
+	p := pr("novox/app", 419, readyHead, "fix/x")
+	p.Title, p.Body = "Fix x", verdictFirstBody
+	w.h.PullEdited(p)
+	w.settleAll()
+	d := w.delivery(id)
+	if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.ReviewedBy != "reviewer@shanks" {
+		t.Fatalf("the edit did not fill the delivery: %+v", d)
+	}
+	if n := len(w.op.open()); n != 1 {
+		t.Fatalf("the operator was asked %d times; wanted once", n)
+	}
+}

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go

@@ -41,6 +41,12 @@ const readyRefusedFor time.Duration = 0
 // groupRefusedFor is how long the merge tool may refuse a closed group's member before it is said as stalled: none (issue 1115).
 const groupRefusedFor time.Duration = 0
 
+// waitsForHead is what a delivery waits for when its verdict was heard and its pull request's head was not (issue 1726).
+const waitsForHead = "its pull request's announcement: its base, branch and title are not heard, so what it merges into is not known"
+
+// headHeardBound is how long a delivery may wait for its pull request's announcement before it is said as stalled.
+const headHeardBound = 10 * time.Minute
+
 // groupClosed is whether a group is closed: a member reached the trunk. Under the lock.
 func (h *Holder) groupClosed(id string) bool {
 	g := h.groups[id]
@@ -181,6 +187,18 @@ func (h *Holder) readyAsks(now time.Time) []func() {
 			}
 			continue
 		}
+		if d.Base == "" {
+			// A delivery made from its verdict before its head was heard (issue 1726): its base, branch and title are
+			// unknown, so what it merges into is too. It is neither asked nor passed over in silence: it waits for its
+			// pull request's announcement, which fills it (pullUpdated), and past headHeardBound it is stalled.
+			if r.Waits != waitsForHead {
+				r.Waits = waitsForHead
+				h.Logf("[mesh-delivery] %s waits: %s", d.ID, waitsForHead)
+				h.keep(d)
+			}
+			h.lookAt(r.Seen.Add(headHeardBound), "a delivery whose pull request was never announced")
+			continue
+		}
 		if d.Base != MergeBranch {
 			if r.OtherBase != d.Base {
 				r.OtherBase = d.Base
@@ -369,6 +387,10 @@ func (h *Holder) readyLines(now time.Time) []StalledLine {
 			continue
 		}
 		switch {
+		case d.Base == "" && d.Number != 0 && now.Sub(r.Seen) >= headHeardBound:
+			out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(),
+				Bound: headHeardBound.String(), H2: "none: a later announcement of the pull request's head (the merge tool announces it again, the forge's announcer restarts, or a new push) or an edit of its description; a recheck does not fill it — the operator's",
+				Says: fmt.Sprintf("%s#%d passed its checks at %s and waits for %s", d.Repository, d.Number, shortOf(r.Head, 12), waitsForHead)})
 		case r.Failed != "" && d.State == Rejected:
 			out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(),
 				Bound: "0s", H2: "none: a new head, or a recheck that passes — the operator's",

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go

@@ -42,6 +42,9 @@ func (h *Holder) waitsFor(d *Delivery) string {
 		if d.MergedAs != "" {
 			return "a walk that answers its merge"
 		}
+		if d.Base == "" && d.Number != 0 {
+			return waitsForHead
+		}
 		return "its merge"
 	case Rejected:
 		return "a new head, or a recheck"