knowledge-base: keep a retraction from being undone by a later acceptance or a redraft

A record proposed before a retraction and accepted after it marked the
retracted clause changed or the retracted record superseded, serving it again;
and returning a drafted record numbered its clauses anew, bringing retracted
ones back in force or making a waiting Approve mark another clause. accept now
leaves what is retracted alone, Return refuses a record with a retraction, and
retract waits until a record's drafting is done.

modules/mesh-knowledge/README.md

@@ -27,7 +27,7 @@ operator, it is accepted only on the operator's Approve, given on a channel that
 | `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; refused for a record whose clauses another record changed or links by number |
 | `withdraw` | `number`, `kind`, `why`, `retraction` | a reserved or proposed record withdrawn, as a new version naming why; its number stays spent. A pending review is cleared and a waiting operator's ask is ended. Refused for a record already accepted, superseded or withdrawn. With `retraction` true, the retraction waiting on the record is withdrawn instead, and the record holds as it did |
-| `retract` | `number`, `kind`, `clauses`, `why` | an accepted or superseded record retracted whole, or the clauses listed, on the operator's Approve only (asked on the router like a record that needs the operator). Until then, and on Decline or expiry, nothing changes. A retracted record or clause no longer holds: `ask` and `holds` leave it out, `get` marks it with by whom, when, why and the warrant, `links` say so both ways, history keeps every version; what it changed or superseded stays so, marked retracted, and the whole-set rules still read its links |
+| `retract` | `number`, `kind`, `clauses`, `why` | an accepted or superseded record retracted whole, or the clauses listed, on the operator's Approve only (asked on the router like a record that needs the operator). Until then, and on Decline or expiry, nothing changes. A retracted record or clause no longer holds: `ask` and `holds` leave it out, `get` marks it with by whom, when, why and the warrant, `links` say so both ways, history keeps every version; what it changed or superseded stays so, marked retracted, and the whole-set rules still read its links. A record accepted after it does not supersede or change what is retracted; a record still being drafted is retracted only once its drafting is done, and a record with a retraction (waiting or approved) is never returned to be drafted again. The caller need not be proven by the runtime, as for `withdraw`: it is recorded, and the operator's Approve is what authorises the retraction |
 | `progress` | `of` (`mesh`, `design <n>`, `milestone <design>/<id>`, `issue <n>`, `decision <n>` or `ADR <n>`, `research <n>`), `detail` (`minimal` or `detailed`), `bound`, `since` (a window such as `60m`, at most `24h`) | how far it has come (hq issue 1947): each child done ✓, moving ▶, waiting ⏸ or stuck ✗ with when it last moved, worked out from the issue tracker's rows and mesh-delivery's deliveries and groups, never written (ADR 0424 §1). `minimal` is one line per milestone, for a report every five minutes (two or three bus calls); `detailed` the report's tree with each issue's pull requests, the groups holding them and what it depends on. A design's title and state are read through the records module, cached by hq's commit; its milestones and their issues are the issue tracker's milestone records (`issue-tracker.milestones`, issue 1781), asked once per answer; a design with none is refused, naming the seed (`issue-tracker.import` with `milestones`). With `since`, only what changed in the window under `of`, headed "N changes in the last 60 min": milestones moved (from what to what, from `issue-tracker.milestones` `one`), issues opened or moved from what to what (from `issue-tracker.changes`, the tracker's log), noted or linked (from `issue-tracker.get`), pull requests opened, checked, merged, held or closed (from `mesh-delivery.show`'s transitions and check). It keeps no state; its warnings say when the tracker cannot answer `changes` (a move is then said by the status it reached, not from what) and that mesh-delivery keeps no time a merge reached its machines |
 
 `ask`, `get`, `holds` and `history` stay within `bound` bytes (4096 unless asked for, at most 65536) and say what they cut

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

@@ -500,6 +500,16 @@ func (k *KB) Return(caller string, key Key, reason string) (map[string]any, erro
 		return nil, fmt.Errorf("the knowledge base holds no %s", key)
 	}
 	r := e.Current.Clone()
+	if r.Retracting != nil || len(r.Retractions) > 0 {
+		// Drafting again numbers the clauses anew and brings them back in force: a retraction waiting would mark
+		// another clause than the operator was shown, and one approved would be undone.
+		which := "approved"
+		if r.Retracting != nil {
+			which = "waiting"
+		}
+		return nil, fmt.Errorf("%s is not returned: it has a retraction %s, which drafting it again would undo or point at other "+
+			"clauses. Withdraw a waiting retraction first; a retracted record or clause stays so", key, which)
+	}
 	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)
 	}

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

@@ -490,6 +490,12 @@ func (k *KB) accept(ctx context.Context, held *Entry, w *Record, by, why, warran
 			k.logf("[mesh-knowledge] %s %s %s, which cannot be read now (%v): the whole-set rules will say so", r.Key(), l.Kind, to, err)
 			continue
 		}
+		if t.Current.Status == StatusRetracted {
+			// What the operator retracted stays retracted: a record proposed before it and accepted after does not
+			// bring it back as superseded or changed.
+			k.logf("[mesh-knowledge] %s %s %s, which is retracted: it stays retracted", r.Key(), l.Kind, to)
+			continue
+		}
 		c := t.Current.Clone()
 		var what string
 		switch l.Kind {
@@ -497,6 +503,9 @@ func (k *KB) accept(ctx context.Context, held *Entry, w *Record, by, why, warran
 			c.Status = StatusSuperseded
 			c.Extra = addExtra(c.Extra, "superseded-by", r.Key().String())
 			for i := range c.Clauses {
+				if c.Clauses[i].Status == ClauseRetracted {
+					continue
+				}
 				c.Clauses[i].Status = ClauseSuperseded
 				c.Clauses[i].ChangedBy = append(c.Clauses[i].ChangedBy, r.Key().String())
 			}
@@ -506,6 +515,10 @@ func (k *KB) accept(ctx context.Context, held *Entry, w *Record, by, why, warran
 			if cl == nil || cl.Status == ClauseSuperseded {
 				continue
 			}
+			if cl.Status == ClauseRetracted {
+				k.logf("[mesh-knowledge] %s changes %s §%d, which is retracted: it stays retracted", r.Key(), to, l.To.Clause)
+				continue
+			}
 			cl.Status = ClauseChanged
 			from := r.Key().String()
 			if l.From > 0 {

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

@@ -97,6 +97,9 @@ func (k *KB) Retract(caller string, key Key, clauses []int, why string) (*Retrac
 		return nil, fmt.Errorf("%s is %s and takes no retraction; nothing was asked", key, cur.Status)
 	case cur.Retracting != nil:
 		return nil, fmt.Errorf("%s has a retraction waiting for the operator (%s): withdraw it first. Nothing was asked", key, cur.Retracting.Says())
+	case cur.Drafting != nil && cur.Drafting.State != DraftDone:
+		return nil, fmt.Errorf("%s's clauses are being drafted (%s): retract it once its drafting is done, so the clauses keep the "+
+			"numbers the operator is shown. Nothing was asked", key, cur.Drafting.State)
 	case e.Pending != nil:
 		return nil, fmt.Errorf("%s has version %d waiting for its review: let it be decided first. Nothing was asked", key, e.PendingVersion)
 	}

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

@@ -133,12 +133,17 @@ func TestARetractionTakesEffectOnlyOnTheOperatorsApprove(t *testing.T) {
 		name    string
 		clauses []int
 		answer  string // approve, decline, or withdraw (the pending retraction withdrawn, then approved too late)
+		// then is a record proposed before the retraction and accepted after it, changing §1 or superseding the record:
+		// its acceptance never brings back what was retracted.
+		then string
 	}
 	for _, c := range []tc{
-		{"the record whole, approved", nil, OptionApprove},
-		{"one clause, approved", []int{1}, OptionApprove},
-		{"the record whole, declined", nil, OptionDecline},
-		{"one clause, withdrawn before the answer", []int{1}, "withdraw"},
+		{"the record whole, approved", nil, OptionApprove, ""},
+		{"one clause, approved", []int{1}, OptionApprove, ""},
+		{"the record whole, declined", nil, OptionDecline, ""},
+		{"one clause, withdrawn before the answer", []int{1}, "withdraw", ""},
+		{"one clause, then a change of it proposed before and accepted after", []int{1}, OptionApprove, LinkChanges},
+		{"the record whole, then a supersession proposed before and accepted after", nil, OptionApprove, LinkSupersedes},
 	} {
 		t.Run(c.name, func(t *testing.T) {
 			eachStore(t, func(t *testing.T, s Store) {
@@ -149,6 +154,18 @@ func TestARetractionTakesEffectOnlyOnTheOperatorsApprove(t *testing.T) {
 				w.accepted(t, "author@test", valid(n))
 				w.k.judgeSet(ctx)
 				findings, _ := w.k.Store.Findings(ctx)
+				later := 0
+				if c.then != "" {
+					later = w.reserved(t, "author@test")
+					p := valid(later)
+					p.Links = []LinkAsk{{Kind: LinkExtends, To: itoa4(n)}, {Kind: LinkChanges, To: itoa4(n) + " §1", From: 1}}
+					if c.then == LinkSupersedes {
+						p.Links = []LinkAsk{{Kind: LinkSupersedes, To: itoa4(n)}}
+					}
+					if _, err := w.k.Propose("author@test", p); err != nil {
+						t.Fatal(err)
+					}
+				}
 				before := mustGet(t, w.k.Store, key)
 				asked := len(w.router.asked)
 
@@ -198,6 +215,21 @@ func TestARetractionTakesEffectOnlyOnTheOperatorsApprove(t *testing.T) {
 				if _, err := w.k.Reconcile(); err != nil {
 					t.Fatal(err)
 				}
+				if c.then != "" {
+					w.reviewAll(t)
+					if l := mustGet(t, w.k.Store, Key{KindDecision, later}).Current; !l.Accepted() {
+						t.Fatalf("the later record was not accepted: %s", l.Status)
+					}
+					r := mustGet(t, w.k.Store, key).Current
+					h, _ := w.k.Holds("", &key, 0)
+					// Whole: retracted, nothing held. One clause: §1 retracted, and §2 the only one held.
+					whole := c.clauses == nil
+					if (whole && (r.Status != StatusRetracted || len(h.Clauses) != 0)) ||
+						(!whole && (r.Clause(1).Status != ClauseRetracted || len(h.Clauses) != 1)) {
+						t.Fatalf("an acceptance brought back what was retracted: %s, %+v, holds %+v", r.Status, r.Clauses, h.Clauses)
+					}
+					return
+				}
 				e := mustGet(t, w.k.Store, key)
 				vs, _ := w.k.Store.Versions(ctx, key)
 				if len(vs) <= before.Versions || e.Current.Retracting != nil {
@@ -303,5 +335,30 @@ func TestARetractionTakesEffectOnlyOnTheOperatorsApprove(t *testing.T) {
 		if _, err := w.k.WithdrawRetraction("lead@shanks", key, "nothing waits"); err == nil {
 			t.Fatal("a retraction was withdrawn where none waits")
 		}
+		// A drafted record: retracted only once its drafting is done, and never returned to be drafted again while a
+		// retraction waits or after one, which would number its clauses anew and bring them back in force.
+		drafted := Key{KindDecision, 240}
+		draftUntil(t, w, drafted, DraftDrafted)
+		if _, err := w.k.Retract("lead@shanks", drafted, []int{1}, why); err == nil || !strings.Contains(err.Error(), "draft") {
+			t.Fatalf("a record still being drafted: %v", err)
+		}
+		draftUntil(t, w, drafted, DraftDone)
+		out, err := w.k.Retract("lead@shanks", drafted, []int{1}, why)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err := w.k.Return("operator@shanks", drafted, "redraft"); err == nil || !strings.Contains(err.Error(), "retract") {
+			t.Fatalf("returned while a retraction waits: %v", err)
+		}
+		w.router.answer(out.Ask, OptionApprove, w.clock)
+		if _, err := w.k.Reconcile(); err != nil {
+			t.Fatal(err)
+		}
+		if _, err := w.k.Return("operator@shanks", drafted, "redraft"); err == nil || !strings.Contains(err.Error(), "retract") {
+			t.Fatalf("returned after a retraction: %v", err)
+		}
+		if c := mustGet(t, w.k.Store, drafted).Current.Clause(1); c == nil || c.Status != ClauseRetracted {
+			t.Fatalf("§1: %+v", c)
+		}
 	})
 }