From 9cfb5f9edb46044a9693bfeca884a43da5d5b13d Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:24:07 +0200 Subject: [PATCH 1/3] mesh-knowledge: keep when an owed review is due, say it in the history, and tell the operator once after its last try (hq issue 497) A failed review's try count was already kept with the version, but not when it was asked again, so a 10-minute pause read as a review lost to a restart; and after the last try only the history and the health said so. --- modules/mesh-knowledge/README.md | 7 +- .../cmd/mesh-knowledge/answers.go | 9 +- .../cmd/mesh-knowledge/operator.go | 9 +- .../cmd/mesh-knowledge/review.go | 89 ++++++++++++++++--- .../cmd/mesh-knowledge/review_owed_test.go | 88 ++++++++++++++++++ .../cmd/mesh-knowledge/store.go | 3 +- 6 files changed, 178 insertions(+), 27 deletions(-) create mode 100644 modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go diff --git a/modules/mesh-knowledge/README.md b/modules/mesh-knowledge/README.md index d3576f92..d3a217fd 100644 --- a/modules/mesh-knowledge/README.md +++ b/modules/mesh-knowledge/README.md @@ -104,8 +104,11 @@ as text. `get` serves each, and any other field a record of hq wrote under `fiel Its author revises it by proposing it again, and that version is reviewed again. - A judgement that cannot be had (the judge unreachable, or an answer that is not `approve`, or `changes` naming no finding) leaves the record proposed and says so on it (`get`'s `review`). It is never taken as an - approval. It is asked again after 10, then 20 minutes; after 3 tries the record says its author proposes it - again to have it reviewed again, and the module is unhealthy, naming it, so the mesh raises a condition. + approval. It is owed: its try count and when it is asked again (10, then 20 minutes later) are kept with the + version in the store and said in its history, so the background work takes it up at that time, also after a + restart of the tool runner (hq issue 497). After 3 tries the record says it was the last and its author + proposes it again to have it reviewed again; the operator is told once on the operator's channel, and the + module is unhealthy, naming it, so the mesh raises a condition. An ask of the operator that cannot be made counts as a try the same way. - A review whose outcome the database refused to keep is held back for that record alone (10, then 20 minutes, 3 tries, logged as given up), so it never blocks the records behind it. diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go b/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go index c32ed260..db2fe8fb 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go @@ -770,11 +770,6 @@ func reviewSays(r *Record) string { case JudgingUnproven: return "not reviewed: " + j.Reason } - s := fmt.Sprintf("not reviewed: %s (try %d of %d); it stays proposed, and no judgement that could not be had counts as an approval", j.Reason, j.Tries, MostReviewTries) - if j.Tries >= MostReviewTries { - s += "; its author proposes it again to have it reviewed again" - } else { - s += "; asked again after a pause" - } - return s + // When it is asked again is said, so a pause is not taken for a review lost (novox/hq issue 497). + return fmt.Sprintf("not reviewed: %s (%s; no judgement that could not be had counts as an approval", j.Reason, j.owedSays()) } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go b/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go index 11c5798c..bc0df747 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go @@ -141,13 +141,8 @@ func (k *KB) askOperator(ctx context.Context, held *Entry, w *Record, by string, } unasked := func(why string) (string, error) { w.Ask = nil - w.Judging = &Judging{State: JudgingFailed, Tries: tries + 1, Reason: why, At: k.now()} - said := fmt.Sprintf("approved by the knowledge base's review, and %s (try %d of %d); it stays proposed", why, tries+1, MostReviewTries) - if tries+1 >= MostReviewTries { - said += ", and its author proposes it again to have it reviewed again" - } else { - said += " and is reviewed again after a pause" - } + w.Judging = failedJudging(tries+1, why, k.now()) + said := fmt.Sprintf("approved by the knowledge base's review, and %s (%s", why, w.Judging.owedSays()) e, err := k.Store.Get(ctx, w.Key()) if err != nil || e == nil { return "", fmt.Errorf("%s: %s, and that could not be kept (%v)", w.Key(), said, err) diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go index 3b63fe73..ca452dbe 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go @@ -9,8 +9,11 @@ package main // - On changes the record stays proposed, the findings kept with the version they judged, and its author revises // it with a new version, which is reviewed again. // - A judgement that cannot be had — the judge unreachable, or its answer unreadable — leaves the record proposed -// and says so on it. It is never taken as an approval. It is asked again after a pause, up to MostReviewTries; -// after that the record says its author proposes it again to have it reviewed again. +// and says so on it. It is never taken as an approval. It is owed: the try count and when the next try is due +// are kept with the version in the store, so the background work takes it up at that time, also after a restart +// of the tool runner (novox/hq issue 497), up to MostReviewTries. After the last try the record says so, its +// author proposes it again to have it reviewed again, the module's health names it, and the operator is told +// once on the operator's channel. // - A record with a version whose writer the runtime did not prove is not reviewed: who wrote it cannot be told, // and no one but its proven author revises it, so its author proposes it again under a new number. // - On approve, a record that does not need the operator is accepted at once; one its author marked @@ -35,6 +38,41 @@ type Judging struct { Tries int `json:"tries,omitempty"` Reason string `json:"reason,omitempty"` At time.Time `json:"at,omitempty"` + // Due is when a failed review is asked again; zero after its last try. It is kept in the store with the + // version, so a restart of the tool runner neither loses the try nor asks it early (novox/hq issue 497). + Due time.Time `json:"due,omitempty"` +} + +// ReviewPause is the pause after a failed try of a review, times the tries so far. +const ReviewPause = 10 * time.Minute + +// failedJudging is a review that could not be had, the tries'th time: owed again at a time it keeps, unless that +// was its last try. +func failedJudging(tries int, reason string, now time.Time) *Judging { + j := &Judging{State: JudgingFailed, Tries: tries, Reason: reason, At: now} + if tries < MostReviewTries { + j.Due = now.Add(time.Duration(tries) * ReviewPause) + } + return j +} + +// due is when a failed review is asked again. A version kept before the time was kept (novox/hq issue 497) has +// none, and is due after the same pause from its last try. +func (j *Judging) due() time.Time { + if !j.Due.IsZero() { + return j.Due + } + return j.At.Add(time.Duration(j.Tries) * ReviewPause) +} + +// owedSays is what a failed try says of what follows it: when it is asked again, or that it was the last. +func (j *Judging) owedSays() string { + if j.Tries >= MostReviewTries { + return fmt.Sprintf("try %d of %d, the last); it stays proposed and nothing of it is in force, the operator is told, "+ + "and its author proposes it again to have it reviewed again", j.Tries, MostReviewTries) + } + return fmt.Sprintf("try %d of %d); it stays proposed, and it is asked again at %s", j.Tries, MostReviewTries, + j.due().UTC().Format("2006-01-02 15:04 UTC")) } // The states of Judging. @@ -162,7 +200,7 @@ func (k *KB) reviewDue(e *Entry) bool { case JudgingDue: return true case JudgingFailed: - return w.Judging.Tries < MostReviewTries && k.now().Sub(w.Judging.At) >= time.Duration(w.Judging.Tries)*10*time.Minute + return w.Judging.Tries < MostReviewTries && !k.now().Before(w.Judging.due()) } return false } @@ -218,6 +256,10 @@ func (k *KB) ReviewStep() (string, error) { return "", err } for _, e := range all { + if ranOutOfTries(e.Working()) { + k.tellUnreviewed(e) + continue + } if !k.reviewDue(e) || k.heldBack(e) { continue } @@ -240,13 +282,44 @@ func Unreviewed(all []*Entry) []string { var out []string for _, e := range all { w := e.Working() - if w.Status == StatusProposed && w.Judging != nil && w.Judging.State == JudgingFailed && w.Judging.Tries >= MostReviewTries { + if ranOutOfTries(w) { out = append(out, fmt.Sprintf("%s (%s)", w.Key(), w.Judging.Reason)) } } return out } +func ranOutOfTries(w *Record) bool { + return w.Status == StatusProposed && w.Judging != nil && w.Judging.State == JudgingFailed && w.Judging.Tries >= MostReviewTries +} + +// tellUnreviewed tells the operator, once for each version, that a proposal ran out of review tries: a history line +// alone is read by nobody, and a proposal never reviewed must not wait in silence (novox/hq issue 497). The store +// keeps that it was told, so a restart does not tell it again. A tell that fails is logged; the module's health +// still names the record, so the mesh's condition stands. +func (k *KB) tellUnreviewed(e *Entry) { + if k.Notify == nil { + return + } + w := e.Working() + version := e.CurrentVersion + if e.Pending != nil { + version = e.PendingVersion + } + first, err := k.Store.Told(context.Background(), fmt.Sprintf("unreviewed %s version %d", w.Key(), version)) + if err != nil || !first { + return + } + summary := fmt.Sprintf("%s was never reviewed: its review could not be had %d times (last: %s). It stays proposed, and nothing of it is in force.", + w.Key(), w.Judging.Tries, w.Judging.Reason) + explanation := "No review that could not be had counts as an approval. Its author proposes it again to have it reviewed again; " + + "until then the knowledge base's health names it. The knowledge base's get and history show it." + key := fmt.Sprintf("knowledge.unreviewed.%s-%d", w.Kind, w.Number) + if err := k.Notify(key, summary, "A proposal was never reviewed: "+w.Key().String(), explanation); err != nil { + k.logf("[mesh-knowledge] the operator could not be told that %s was never reviewed: %v; the module's health still names it", w.Key(), err) + } +} + // reviewOne asks the judgement on one record's working version, and acts on its verdict. The write lock is held // only to read and to write, never across the judgement; a record revised while it was judged is judged again. func (k *KB) reviewOne(key Key) (string, error) { @@ -312,12 +385,8 @@ func (k *KB) reviewOne(key Key) (string, error) { } if err != nil { // No verdict: the record stays proposed and says so. Never an approval. - w.Judging.State, w.Judging.Reason, w.Judging.At = JudgingFailed, err.Error(), k.now() - w.Judging.Tries++ - why := fmt.Sprintf("not reviewed: %v (try %d of %d); it stays proposed", err, w.Judging.Tries, MostReviewTries) - if w.Judging.Tries >= MostReviewTries { - why += ", and its author proposes it again to have it reviewed again" - } + w.Judging = failedJudging(w.Judging.Tries+1, err.Error(), k.now()) + why := fmt.Sprintf("not reviewed: %v (%s", err, w.Judging.owedSays()) by = ReviewBy if serr := save(w, why); serr != nil { return "", fmt.Errorf("%s: %s, and that could not be kept (%v)", key, why, serr) diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go new file mode 100644 index 00000000..75d326f0 --- /dev/null +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go @@ -0,0 +1,88 @@ +package main + +import ( + "context" + "fmt" + "strings" + "testing" + "time" +) + +// novox/hq issue 497: a review that could not be had is owed. What is owed is kept in the knowledge base's store, +// with its try count and when its next try is due, so the background work takes it up after a restart of the tool +// runner as it would have without one; the history says when it is asked again, so nobody takes a pause for a +// loss; and after its last try the history says so and the operator is told once. It is never an approval. +func TestAReviewOwedOutlivesARestartAndItsLastTryIsSaid(t *testing.T) { + eachStore(t, func(t *testing.T, s Store) { + w := imported(t, s) + n := proposed(t, w, "author@g14", false) + key := Key{KindDecision, n} + lastWhy := func() string { + vs, err := s.Versions(context.Background(), key) + if err != nil || len(vs) == 0 { + t.Fatalf("versions of %s: %v", key, err) + } + return vs[len(vs)-1].Why + } + var due time.Time + for try := 1; try <= MostReviewTries; try++ { + if try > 1 { + // The tool runner restarts: a new knowledge base over the same store, nothing of the old one's memory. + w = newWorld(t, s, fixture()) + w.clock = due.Add(-time.Second) + if said := w.reviewAll(t); len(said) != 0 { + t.Fatalf("try %d asked before it was due: %v", try, said) + } + w.clock = due + } + w.judge.reviewFail = true + if said := w.reviewAll(t); len(said) != 1 || !strings.Contains(said[0], "not reviewed") { + t.Fatalf("try %d after a restart was not taken up: %v", try, said) + } + e := mustGet(t, s, key) + if e.Current.Status != StatusProposed || e.Current.Judging == nil || e.Current.Judging.State != JudgingFailed || + e.Current.Judging.Tries != try || len(e.Current.Reviews) != 0 { + t.Fatalf("try %d is not counted in the store: %s, %+v", try, e.Current.Status, e.Current.Judging) + } + why := lastWhy() + if !strings.Contains(why, fmt.Sprintf("try %d of %d", try, MostReviewTries)) { + t.Fatalf("try %d: the history does not count it: %q", try, why) + } + if try < MostReviewTries { + due = w.clock.Add(time.Duration(try) * 10 * time.Minute) + when := "asked again at " + due.Format("2006-01-02 15:04 UTC") + if !strings.Contains(why, when) { + t.Fatalf("try %d: the history does not say when it is asked again (%q): %q", try, when, why) + } + if g, _ := w.k.Get(GetAsk{Number: n}); !strings.Contains(g.Review, when) { + t.Fatalf("try %d: get does not say when it is asked again (%q): %q", try, when, g.Review) + } + if len(w.told) != 0 { + t.Fatalf("try %d: the operator was told before the last try: %v", try, w.told) + } + } + } + // After its last try: the history says it was the last, and the operator is told, once. + if why := lastWhy(); !strings.Contains(why, "the last") || !strings.Contains(why, "the operator is told") { + t.Fatalf("the last try is not said in the history: %q", why) + } + if len(w.told) != 1 || !strings.Contains(w.told[0], key.String()) || !strings.Contains(w.told[0], "never reviewed") { + t.Fatalf("the operator is not told once of the last try: %v", w.told) + } + w.clock = w.clock.Add(24 * time.Hour) + if said := w.reviewAll(t); len(said) != 0 || len(w.told) != 1 { + t.Fatalf("after the last try: asked %v, told %v", said, w.told) + } + // Another restart neither asks it again nor tells the operator again. + clock := w.clock + w = newWorld(t, s, fixture()) + w.clock = clock + if said := w.reviewAll(t); len(said) != 0 || len(w.told) != 0 { + t.Fatalf("after a restart past the last try: asked %v, told %v", said, w.told) + } + // Never an approval. + if e := mustGet(t, s, key); e.Current.Status != StatusProposed || len(w.said.of(EventAccepted)) != 0 { + t.Fatalf("a review never had approved it: %s", e.Current.Status) + } + }) +} diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/store.go b/modules/mesh-knowledge/cmd/mesh-knowledge/store.go index bbba2dbe..e64b7d0e 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/store.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/store.go @@ -63,7 +63,8 @@ type Store interface { // SetFindings replaces the whole-set findings; Findings reads them. SetFindings(ctx context.Context, f []Finding) error Findings(ctx context.Context) ([]Finding, error) - // Told records that the operator was told about a topic's clauses, once; false when it was already. + // Told records that the operator was told of something, once, by its key: a topic's clauses drafted, or a + // version that ran out of review tries (novox/hq issue 497); false when it was already. Told(ctx context.Context, topic string) (bool, error) } From d45761678a2ac91ea981ed97d861c6e22df18188 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:36:14 +0200 Subject: [PATCH 2/3] mesh-knowledge: clear a never-reviewed notice once the record moves on, key it by version and run of tries, say times locally (review of #219) A notice left open kept telling the operator a record was proposed and not in force after it was proposed again or accepted, and a later version's notice merged into it. --- modules/mesh-knowledge/README.md | 5 +- .../mesh-knowledge/cmd/mesh-knowledge/kb.go | 4 + .../cmd/mesh-knowledge/kit_test.go | 13 ++- .../mesh-knowledge/cmd/mesh-knowledge/main.go | 23 +++++ .../cmd/mesh-knowledge/pgstore.go | 23 +++++ .../cmd/mesh-knowledge/review.go | 90 +++++++++++++++++-- .../cmd/mesh-knowledge/review_owed_test.go | 64 ++++++++++++- .../cmd/mesh-knowledge/store.go | 23 +++++ 8 files changed, 231 insertions(+), 14 deletions(-) diff --git a/modules/mesh-knowledge/README.md b/modules/mesh-knowledge/README.md index d3a217fd..4657b39c 100644 --- a/modules/mesh-knowledge/README.md +++ b/modules/mesh-knowledge/README.md @@ -107,8 +107,9 @@ as text. `get` serves each, and any other field a record of hq wrote under `fiel approval. It is owed: its try count and when it is asked again (10, then 20 minutes later) are kept with the version in the store and said in its history, so the background work takes it up at that time, also after a restart of the tool runner (hq issue 497). After 3 tries the record says it was the last and its author - proposes it again to have it reviewed again; the operator is told once on the operator's channel, and the - module is unhealthy, naming it, so the mesh raises a condition. + proposes it again to have it reviewed again; the operator is told once on the operator's channel (keyed by + record and version, and cleared once the record is proposed again, accepted, withdrawn or superseded), and + the module is unhealthy, naming it, so the mesh raises a condition. An ask of the operator that cannot be made counts as a try the same way. - A review whose outcome the database refused to keep is held back for that record alone (10, then 20 minutes, 3 tries, logged as given up), so it never blocks the records behind it. diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go b/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go index 64b12530..a99f8747 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go @@ -67,6 +67,9 @@ type Judgement func(question string, schema map[string]any, model string) (json. // Notifier tells the operator's channel something (operator-channel.notify). type Notifier func(key, summary, headline, explanation string) error +// Clearer tells the operator's channel that what it was told under key is over, in one resolved line. +type Clearer func(key, resolved string) error + // KB is the knowledge base. type KB struct { Store Store @@ -75,6 +78,7 @@ type KB struct { Judge Judgement Asks AskBus Notify Notifier + Clear Clearer Emit func(event string, body any) error Settings func() (Settings, error) Now func() time.Time diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go index 944b54bb..5247479a 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go @@ -397,7 +397,10 @@ type world struct { router *router said *said told []string - clock time.Time + // toldKeys are the keys told under, and cleared what was cleared: key, then its resolved line. + toldKeys []string + cleared [][2]string + clock time.Time } func newWorld(t *testing.T, s Store, records Records) *world { @@ -406,8 +409,12 @@ func newWorld(t *testing.T, s Store, records Records) *world { clock: time.Date(2026, 10, 11, 9, 0, 0, 0, time.UTC)} w.k = &KB{Store: s, Records: records, Reserve: w.tracker.reserve, Judge: w.judge.judge, Asks: w.router, Emit: w.said.emit, Settings: func() (Settings, error) { return Settings{}, nil }, Now: func() time.Time { return w.clock }, - Logf: func(f string, a ...any) { t.Logf(f, a...) }, - Notify: func(key, summary, headline, explanation string) error { w.told = append(w.told, summary); return nil }} + Logf: func(f string, a ...any) { t.Logf(f, a...) }, + Notify: func(key, summary, headline, explanation string) error { + w.told, w.toldKeys = append(w.told, summary), append(w.toldKeys, key) + return nil + }, + Clear: func(key, resolved string) error { w.cleared = append(w.cleared, [2]string{key, resolved}); return nil }} return w } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/main.go b/modules/mesh-knowledge/cmd/mesh-knowledge/main.go index dd33011b..353db2b0 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/main.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/main.go @@ -87,6 +87,7 @@ func newKB(store Store) *KB { Judge: busJudgement(os.Getenv("MESH_NODE")), Asks: sdkAskBus{}, Notify: busNotifier, + Clear: busClearer, Emit: func(event string, body any) error { return stdio.Emit(event, body) }, Settings: func() (Settings, error) { return readSettings(file) }, Logf: logf, @@ -230,6 +231,14 @@ func busNotifier(key, summary, headline, explanation string) error { return err } +// busClearer tells the operator's channel that what it was told under key is over: the message is edited to say +// resolved (novox/hq issue 497). +func busClearer(key, resolved string) error { + _, err := stdio.Ask("seat:operator-channel.notify", map[string]any{"key": key, "subject": "the knowledge base", + "clear": true, "resolved": resolved}) + return err +} + // ---- the store, opened when it is first needed ------------------------------------------------------------ // lazyStore opens the database on the first call, and again after a failure, so a database that arrives late is @@ -329,6 +338,20 @@ func (l *lazyStore) Told(ctx context.Context, topic string) (bool, error) { } return s.Told(ctx, topic) } +func (l *lazyStore) ToldOf(ctx context.Context, prefix string) ([]string, error) { + s, err := l.store(ctx) + if err != nil { + return nil, err + } + return s.ToldOf(ctx, prefix) +} +func (l *lazyStore) Untold(ctx context.Context, key string) error { + s, err := l.store(ctx) + if err != nil { + return err + } + return s.Untold(ctx, key) +} // ---- prepare, check, dry-run -------------------------------------------------------------------------------- diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/pgstore.go b/modules/mesh-knowledge/cmd/mesh-knowledge/pgstore.go index c5c55204..6f141c8f 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/pgstore.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/pgstore.go @@ -549,6 +549,29 @@ func (s *PgStore) Findings(ctx context.Context) ([]Finding, error) { return out, rows.Err() } +func (s *PgStore) ToldOf(ctx context.Context, prefix string) ([]string, error) { + // left() and not like: a prefix may hold an underscore, which like would take for any character. + rows, err := s.pool.Query(ctx, `select topic from told where left(topic, length($1)) = $1 order by topic`, prefix) + if err != nil { + return nil, err + } + defer rows.Close() + var out []string + for rows.Next() { + var k string + if err := rows.Scan(&k); err != nil { + return nil, err + } + out = append(out, k) + } + return out, rows.Err() +} + +func (s *PgStore) Untold(ctx context.Context, key string) error { + _, err := s.pool.Exec(ctx, `delete from told where topic = $1`, key) + return err +} + func (s *PgStore) Told(ctx context.Context, topic string) (bool, error) { tag, err := s.pool.Exec(ctx, `insert into told (topic) values ($1) on conflict do nothing`, topic) if err != nil { diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go index ca452dbe..f6479d2d 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go @@ -72,7 +72,7 @@ func (j *Judging) owedSays() string { "and its author proposes it again to have it reviewed again", j.Tries, MostReviewTries) } return fmt.Sprintf("try %d of %d); it stays proposed, and it is asked again at %s", j.Tries, MostReviewTries, - j.due().UTC().Format("2006-01-02 15:04 UTC")) + j.due().Local().Format("2006-01-02 15:04 MST")) } // The states of Judging. @@ -255,6 +255,7 @@ func (k *KB) ReviewStep() (string, error) { if err != nil { return "", err } + k.clearUnreviewed(all) for _, e := range all { if ranOutOfTries(e.Working()) { k.tellUnreviewed(e) @@ -293,33 +294,106 @@ func ranOutOfTries(w *Record) bool { return w.Status == StatusProposed && w.Judging != nil && w.Judging.State == JudgingFailed && w.Judging.Tries >= MostReviewTries } -// tellUnreviewed tells the operator, once for each version, that a proposal ran out of review tries: a history line -// alone is read by nobody, and a proposal never reviewed must not wait in silence (novox/hq issue 497). The store -// keeps that it was told, so a restart does not tell it again. A tell that fails is logged; the module's health -// still names the record, so the mesh's condition stands. +// unreviewedPrefix starts the key of every notice of a proposal that ran out of review tries. The notice's key is +// unreviewedPrefix, the record and the version that recorded its last try (knowledge.unreviewed.decision-294.v9), so +// a later version's notice is its own; what the store keeps as told is that key, a space and the last failed try's +// time, which names the run of tries whatever versions are written after it (a link change writes one). +const unreviewedPrefix = "knowledge.unreviewed." + +func unreviewedOf(k Key) string { return fmt.Sprintf("%s%s-%d.", unreviewedPrefix, k.Kind, k.Number) } + +// tellUnreviewed tells the operator, once for each run of tries, that a proposal ran out of review tries: a history +// line alone is read by nobody, and a proposal never reviewed must not wait in silence (novox/hq issue 497). func (k *KB) tellUnreviewed(e *Entry) { if k.Notify == nil { return } + ctx := context.Background() w := e.Working() + at := w.Judging.At.UTC().Format(time.RFC3339Nano) + told, err := k.Store.ToldOf(ctx, unreviewedOf(w.Key())) + if err != nil { + k.logf("[mesh-knowledge] whether the operator was told that %s was never reviewed cannot be read: %v", w.Key(), err) + return + } + for _, t := range told { + if strings.HasSuffix(t, " "+at) { + return + } + } version := e.CurrentVersion if e.Pending != nil { version = e.PendingVersion } - first, err := k.Store.Told(context.Background(), fmt.Sprintf("unreviewed %s version %d", w.Key(), version)) - if err != nil || !first { + key := fmt.Sprintf("%sv%d", unreviewedOf(w.Key()), version) + // Kept as told before the notice is sent, as for drafted clauses: a notice that fails is logged and not sent + // again, and the module's health still names the record, so the mesh's condition stands for it. + if first, err := k.Store.Told(ctx, key+" "+at); err != nil || !first { return } summary := fmt.Sprintf("%s was never reviewed: its review could not be had %d times (last: %s). It stays proposed, and nothing of it is in force.", w.Key(), w.Judging.Tries, w.Judging.Reason) explanation := "No review that could not be had counts as an approval. Its author proposes it again to have it reviewed again; " + "until then the knowledge base's health names it. The knowledge base's get and history show it." - key := fmt.Sprintf("knowledge.unreviewed.%s-%d", w.Kind, w.Number) if err := k.Notify(key, summary, "A proposal was never reviewed: "+w.Key().String(), explanation); err != nil { k.logf("[mesh-knowledge] the operator could not be told that %s was never reviewed: %v; the module's health still names it", w.Key(), err) } } +// clearUnreviewed clears every notice of a proposal that ran out of review tries once the record has left that +// state — proposed again, accepted, withdrawn or superseded — so the operator's message never keeps saying it waits +// (novox/hq issue 497). A clear that fails is kept as told and sent again at the next step. +func (k *KB) clearUnreviewed(all []*Entry) { + if k.Clear == nil { + return + } + ctx := context.Background() + told, err := k.Store.ToldOf(ctx, unreviewedPrefix) + if err != nil { + k.logf("[mesh-knowledge] the notices of proposals never reviewed cannot be read: %v", err) + return + } + byKey := map[string]*Entry{} + for _, e := range all { + byKey[unreviewedOf(e.Current.Key())] = e + } + for _, t := range told { + key, at, ok := strings.Cut(t, " ") + if !ok { + continue + } + var e *Entry + for prefix, x := range byKey { + if strings.HasPrefix(key, prefix) { + e = x + break + } + } + resolved := strings.TrimPrefix(key, unreviewedPrefix) + " is no longer held by the knowledge base" + if e != nil { + w := e.Working() + if ranOutOfTries(w) && w.Judging.At.UTC().Format(time.RFC3339Nano) == at { + continue + } + switch { + case w.Status == StatusProposed: + resolved = w.Key().String() + " was proposed again and is reviewed again" + case w.Accepted(): + resolved = w.Key().String() + " was reviewed and is " + w.Status + default: + resolved = w.Key().String() + " no longer waits for its review: it is " + w.Status + } + } + if err := k.Clear(key, resolved); err != nil { + k.logf("[mesh-knowledge] the notice that %s was never reviewed could not be cleared: %v; it is cleared again at the next step", key, err) + continue + } + if err := k.Store.Untold(ctx, t); err != nil { + k.logf("[mesh-knowledge] the notice %s was cleared, and that could not be kept: %v", key, err) + } + } +} + // reviewOne asks the judgement on one record's working version, and acts on its verdict. The write lock is held // only to read and to write, never across the judgement; a record revised while it was judged is judged again. func (k *KB) reviewOne(key Key) (string, error) { diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go index 75d326f0..d07ce3ea 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go @@ -50,7 +50,7 @@ func TestAReviewOwedOutlivesARestartAndItsLastTryIsSaid(t *testing.T) { } if try < MostReviewTries { due = w.clock.Add(time.Duration(try) * 10 * time.Minute) - when := "asked again at " + due.Format("2006-01-02 15:04 UTC") + when := "asked again at " + due.Local().Format("2006-01-02 15:04 MST") if !strings.Contains(why, when) { t.Fatalf("try %d: the history does not say when it is asked again (%q): %q", try, when, why) } @@ -86,3 +86,65 @@ func TestAReviewOwedOutlivesARestartAndItsLastTryIsSaid(t *testing.T) { } }) } + +// novox/hq issue 497: the notice of a proposal never reviewed says it stays proposed, so it is cleared once that is +// no longer so; and a later version's run of tries is a notice of its own, told once whatever else is written. +func TestProposedAgainAfterTheLastTryClearsTheNotice(t *testing.T) { + eachStore(t, func(t *testing.T, s Store) { + w := imported(t, s) + n := proposed(t, w, "author@g14", false) + key := Key{KindDecision, n} + runOut := func(from int) { + t.Helper() + w.judge.reviewFail = true + for try := from; try <= MostReviewTries; try++ { + if said := w.reviewAll(t); len(said) != 1 { + t.Fatalf("try %d: %v", try, said) + } + w.clock = w.clock.Add(time.Duration(try) * ReviewPause) + } + } + runOut(1) + e := mustGet(t, s, key) + first := fmt.Sprintf("knowledge.unreviewed.decision-%d.v%d", n, e.CurrentVersion) + if len(w.toldKeys) != 1 || w.toldKeys[0] != first || len(w.cleared) != 0 { + t.Fatalf("after the last try: told %v, cleared %v", w.toldKeys, w.cleared) + } + // Another version written while it waits (as a link change on it writes one) tells nothing again. + r := e.Current.Clone() + if err := s.Save(context.Background(), Version{By: "test", At: w.clock, Why: "a link changed", Record: r}, true, false, e.Revision); err != nil { + t.Fatal(err) + } + if w.reviewAll(t); len(w.toldKeys) != 1 || len(w.cleared) != 0 { + t.Fatalf("a version written while it waits: told %v, cleared %v", w.toldKeys, w.cleared) + } + // Proposed again: the notice is cleared, saying so. + if _, err := w.k.Propose("author@g14", valid(n)); err != nil { + t.Fatal(err) + } + w.clock = w.clock.Add(time.Second) + w.reviewAll(t) // its try 1, which fails, and the clearing + w.clock = w.clock.Add(ReviewPause) + if len(w.cleared) != 1 || w.cleared[0][0] != first || !strings.Contains(w.cleared[0][1], "proposed again") { + t.Fatalf("proposed again: cleared %v", w.cleared) + } + // Its own run of tries out is a notice of its own, under its own version. + runOut(2) + if len(w.toldKeys) != 2 || w.toldKeys[1] == first || len(w.cleared) != 1 { + t.Fatalf("a later version ran out: told %v, cleared %v", w.toldKeys, w.cleared) + } + // Proposed again and accepted at last: that notice is cleared too, and nothing is cleared twice, also after + // a restart. + w.judge.reviewFail = false + w.accepted(t, "author@g14", valid(n)) + if len(w.cleared) != 2 || w.cleared[1][0] != w.toldKeys[1] || !strings.Contains(w.cleared[1][1], "proposed again") { + t.Fatalf("accepted: cleared %v", w.cleared) + } + clock := w.clock + w = newWorld(t, s, fixture()) + w.clock = clock + if w.reviewAll(t); len(w.cleared) != 0 || len(w.told) != 0 { + t.Fatalf("after a restart: told %v, cleared %v", w.told, w.cleared) + } + }) +} diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/store.go b/modules/mesh-knowledge/cmd/mesh-knowledge/store.go index e64b7d0e..9dcc30e1 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/store.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/store.go @@ -66,6 +66,9 @@ type Store interface { // Told records that the operator was told of something, once, by its key: a topic's clauses drafted, or a // version that ran out of review tries (novox/hq issue 497); false when it was already. Told(ctx context.Context, topic string) (bool, error) + // ToldOf is every key told of that starts with prefix; Untold takes one away once what it told of is over. + ToldOf(ctx context.Context, prefix string) ([]string, error) + Untold(ctx context.Context, key string) error } // memStore is the Store in memory. @@ -272,6 +275,26 @@ func (s *memStore) Findings(context.Context) ([]Finding, error) { return append([]Finding(nil), s.findings...), nil } +func (s *memStore) ToldOf(_ context.Context, prefix string) ([]string, error) { + s.mu.Lock() + defer s.mu.Unlock() + var out []string + for k := range s.told { + if strings.HasPrefix(k, prefix) { + out = append(out, k) + } + } + sort.Strings(out) + return out, nil +} + +func (s *memStore) Untold(_ context.Context, key string) error { + s.mu.Lock() + defer s.mu.Unlock() + delete(s.told, key) + return nil +} + func (s *memStore) Told(_ context.Context, topic string) (bool, error) { s.mu.Lock() defer s.mu.Unlock() From c394ecd9fdc8e70e83e84e7ac0880a0f4f082e1e Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:41:45 +0200 Subject: [PATCH 3/3] mesh-knowledge: give a never-reviewed notice its resolved line when raised, and test a refused clear and local time in a set zone (review of #219) The operator's channel edits a cleared message with the resolved line it was raised with, so one sent only with the clear was never shown. --- .../cmd/mesh-knowledge/draft.go | 2 +- .../mesh-knowledge/cmd/mesh-knowledge/kb.go | 5 +- .../cmd/mesh-knowledge/kit_test.go | 5 +- .../mesh-knowledge/cmd/mesh-knowledge/main.go | 10 ++-- .../cmd/mesh-knowledge/review.go | 4 +- .../cmd/mesh-knowledge/review_owed_test.go | 51 ++++++++++++++++++- 6 files changed, 67 insertions(+), 10 deletions(-) diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/draft.go b/modules/mesh-knowledge/cmd/mesh-knowledge/draft.go index 4bcbe82e..7e1bc2bb 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/draft.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/draft.go @@ -356,7 +356,7 @@ func (k *KB) tellTopic(ctx context.Context, topic string) { summary := fmt.Sprintf("The clauses of %d prose decisions on %s are drafted and checked: %d confirmed, %d still drafted.", records, topic, confirmed, drafted) explanation := "A second judgement confirmed each clause against its record; a clause still drafted is never served as in force. " + "You may send any record back with the knowledge base's return, and its clauses are drafted again. Nothing needs you otherwise." - if err := k.Notify("knowledge.clauses."+strings.ReplaceAll(topic, " ", "-"), summary, "Clauses drafted for "+topic, explanation); err != nil { + if err := k.Notify("knowledge.clauses."+strings.ReplaceAll(topic, " ", "-"), summary, "Clauses drafted for "+topic, explanation, ""); err != nil { k.logf("[mesh-knowledge] the operator could not be told about %s's clauses: %v", topic, err) } } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go b/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go index a99f8747..9e51e68d 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go @@ -64,8 +64,9 @@ type Reserver func(kind, title string) (int, error) // the structured answer and who judged. type Judgement func(question string, schema map[string]any, model string) (json.RawMessage, string, error) -// Notifier tells the operator's channel something (operator-channel.notify). -type Notifier func(key, summary, headline, explanation string) error +// Notifier tells the operator's channel something (operator-channel.notify). Resolved is the line its message says +// once it is cleared: the channel keeps the one given when it is raised (novox/hq issue 497); "" for none. +type Notifier func(key, summary, headline, explanation, resolved string) error // Clearer tells the operator's channel that what it was told under key is over, in one resolved line. type Clearer func(key, resolved string) error diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go index 5247479a..1d0ad616 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go @@ -400,6 +400,7 @@ type world struct { // toldKeys are the keys told under, and cleared what was cleared: key, then its resolved line. toldKeys []string cleared [][2]string + resolved []string // the resolved line given with each notice raised clock time.Time } @@ -410,8 +411,8 @@ func newWorld(t *testing.T, s Store, records Records) *world { w.k = &KB{Store: s, Records: records, Reserve: w.tracker.reserve, Judge: w.judge.judge, Asks: w.router, Emit: w.said.emit, Settings: func() (Settings, error) { return Settings{}, nil }, Now: func() time.Time { return w.clock }, Logf: func(f string, a ...any) { t.Logf(f, a...) }, - Notify: func(key, summary, headline, explanation string) error { - w.told, w.toldKeys = append(w.told, summary), append(w.toldKeys, key) + Notify: func(key, summary, headline, explanation, resolved string) error { + w.told, w.toldKeys, w.resolved = append(w.told, summary), append(w.toldKeys, key), append(w.resolved, resolved) return nil }, Clear: func(key, resolved string) error { w.cleared = append(w.cleared, [2]string{key, resolved}); return nil }} diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/main.go b/modules/mesh-knowledge/cmd/mesh-knowledge/main.go index 353db2b0..b2836798 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/main.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/main.go @@ -225,9 +225,13 @@ func busJudgement(node string) Judgement { } // busNotifier tells the operator's channel, as a warning: it is the one severity below urgent. -func busNotifier(key, summary, headline, explanation string) error { - _, err := stdio.Ask("seat:operator-channel.notify", map[string]any{"key": key, "subject": "the knowledge base", - "severity": "warning", "summary": summary, "headline": headline, "explanation": explanation}) +func busNotifier(key, summary, headline, explanation, resolved string) error { + body := map[string]any{"key": key, "subject": "the knowledge base", + "severity": "warning", "summary": summary, "headline": headline, "explanation": explanation} + if resolved != "" { + body["resolved"] = resolved + } + _, err := stdio.Ask("seat:operator-channel.notify", body) return err } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go index f6479d2d..4a58c4ce 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go @@ -335,7 +335,9 @@ func (k *KB) tellUnreviewed(e *Entry) { w.Key(), w.Judging.Tries, w.Judging.Reason) explanation := "No review that could not be had counts as an approval. Its author proposes it again to have it reviewed again; " + "until then the knowledge base's health names it. The knowledge base's get and history show it." - if err := k.Notify(key, summary, "A proposal was never reviewed: "+w.Key().String(), explanation); err != nil { + // The channel edits a cleared message with the resolved line given here, not the one sent with the clear. + resolved := w.Key().String() + " no longer waits for its review" + if err := k.Notify(key, summary, "A proposal was never reviewed: "+w.Key().String(), explanation, resolved); err != nil { k.logf("[mesh-knowledge] the operator could not be told that %s was never reviewed: %v; the module's health still names it", w.Key(), err) } } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go index d07ce3ea..c094a35f 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "strings" "testing" @@ -14,6 +15,7 @@ import ( // loss; and after its last try the history says so and the operator is told once. It is never an approval. func TestAReviewOwedOutlivesARestartAndItsLastTryIsSaid(t *testing.T) { eachStore(t, func(t *testing.T, s Store) { + inZone(t) w := imported(t, s) n := proposed(t, w, "author@g14", false) key := Key{KindDecision, n} @@ -51,6 +53,9 @@ func TestAReviewOwedOutlivesARestartAndItsLastTryIsSaid(t *testing.T) { if try < MostReviewTries { due = w.clock.Add(time.Duration(try) * 10 * time.Minute) when := "asked again at " + due.Local().Format("2006-01-02 15:04 MST") + if utc := due.UTC().Format("15:04"); strings.Contains(when, utc) || !strings.Contains(when, "CEST") { + t.Fatalf("the time is not said locally: %q (UTC %s)", when, utc) + } if !strings.Contains(why, when) { t.Fatalf("try %d: the history does not say when it is asked again (%q): %q", try, when, why) } @@ -107,7 +112,8 @@ func TestProposedAgainAfterTheLastTryClearsTheNotice(t *testing.T) { runOut(1) e := mustGet(t, s, key) first := fmt.Sprintf("knowledge.unreviewed.decision-%d.v%d", n, e.CurrentVersion) - if len(w.toldKeys) != 1 || w.toldKeys[0] != first || len(w.cleared) != 0 { + if len(w.toldKeys) != 1 || w.toldKeys[0] != first || len(w.cleared) != 0 || + w.resolved[0] != key.String()+" no longer waits for its review" { t.Fatalf("after the last try: told %v, cleared %v", w.toldKeys, w.cleared) } // Another version written while it waits (as a link change on it writes one) tells nothing again. @@ -148,3 +154,46 @@ func TestProposedAgainAfterTheLastTryClearsTheNotice(t *testing.T) { } }) } + +// inZone sets the local zone to one that is not UTC for one test, so a time said locally is proven to be, wherever +// the test runs. +func inZone(t *testing.T) { + t.Helper() + was := time.Local + time.Local = time.FixedZone("CEST", 2*60*60) + t.Cleanup(func() { time.Local = was }) +} + +// novox/hq issue 497: a clear the operator's channel refused keeps its notice as told, and is sent again at the next +// step until it is taken. +func TestAClearThatFailsIsSentAgain(t *testing.T) { + eachStore(t, func(t *testing.T, s Store) { + w := imported(t, s) + n := proposed(t, w, "author@g14", false) + w.judge.reviewFail = true + for try := 1; try <= MostReviewTries; try++ { + w.reviewAll(t) + w.clock = w.clock.Add(time.Duration(try) * ReviewPause) + } + if len(w.toldKeys) != 1 { + t.Fatalf("after the last try: told %v", w.toldKeys) + } + prefix := fmt.Sprintf("knowledge.unreviewed.decision-%d.", n) + refused := 0 + w.k.Clear = func(key, resolved string) error { refused++; return errors.New("the channel is down") } + if _, err := w.k.Propose("author@g14", valid(n)); err != nil { + t.Fatal(err) + } + w.reviewAll(t) + if told, err := s.ToldOf(context.Background(), prefix); err != nil || len(told) != 1 || refused == 0 { + t.Fatalf("a refused clear: told %v (%v), refused %d", told, err, refused) + } + w.k.Clear = func(key, resolved string) error { w.cleared = append(w.cleared, [2]string{key, resolved}); return nil } + w.clock = w.clock.Add(time.Second) + w.reviewAll(t) + if told, err := s.ToldOf(context.Background(), prefix); err != nil || len(told) != 0 || len(w.cleared) != 1 || + w.cleared[0][0] != w.toldKeys[0] { + t.Fatalf("the next step: told %v (%v), cleared %v", told, err, w.cleared) + } + }) +}