Merge pull request 'mesh-knowledge: a record may change a drafted clause, and a record whose clauses others link by number is never redrafted (hq issue 494, ADR 0321)' (#245) from fix/494-change-a-drafted-clause into main

modules/mesh-knowledge/README.md

@@ -23,7 +23,7 @@ operator, it is accepted only on the operator's Approve, given on a channel that
 | `reserve` | `kind`, `for` | the next decision or research number, asked of the issue tracker (`issue-tracker.reserve`); spent whether or not its record is accepted |
 | `propose` | the record whole | proposed, or refused naming every rule it breaks; an accepted record changes only by a progressive insight (`insight`). Each version proposed is then reviewed by the knowledge base itself (below) |
 | `import` | `dry-run`, `commit` | hq's decision records and research efforts at the commit the records module stands at, each whole at its own number |
-| `return` | `number`, `reason` | drafted clauses sent back to be drafted again |
+| `return` | `number`, `reason` | drafted clauses sent back to be drafted again; refused for a record whose clauses another record changed or links by number |
 
 `ask`, `get`, `holds` and `history` stay within `bound` bytes (4096 unless asked for, at most 65536) and say what they cut
 and how to read the rest. The module's own tool `knowledge_health` answers its health (below).
@@ -78,7 +78,7 @@ as text. `get` serves each, and any other field a record of hq wrote under `fiel
    - 25 on the structure: kind, reserved number, title, topic, date, deciders, research status, at least one
      clause, clause numbering, bold lead, clause size, the Context, Consequences and "How it is checked" sections,
      a research effort's overview, link kind, link target, link fit, no link to itself, what it extends, what it
-     supersedes or changes is accepted, a changed clause is in force, a superseded record cited beside its
+     supersedes or changes is accepted, a changed clause is in force or changed (a drafted one too), a superseded record cited beside its
      superseder, a graduated effort says what it became, and the links in its text resolve;
    - 4 on the prose: no word the glossary retired, no daemon's line, a deferral names its tracking issue, and a
      progressive insight in the dated, marked form.
@@ -178,6 +178,12 @@ the older clause marked `changed`. When every prose record of a topic has been t
 operator is told once (`operator-channel.notify`, a warning) with the counts. `return` sends a record back to be
 drafted again.
 
+A drafted clause may be changed by a later record (`changes 0281 §9`, novox/hq issue 494, ADR 0321): on its acceptance the
+clause is marked `changed` with the change named, keeps its number and stays `drafted`, so it is still never served
+as in force, and `get` says both. A redraft numbers a record's clauses anew, so `return` refuses a record any of
+whose clauses another record changed or links by number, served or proposed, naming each link: a link never comes
+to point at another clause.
+
 ## Where it keeps its records
 
 In a database in the store: the module requires a `postgres-database` provision named `knowledge`, as

modules/mesh-knowledge/cmd/mesh-knowledge/answers.go

@@ -70,6 +70,13 @@ func held(r *Record, c Clause) string {
 		if c.Unconfirmed != "" {
 			s += " (the second judgement did not confirm it: " + c.Unconfirmed + ")"
 		}
+		if len(c.ChangedBy) > 0 {
+			if c.Status == ClauseSuperseded {
+				s += "; superseded by " + strings.Join(c.ChangedBy, ", ")
+			} else {
+				s += "; changed by " + strings.Join(c.ChangedBy, ", ") + ": read both"
+			}
+		}
 		return s
 	case r.Status == StatusProposed || c.Status == ClauseProposed:
 		return "proposed: not in force"

modules/mesh-knowledge/cmd/mesh-knowledge/draft.go

@@ -503,6 +503,15 @@ func (k *KB) Return(caller string, key Key, reason string) (map[string]any, erro
 	if r.Drafting == nil {
 		return nil, fmt.Errorf("%s's clauses were written by its author, not drafted: a written clause is changed by a new record", key)
 	}
+	all, err := k.Store.All(ctx)
+	if err != nil {
+		return nil, err
+	}
+	if held := clauseHolds(r, all); len(held) > 0 {
+		return nil, fmt.Errorf("%s is not returned: drafting it again would number its clauses anew, and these hold them to their numbers: %s. "+
+			"A clause keeps its number once another record links it (novox/hq issue 494). To say a drafted clause is wrong "+
+			"or no longer holds, propose a record that changes it (changes %s §N)", key, strings.Join(held, "; "), strings.TrimPrefix(key.String(), "ADR "))
+	}
 	r.Clauses = nil
 	r.Drafting = &Drafting{State: DraftWaiting, Returned: reason, At: k.now()}
 	why := "returned to be drafted again: " + reason
@@ -512,3 +521,36 @@ func (k *KB) Return(caller string, key Key, reason string) (map[string]any, erro
 	k.emit(EventReturned, r, caller, why, e.Versions+1)
 	return map[string]any{"record": key.String(), "says": fmt.Sprintf("%s's clauses are taken back and will be drafted again, with your reason; until then it serves none.", key)}, nil
 }
+
+// clauseHolds is what holds a record's clauses to their numbers: each clause another record changed or superseded,
+// and each link of another record, served or proposed, that names one of its clauses. A redraft numbers the clauses
+// anew, so a record any of these name is never returned: its links would point at other clauses.
+func clauseHolds(r *Record, all []*Entry) []string {
+	var out []string
+	for _, c := range r.Clauses {
+		if len(c.ChangedBy) > 0 && (c.Status == ClauseChanged || c.Status == ClauseSuperseded) {
+			out = append(out, fmt.Sprintf("§%d is %s by %s", c.N, c.Status, strings.Join(c.ChangedBy, ", ")))
+		}
+	}
+	key := r.Key()
+	seen := map[string]bool{}
+	for _, e := range all {
+		for _, v := range []*Record{e.Current, e.Pending} {
+			if v == nil || v.Key() == key || v.Status == StatusWithdrawn {
+				continue
+			}
+			for _, l := range v.Links {
+				to, ok := l.To.Record()
+				if !ok || to != key || l.To.Clause == 0 {
+					continue
+				}
+				s := fmt.Sprintf("%s (%s) %s §%d", v.Key(), v.Status, l.Kind, l.To.Clause)
+				if !seen[s] {
+					seen[s] = true
+					out = append(out, s)
+				}
+			}
+		}
+	}
+	return out
+}

modules/mesh-knowledge/cmd/mesh-knowledge/issue494_test.go

@@ -0,0 +1,108 @@
+package main
+
+import (
+	"context"
+	"strings"
+	"testing"
+)
+
+// novox/hq issue 494: a record may change a clause the knowledge base holds only as drafted, and a record whose
+// clauses another record links by number is never drafted again, so no link ever points at another clause.
+
+// draftedNotConfirmed drafts 0240 and has the second judgement reject its first clause: it stays drafted.
+func draftedNotConfirmed(t *testing.T, w *world) Key {
+	t.Helper()
+	k := Key{KindDecision, 240}
+	draftUntil(t, w, k, DraftDrafted)
+	e := mustGet(t, w.k.Store, k)
+	r := e.Current.Clone()
+	r.Clauses[0].Text += " NOT EXACT"
+	if err := w.k.Store.Save(context.Background(), Version{By: "test", Why: "a draft the checker will reject", Record: r}, true, false, e.Revision); err != nil {
+		t.Fatal(err)
+	}
+	r = draftUntil(t, w, k, DraftDone)
+	if r.Clauses[0].Source != SourceDrafted || !r.Accepted() {
+		t.Fatalf("0240 §1 is not a drafted clause of an accepted record: %s %+v", r.Status, r.Clauses[0])
+	}
+	return k
+}
+
+func TestARecordMayChangeAClauseHeldOnlyAsDrafted(t *testing.T) {
+	eachStore(t, func(t *testing.T, s Store) {
+		w := imported(t, s)
+		k := draftedNotConfirmed(t, w)
+		n := w.reserved(t, "author@test")
+		p := valid(n)
+		p.Links = append(p.Links, LinkAsk{Kind: LinkChanges, To: "0240 §1", From: 2})
+		if _, err := w.k.Propose("author@test", p); err != nil {
+			t.Fatalf("a change of a drafted clause was refused: %v", err)
+		}
+		w.reviewAll(t)
+		if e := mustGet(t, s, Key{KindDecision, n}); !e.Current.Accepted() {
+			t.Fatalf("not accepted: %s", e.Current.Status)
+		}
+		// The drafted clause is changed by it, keeps its number and stays drafted: still never served as in force.
+		r := mustGet(t, s, k).Current
+		c := r.Clause(1)
+		want := itoa4(n) + " §2"
+		if c == nil || c.Status != ClauseChanged || c.Source != SourceDrafted || len(c.ChangedBy) != 1 || !strings.HasSuffix(c.ChangedBy[0], want) {
+			t.Fatalf("0240 §1 after the change: %+v", c)
+		}
+		if Served(r, *c) {
+			t.Fatal("a drafted clause that was changed is served as holding")
+		}
+		g, err := w.k.Get(GetAsk{Kind: KindDecision, Number: 240})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if h := g.Clauses[0].Holds; !strings.HasPrefix(h, "drafted, not confirmed") || !strings.Contains(h, "changed by ADR "+want) {
+			t.Fatalf("get says of 0240 §1: %q", h)
+		}
+		// The whole-set rules find nothing wrong with the change.
+		f, _ := s.Findings(context.Background())
+		for _, x := range f {
+			if x.Rule == "partial" {
+				t.Fatalf("a whole-set finding: %+v", x)
+			}
+		}
+	})
+}
+
+func TestARecordWhoseClausesAreLinkedByNumberIsNeverReturned(t *testing.T) {
+	eachStore(t, func(t *testing.T, s Store) {
+		w := imported(t, s)
+		k := draftedNotConfirmed(t, w)
+		// A proposed record cites 0240 §2: returning 0240 would point it at whatever is drafted second next time.
+		n := w.reserved(t, "author@test")
+		p := valid(n)
+		p.Links = append(p.Links, LinkAsk{Kind: LinkCites, To: "0240 §2", From: 1})
+		w.judge.review = `{"verdict":"changes","findings":["not yet"]}`
+		if _, err := w.k.Propose("author@test", p); err != nil {
+			t.Fatal(err)
+		}
+		w.reviewAll(t)
+		before := mustGet(t, s, k)
+		_, err := w.k.Return("operator@shanks", k, "clause 1 says more than the section")
+		if err == nil {
+			t.Fatal("a record a proposed record links by clause was returned")
+		}
+		if !strings.Contains(err.Error(), "ADR "+itoa4(n)+" (proposed) cites §2") || !strings.Contains(err.Error(), "changes 0240 §N") {
+			t.Fatalf("the refusal does not say what holds the clauses or the way forward: %v", err)
+		}
+		after := mustGet(t, s, k)
+		if after.Versions != before.Versions || len(after.Current.Clauses) != len(before.Current.Clauses) || len(w.said.of(EventReturned)) != 0 {
+			t.Fatalf("a refused return changed 0240: %d versions (was %d), %d clauses", after.Versions, before.Versions, len(after.Current.Clauses))
+		}
+
+		// A clause another record changed holds the record to its numbers too, with no link left to scan.
+		w2 := imported(t, newMemStore())
+		k2 := draftedNotConfirmed(t, w2)
+		n2 := w2.reserved(t, "author@test")
+		p2 := valid(n2)
+		p2.Links = append(p2.Links, LinkAsk{Kind: LinkChanges, To: "0240 §1", From: 2})
+		w2.accepted(t, "author@test", p2)
+		if _, err := w2.k.Return("operator@shanks", k2, "again"); err == nil || !strings.Contains(err.Error(), "§1 is changed by ADR "+itoa4(n2)+" §2") {
+			t.Fatalf("a record with a changed clause was returned, or the refusal does not name the change: %v", err)
+		}
+	})
+}

modules/mesh-knowledge/cmd/mesh-knowledge/main.go

@@ -1010,7 +1010,8 @@ func Tools(k *KB, caller func(map[string]any) string) []stdio.Tool {
 		{Name: Seat + ".return",
 			Description: "Send the clauses drafted from a prose decision back to be drafted again, with your reason, which the next " +
 				"drafting reads. Until they are drafted and confirmed again the record serves none. Refused: a record whose clauses its " +
-				"author wrote, an empty reason.",
+				"author wrote, an empty reason, and a record whose clauses another record changed or links by number (a redraft " +
+				"numbers them anew; a record that changes the drafted clause says it is wrong instead).",
 			Input: schema([]string{"number", "reason"}, map[string]any{"kind": kindArg(), "number": integer("the record's number"),
 				"reason": text("what is wrong with the drafted clauses")}),
 			Run: func(m map[string]any) (any, error) {

modules/mesh-knowledge/cmd/mesh-knowledge/rules.go

@@ -48,7 +48,7 @@ var PerRecordRules = []Rule{
 	{"self-link", "no link points at the record itself", "records.py supersession"},
 	{"rests-on", "a record it extends is accepted or proposed, never superseded or withdrawn", "records.py rests-on"},
 	{"supersedes-accepted", "a record it supersedes or changes is accepted", "records.py supersession, partial-supersession"},
-	{"changes-in-force", "a clause it changes is in force or changed, never superseded, proposed or drafted", "records.py partial-supersession"},
+	{"changes-in-force", "a clause it changes is in force or changed, never superseded or proposed; a drafted one may be changed", "records.py partial-supersession"},
 	{"live-citation", "a superseded record it cites is cited beside the record that replaced it", "records.py live-citation"},
 	{"became", "a graduated research effort names what it became", "cycle.py research"},
 	{"text-links", "every link its text writes to a decision record or research effort names one the knowledge base holds", "records.py links"},
@@ -261,8 +261,10 @@ func JudgeRecord(r *Record, held *Entry, lookup Lookup, ref *Reference) []Refusa
 			if !target.Accepted() || target.Status == StatusSuperseded {
 				refuse("supersedes-accepted", "%s changes %s, which is %s, not accepted", self, to, target.Status)
 			} else if c := target.Clause(l.To.Clause); c != nil {
-				if (c.Status != ClauseInForce && c.Status != ClauseChanged) || c.Source == SourceDrafted {
-					refuse("changes-in-force", "%s changes %s §%d, which is %s (%s)", self, to, l.To.Clause, c.Status, c.Source)
+				// A drafted clause may be changed (novox/hq issue 494, ADR 0321): the change is recorded against the clause the
+				// knowledge base holds, by its number, which nothing renumbers once another record links it (Return).
+				if c.Status != ClauseInForce && c.Status != ClauseChanged {
+					refuse("changes-in-force", "%s changes %s §%d, which is %s", self, to, l.To.Clause, c.Status)
 				}
 			}
 		case LinkCites: