diff --git a/modules/distribution/cmd/distribution-tools/contract_test.go b/modules/distribution/cmd/distribution-tools/contract_test.go index d2453f9ea..7bd5c1e22 100644 --- a/modules/distribution/cmd/distribution-tools/contract_test.go +++ b/modules/distribution/cmd/distribution-tools/contract_test.go @@ -57,7 +57,7 @@ func TestTheVerbsSpeakTheSeatsContract(t *testing.T) { t.Errorf("%s's body %s is not the contract's: %v", event, b, err) } } - // The caller list is read on each call: one that cannot be read answers no verb, and registry_callers says it as read. + // The caller list is read held: one never read answers no verb, and registry_callers says it as read. unread := func() (holder.Callers, error) { return nil, errors.New("config refused") } for _, tool := range tools(&holder.Verbs{}, kept[holder.Window]{}, unread) { if tool.Name == holder.ImageSeat+"."+holder.VerbVersions { diff --git a/modules/distribution/cmd/distribution-tools/main.go b/modules/distribution/cmd/distribution-tools/main.go index dc80fc85d..4270a2adb 100644 --- a/modules/distribution/cmd/distribution-tools/main.go +++ b/modules/distribution/cmd/distribution-tools/main.go @@ -248,7 +248,7 @@ func arg[T any](args map[string]any) (T, error) { func str(args map[string]any, k string) string { s, _ := args[k].(string); return strings.TrimSpace(s) } -// tools are distribution's tools over v; each call that checks a caller reads the caller list on that call (callers: +// tools are distribution's tools over v; each call that checks a caller reads the caller list held (callers: // the config value as mesh-config resolves it now), so an operator's change of it holds from the next call on. func tools(v *holder.Verbs, windows kept[holder.Window], callers func() (holder.Callers, error)) []stdio.Tool { now := func() (*holder.Verbs, error) { diff --git a/modules/distribution/internal/holder/callers.go b/modules/distribution/internal/holder/callers.go index b00894c82..b3021139e 100644 --- a/modules/distribution/internal/holder/callers.go +++ b/modules/distribution/internal/holder/callers.go @@ -93,11 +93,15 @@ func ParseCallers(raw string) (Callers, error) { return selfNotReleasing(c) } -// ConfiguredCallers is the config value `callers` as mesh-config resolved it for this machine, read through the tools -// runtime's stdio.Config on each call (never an environment variable: nothing renders a config value into a -// program's environment); unset is DefaultCallers, and Self on release's list is refused as ParseCallers refuses it. +// configured holds the config value `callers`: asked of the runtime once, by a watch, and replaced on each change of +// the value, so a call never asks (issue 1966: an ask that timed out refused every verb). +var configured = stdio.HoldVerbCallers(CallersConfig, VerbsOf(ImageSeat), DefaultCallers()) + +// ConfiguredCallers is the config value `callers` as mesh-config resolved it for this machine, held from the tools +// runtime's watch of it (never an environment variable: nothing renders a config value into a program's environment); +// unset is DefaultCallers, and Self on release's list is refused as ParseCallers refuses it. func ConfiguredCallers() (Callers, error) { - c, err := stdio.ConfiguredVerbCallers(CallersConfig, VerbsOf(ImageSeat), DefaultCallers()) + c, err := configured() if err != nil { return nil, err } diff --git a/modules/gitea/internal/packageseats/holder.go b/modules/gitea/internal/packageseats/holder.go index 244f1322d..ea56b67f0 100644 --- a/modules/gitea/internal/packageseats/holder.go +++ b/modules/gitea/internal/packageseats/holder.go @@ -77,10 +77,12 @@ func DefaultCallers() Callers { VerbVersions: {stdio.AnyCaller}} } -// ConfiguredCallers is the config value `callers` as mesh-config resolved it; unset is DefaultCallers. -func ConfiguredCallers() (Callers, error) { - return stdio.ConfiguredVerbCallers("callers", Verbs, DefaultCallers()) -} +// configured holds the config value `callers`: asked of the runtime once, by a watch, and replaced on each change of +// the value, so a call never asks (issue 1966). +var configured = stdio.HoldVerbCallers("callers", Verbs, DefaultCallers()) + +// ConfiguredCallers is the config value `callers` as mesh-config resolved it, held from its watch; unset is DefaultCallers. +func ConfiguredCallers() (Callers, error) { return configured() } // Publish is a publish as the seats' contract names it (mesh-contracts npmpackageregistry and gopackageregistry share // one shape): what the tools bundle reads from either seat's PublishInput. diff --git a/modules/mesh-build-go/cmd/mesh-build-go/fetch.go b/modules/mesh-build-go/cmd/mesh-build-go/fetch.go index 59a06ade8..1a17af061 100644 --- a/modules/mesh-build-go/cmd/mesh-build-go/fetch.go +++ b/modules/mesh-build-go/cmd/mesh-build-go/fetch.go @@ -334,7 +334,11 @@ func (m *Module) goEnv() []string { // The Go seat first, the public proxy second, the forge's git last (CAT-BG3, E11: direct goes in the first // change after mesh-sdk's and mesh-contracts' first numbered versions are in the Go seat); no GOPRIVATE, and // the checksum database off for the forge's paths, which go.sum and the builder's h1: digest guard instead. - env = append(env, "GOPROXY="+proxy+","+PublicGoProxy+",direct", "GONOSUMDB="+ForgeGoPaths, "GOPRIVATE=") + // The seat goes on to the public proxy only on a 404 or 410 (",": a package it does not hold), so a seat that + // fails otherwise (5xx, refused, timeout) fails the fetch loudly; the public proxy goes on to direct on any + // failure ("|"): a public proxy the node cannot reach stopped the fetch before direct, and direct reaches the + // forge's git for a package the seat does not hold (issue 1966, ADR 0605 §6). + env = append(env, "GOPROXY="+proxy+","+PublicGoProxy+"|direct", "GONOSUMDB="+ForgeGoPaths, "GOPRIVATE=") } else { env = append(env, "GOPRIVATE="+ForgeGoPaths) } diff --git a/modules/mesh-build-go/cmd/mesh-build-go/fetch_test.go b/modules/mesh-build-go/cmd/mesh-build-go/fetch_test.go index e01fd9b45..d76472a6f 100644 --- a/modules/mesh-build-go/cmd/mesh-build-go/fetch_test.go +++ b/modules/mesh-build-go/cmd/mesh-build-go/fetch_test.go @@ -153,7 +153,7 @@ func TestFetchAsksGitForTheSshRemoteAndDownloadsEachModule(t *testing.T) { } m.GoSeatFile = seat env := strings.Join(m.goEnv(), " ") - for _, want := range []string{"GOPROXY=http://forge.mesh:3000/api/packages/novox/go,https://proxy.golang.org,direct", "GONOSUMDB=git.novox.be", "GOPRIVATE= "} { + for _, want := range []string{"GOPROXY=http://forge.mesh:3000/api/packages/novox/go,https://proxy.golang.org|direct", "GONOSUMDB=git.novox.be", "GOPRIVATE= "} { if !strings.Contains(env+" ", want) { t.Errorf("with the Go seat bound the environment lacks %q: %s", want, env) } diff --git a/modules/mesh-build-go/cmd/mesh-build-go/versions.go b/modules/mesh-build-go/cmd/mesh-build-go/versions.go index 48eff74e5..a34b31747 100644 --- a/modules/mesh-build-go/cmd/mesh-build-go/versions.go +++ b/modules/mesh-build-go/cmd/mesh-build-go/versions.go @@ -96,7 +96,6 @@ func saidSuffix(said string) string { return ": " + said } -// takeVersion takes one asked version into dir's modfile (takeVersions). // downloadFailure is a failed download said in go's own words; one whose words are a network's (a lookup, a timeout, a reset // connection) is a sdkstep.Passing, so the build module gives the ask again after a delay instead of failing it (ADR 0664 §1). func downloadFailure(failure, format string, args ...any) error { @@ -107,6 +106,7 @@ func downloadFailure(failure, format string, args ...any) error { return err } +// takeVersion takes one asked version into dir's modfile (takeVersions). func (m *Module) takeVersion(ctx context.Context, root, dir string, tv *TakenVersion, pins *sync.Mutex) error { modfile, err := m.ensureModfile(dir) if err != nil { @@ -304,5 +304,10 @@ func comparePre(a, b string) int { // "invalid version" it wraps it in): it labels a commit the forge holds on no pull request head (placePin), and decides // nothing; every other failure is the network's or the forge's. func unknownRevision(failure string) bool { + // go words any failure of a proxy as "invalid version: ..." (issue 1966): a failure of the network is not go knowing + // no such revision, whatever else the text says. + if sdkstep.PassingCause(failure) { + return false + } return strings.Contains(failure, "unknown revision") || strings.Contains(failure, "invalid version") } diff --git a/modules/mesh-build-go/cmd/mesh-build-go/versions_test.go b/modules/mesh-build-go/cmd/mesh-build-go/versions_test.go index 68097ae6f..f2e54402b 100644 --- a/modules/mesh-build-go/cmd/mesh-build-go/versions_test.go +++ b/modules/mesh-build-go/cmd/mesh-build-go/versions_test.go @@ -49,6 +49,10 @@ func TestAnAskedVersionIsWrittenIntoTheModfileWhateverGoModNames(t *testing.T) { built: commitSHA, said: "placed from the pull request's head"}, {name: "a lookup that times out is no pin", asked: offHead, failing: true, failsWith: "i/o timeout", passing: true, downloads: 1, answer: `{"Path":"` + sdk + `","Error":"git.novox.be/novox/mesh-sdk/go@` + offHead + `: dial tcp: lookup git.novox.be on 10.10.0.3:53: read udp 10.10.0.3:42292->10.10.0.3:53: i/o timeout"}`}, + // Issue 1966: go words a public proxy it cannot reach as "invalid version"; that is the network's failure, not an + // unknown revision, so a trunk commit on no pull request head is not labelled "on no pull request head". + {name: "an unreachable proxy worded invalid version is no pin", asked: offHead, failing: true, failsWith: "network is unreachable", passing: true, + answer: `{"Path":"` + sdk + `","Error":"invalid version: Get \"https://proxy.golang.org/` + sdk + `/@v/` + offHead + `.info\": dial tcp [2a00:1450::1]:443: connect: network is unreachable"}`}, // The forge's own listing failing is a failure that may be retaken too, with its own cause (review 3). {name: "a forge that cannot be listed is retaken", asked: commitSHA, failing: true, forgeDown: true, failsWith: "unable to access the forge", passing: true, diff --git a/modules/mesh-build-image/cmd/mesh-build-image/copy.go b/modules/mesh-build-image/cmd/mesh-build-image/copy.go index bd84c8ceb..f9e8038de 100644 --- a/modules/mesh-build-image/cmd/mesh-build-image/copy.go +++ b/modules/mesh-build-image/cmd/mesh-build-image/copy.go @@ -99,7 +99,7 @@ func (m *Module) copy(ctx context.Context, args map[string]any) (any, error) { now = m.Now } asked := CopyWait{Digest: digest, Module: module, Artifact: name, From: from, Node: m.Node, - Asked: now().UTC(), Token: strconv.FormatInt(now().UnixNano(), 36) + "-" + m.Node} + Asked: now().UTC(), Nonce: strconv.FormatInt(now().UnixNano(), 36) + "-" + m.Node} // Waited for before the ask, in memory and in the state mirror-waits, so an outcome said at once, or heard by another // instance of the module (the seat's events reach one of them), is not missed. heard, stop := m.Outcomes.Expect(digest) @@ -107,7 +107,7 @@ func (m *Module) copy(ctx context.Context, args map[string]any) (any, error) { if _, err := m.Outcomes.Waits.Put(key, asked); err != nil { return nil, fmt.Errorf("%s%s: keeping the copy's wait in %s: %v", builds.NotStored, name, MirrorWaitsState, err) } - defer m.Outcomes.drop(key, asked.Token) + defer m.Outcomes.drop(key, asked.Nonce) said := buildstep.NarrateSeat(ctx, buildstep.ImageSeat, "mirror of "+from) raw, err := m.call()("seat:"+buildstep.ImageSeat+"."+reg.VerbMirror, reg.MirrorInput{Name: module + "/" + name, Source: from, Digest: digest}) if err != nil { @@ -172,7 +172,8 @@ const MirrorWaitsState = "mirror-waits" // MirrorWaitKey is a copy's key in mirror-waits: its digest, with the colon a key cannot hold as a dash. func MirrorWaitKey(digest string) string { return strings.Replace(digest, ":", "-", 1) } -// CopyWait is one value of mirror-waits: the copy asked, and its outcome once heard. +// CopyWait is one value of mirror-waits: the copy asked, and its outcome once heard. Nonce tells this copy's wait from +// a later copy's of the same digest; it is no credential, and no field here may be named as one (the state refuses it). type CopyWait struct { Digest string `json:"digest"` Module string `json:"module"` @@ -180,7 +181,7 @@ type CopyWait struct { From string `json:"from"` Node string `json:"node"` Asked time.Time `json:"asked"` - Token string `json:"token"` + Nonce string `json:"nonce"` Outcome *MirrorOutcome `json:"outcome,omitempty"` } @@ -283,9 +284,9 @@ func (o *Outcomes) kept(key string) (MirrorOutcome, bool) { return *w.Outcome, true } -// drop removes key's wait when it is still the copy's own (token): a later copy of the same digest keeps its own. -func (o *Outcomes) drop(key, token string) { - if w, err := o.read(key); err == nil && w != nil && w.Token == token { +// drop removes key's wait when it is still the copy's own (nonce): a later copy of the same digest keeps its own. +func (o *Outcomes) drop(key, nonce string) { + if w, err := o.read(key); err == nil && w != nil && w.Nonce == nonce { _ = o.Waits.Delete(key) } } diff --git a/modules/mesh-build-image/cmd/mesh-build-image/helpers_test.go b/modules/mesh-build-image/cmd/mesh-build-image/helpers_test.go index 024543477..4e01d5282 100644 --- a/modules/mesh-build-image/cmd/mesh-build-image/helpers_test.go +++ b/modules/mesh-build-image/cmd/mesh-build-image/helpers_test.go @@ -115,6 +115,9 @@ func (w *memWaits) Put(key string, value any) (uint64, error) { if err != nil { return 0, err } + if f := credentialShaped(b); f != "" { + return 0, fmt.Errorf("the state refuses a field %q, which names a credential", f) + } w.mu.Lock() if w.m == nil { w.m = map[string]json.RawMessage{} @@ -339,3 +342,38 @@ func tool(t *testing.T, m *Module, name string) func(map[string]any) (any, error t.Fatalf("no tool %s", name) return nil } + +// credentialShaped is the first field name in a JSON value that ends as the state's guard refuses (the bus's +// credentialEndings), or "": the in-memory state refuses it as the bus does, so every copy test meets the guard. +func credentialShaped(value []byte) string { + var v any + if json.Unmarshal(value, &v) != nil { + return "" + } + var walk func(any) string + walk = func(v any) string { + switch t := v.(type) { + case map[string]any: + for k, x := range t { + norm := strings.NewReplacer("-", "", "_", "", " ", "").Replace(strings.ToLower(k)) + for _, end := range []string{"password", "passwd", "secret", "token", "credential", "credentials", + "authorization", "apikey", "privatekey", "accesskey", "cookie"} { + if strings.HasSuffix(norm, end) { + return k + } + } + if f := walk(x); f != "" { + return f + } + } + case []any: + for _, x := range t { + if f := walk(x); f != "" { + return f + } + } + } + return "" + } + return walk(v) +} diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go b/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go index 4a9941fb3..9b29b439f 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go @@ -311,6 +311,8 @@ type Got struct { // Idea is said of a research effort kept as an idea: it is not what holds (ADR 0629 §2). Idea string `json:"idea,omitempty"` Waiting string `json:"waiting,omitempty"` + // Round is the round of the next review of the working version (ADR 0665). + Round int `json:"round,omitempty"` // Retracted are the retractions the operator approved; Retracting, one waiting for the operator. Retracted []RetractionRow `json:"retracted,omitempty"` Retracting string `json:"retracting,omitempty"` @@ -386,11 +388,21 @@ func (k *KB) Get(a GetAsk) (*Got, error) { e.PendingVersion, k, last.Version) } } - if r.Ask != nil { - g.Waiting = "the operator's ask: " + r.Ask.Said - if r.Ask.Said == AskWaiting && r.Ask.Held != "" { + if w := e.Working(); a.Version == 0 && w.Status == StatusProposed { + g.Round = w.Round() + } + ask := r.Ask + if a.Version == 0 && e.Pending != nil && e.Pending.Ask != nil { + ask = e.Pending.Ask // the ask of the version being worked + } + if ask != nil { + g.Waiting = "the operator's ask: " + ask.Said + if ask.Round > 0 { + g.Waiting = fmt.Sprintf("the operator's ask after round %d asked changes: %s", ask.Round, ask.Said) + } + if ask.Said == AskWaiting && ask.Held != "" { // Held by the router, not shown to the operator yet: said so, with why (ADR 0397, issue 926). - g.Waiting = "the operator's ask is held by the router, not yet shown to the operator: " + r.Ask.Held + g.Waiting = "the operator's ask is held by the router, not yet shown to the operator: " + ask.Held } } if a.Version == 0 { diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/close.go b/modules/mesh-knowledge/cmd/mesh-knowledge/close.go index 5d2c95085..5970de867 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/close.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/close.go @@ -85,6 +85,10 @@ func (k *KB) Close(caller string, a CloseAsk) (*Closed, error) { if cur.Retracting != nil { return nil, fmt.Errorf("%s has a retraction waiting for the operator; nothing was closed", key) } + if w := e.Working(); w.Ask != nil && w.Ask.Said == AskWaiting && w.Ask.Round > 0 { + return nil, fmt.Errorf("%s is at round %d of its review, which asked changes: the operator decides it, and no review runs before the "+ + "answer. Nothing was closed; close it after the operator's answer", key, w.Ask.Round) + } want := closeLinkKind[a.To] var added []Link var refusals []Refusal @@ -111,7 +115,11 @@ func (k *KB) Close(caller string, a CloseAsk) (*Closed, error) { name = fmt.Sprintf("%s (%d)", closeSection[a.To], n) } r.Sections = append(r.Sections, Section{Name: name, Text: note}) - r.Reviews, r.Ask, r.Retracting = nil, nil, nil + r.Reviews, r.CountFrom, r.Ask, r.Retracting = nil, 0, nil, nil + if e.Pending != nil { + // The closing keeps the round count of the version it replaces (ADR 0665 §1). + r.Reviews, r.CountFrom = e.Pending.Reviews, e.Pending.CountFrom + } r.NeedsOperator, r.Brief = false, nil r.Author = caller r.Writers = append([]string{}, cur.Writers...) @@ -243,7 +251,8 @@ func ClosingQuestion(r *Record, from string, linked []*Record) string { b.WriteString("the note says why it is parked as an idea. A closing to an idea takes no links: never ask for one.") } b.WriteString(" Answer changes otherwise, with findings, each naming the note or the link and saying what to change. " + - "Do not ask for changes of taste or wording that leave the meaning as it is.\n\n") + "Do not ask for changes of taste or wording that leave the meaning as it is.\n") + b.WriteString(roundText(r) + "\n") fmt.Fprintf(&b, "The note, section %q:\n\n%s\n\n", c.Section, c.Note) if c.To == StatusGraduated { b.WriteString("The became links the effort carries, those it carried before and those the closing adds:\n") diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go b/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go index ec4edba72..87741c18f 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go @@ -364,6 +364,10 @@ func (k *KB) Propose(caller string, a ProposeAsk) (*Proposed, error) { current := true why := strings.TrimSpace(a.Why) if held != nil { + if w := held.Working(); w.Status == StatusProposed && w.Ask != nil && w.Ask.Said == AskWaiting && w.Ask.Round > 0 { + return nil, fmt.Errorf("%s is at round %d of its review, which asked changes: the operator decides it, and no review runs before the "+ + "answer. Nothing was proposed; propose again after the operator's answer", key, w.Ask.Round) + } switch w := held.Working(); { case held.Current.Status == StatusReserved: r.ReservedBy, r.ReservedAt, r.ReservedFor = held.Current.ReservedBy, held.Current.ReservedAt, held.Current.ReservedFor @@ -380,7 +384,7 @@ func (k *KB) Propose(caller string, a ProposeAsk) (*Proposed, error) { "Nothing was proposed", key, w.Author, caller) } r.Author, r.Reviews, r.ReservedBy, r.ReservedAt, r.ReservedFor = w.Author, w.Reviews, w.ReservedBy, w.ReservedAt, w.ReservedFor - r.Parts = w.Parts + r.Parts, r.CountFrom = w.Parts, w.CountFrom r.Writers = w.Writers if a.Insight != "" { refusals = append(refusals, Refusal{Rule: "reserved", Says: "a proposal not yet accepted is revised, not corrected by an insight"}) @@ -411,6 +415,10 @@ func (k *KB) Propose(caller string, a ProposeAsk) (*Proposed, error) { } r.AcceptedAt = held.Current.AcceptedAt r.Parts = held.Current.Parts + if held.Pending != nil { + // A revision of a pending version keeps its earlier reviews, so the round count goes on (ADR 0665 §1). + r.Reviews, r.CountFrom = held.Pending.Reviews, held.Pending.CountFrom + } r.Pending = &PendingState{Insight: Insight{Date: k.now().Format("2006-01-02"), Before: strings.TrimSpace(a.Insight), By: caller}} r.Date = held.Current.Date r.Effort = a.Status diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go b/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go index 202ac029a..5bdfb7bfc 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go @@ -92,6 +92,9 @@ const AskGlance = 1 // again to hold the warrant to it — in the form it was asked in, so an ask made before a form changed is still // held to the words the operator was shown. func composeAsk(r *Record, a *OperatorAsk) oc.Ask { + if a.Round > 0 { + return changesAsk(r, a) + } if a.Form == AskGlance { return glanceAsk(r, a) } @@ -124,6 +127,44 @@ func glanceAsk(r *Record, a *OperatorAsk) oc.Ask { return withOptions(r, a, oc.Ask{ID: a.ID, Headline: headline, Explanation: explanation, Details: details}) } +// changesAsk is the ask of a record whose round-5 review asked changes (ADR 0665 §3): the findings, read at a glance, +// and the two answers. It is composed from the record and its ask alone: the findings are the last review's. +func changesAsk(r *Record, a *OperatorAsk) oc.Ask { + name := r.Key().String() + var b strings.Builder + fmt.Fprintf(&b, "The knowledge base's review of %s asked changes at round %d, the last round: you decide. Approve accepts the version as written; "+ + "Decline leaves it proposed for its author, and its next review starts at round 1.", name, a.Round) + if t := strings.TrimSuffix(strings.TrimSpace(r.Title), "."); shown(t) { + fmt.Fprintf(&b, "\n\nIts title: %s.", t) + } + if n := len(r.Reviews); n > 0 { + b.WriteString("\n\nThe findings:") + size, hidden := 0, 0 + for _, f := range strings.Split(r.Reviews[n-1].Findings, "\n") { + if f = strings.TrimSpace(f); f == "" { + continue + } + if rs := []rune(f); len(rs) > 300 { + f = string(rs[:300]) + "..." + } + if !shown(f) || size+len(f) > 1500 { + hidden++ + continue + } + size += len(f) + fmt.Fprintf(&b, "\n- %s", f) + } + if hidden > 0 { + fmt.Fprintf(&b, "\n(%d finding(s) not shown here)", hidden) + } + } + fmt.Fprintf(&b, "\n\nRead it whole with the knowledge base's get, number %d; its fingerprint is %s.", r.Number, fingerprint(r.Digest())) + out := withOptions(r, a, oc.Ask{ID: a.ID, Headline: fmt.Sprintf("%s: accept it as written after round %d?", name, a.Round), Explanation: b.String()}) + out.Options[0].Does = "the version is accepted as written and its clauses are in force" + out.Options[1].Does = "the record stays proposed for its author; its next review is round 1" + return out +} + // listedAsk is the ask as it was composed before the brief: every clause's lead listed. Composed only to hold // the warrant of an ask made then. func listedAsk(r *Record, a *OperatorAsk) oc.Ask { @@ -180,7 +221,11 @@ func newAskID(r *Record) string { // it. The caller holds the write lock. An ask that could not be made or published is taken back, and the review is // asked again after a pause, counted with the tries before it (tries): the record stays proposed, and is reviewed // at most MostReviewTries times. -func (k *KB) askOperator(ctx context.Context, held *Entry, w *Record, by string, tries int) (string, error) { +// +// round is 0 for a record the review approved. For round MostReviewRounds it is that round: the review asked changes, +// and the operator is asked to accept the version as written or leave it proposed (ADR 0665 §3). An ask not made +// then takes the round's review back, so its retry is the same round. +func (k *KB) askOperator(ctx context.Context, held *Entry, w *Record, by string, tries int, round int) (string, error) { version := held.CurrentVersion if held.Pending != nil { version = held.PendingVersion @@ -189,6 +234,10 @@ func (k *KB) askOperator(ctx context.Context, held *Entry, w *Record, by string, w.Ask = nil 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()) + if round > 0 { + w.Reviews = w.Reviews[:len(w.Reviews)-1] + said = fmt.Sprintf("round %d asked changes, and %s (%s", round, 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) @@ -203,17 +252,21 @@ func (k *KB) askOperator(ctx context.Context, held *Entry, w *Record, by string, if k.Asks == nil { return unasked("the operator cannot be asked from here") } - oa := &OperatorAsk{ID: newAskID(w), At: k.now(), Version: version, Said: AskWaiting, Form: AskGlance} + oa := &OperatorAsk{ID: newAskID(w), At: k.now(), Version: version, Said: AskWaiting, Form: AskGlance, Round: round} ask := composeAsk(w, oa) if err := occ.Check(ask, k.now()); err != nil { return unasked(fmt.Sprintf("the ask could not be made (%v)", err)) } w.Ask = oa - if err := k.Store.Save(ctx, Version{By: by, At: k.now(), Why: "reviewed: approved; the operator is asked", Record: w, Insight: held.Pending != nil && held.Pending.IsInsight()}, + asked := "approved; the operator is asked" + if round > 0 { + asked = fmt.Sprintf("round %d asked changes; the operator is asked", round) + } + if err := k.Store.Save(ctx, Version{By: by, At: k.now(), Why: "reviewed: " + asked, Record: w, Insight: held.Pending != nil && held.Pending.IsInsight()}, held.Pending == nil, false, held.Revision); err != nil { return "", fmt.Errorf("%s: the review could not be kept (%v); the operator was not asked", w.Key(), err) } - k.emit(EventReviewed, w, by, "approved; the operator is asked", held.Versions+1) + k.emit(EventReviewed, w, by, asked, held.Versions+1) if err := k.Asks.Publish(oc.AcceptSubjectAsk(Module), ask, ask.ID); err != nil { k.logf("[mesh-knowledge] the ask for %s could not be published: %v", w.Key(), err) return unasked(fmt.Sprintf("the operator could not be asked (%v)", err)) @@ -351,6 +404,9 @@ func (k *KB) decide(ctx context.Context, e *Entry, w *Record, warrant oc.Warrant case OptionDecline: w = w.Clone() w.Ask.Warrant = occ.Says(warrant) + if w.Ask.Round > 0 { + w.CountFrom = len(w.Reviews) // the revision's review starts a new count at round 1 (ADR 0665 §3) + } return k.endAsk(ctx, e, w, "declined by the operator") } return k.endAsk(ctx, e, w, "was answered with "+option.ID+", which it does not offer") diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/record.go b/modules/mesh-knowledge/cmd/mesh-knowledge/record.go index 79cf58fe4..1cc60e6e9 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/record.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/record.go @@ -264,6 +264,9 @@ type OperatorAsk struct { // Held is why the router holds the ask while it waits: no channel can carry it yet (ADR 0397, issue 926). // Empty once the router offers it, or when it ended. Held string `json:"held,omitempty"` + // Round is the review round whose changes this ask lays before the operator (ADR 0665 §3); 0 for an ask of + // the approval of a record the review approved. + Round int `json:"round,omitempty"` } // Drafting is where a prose record stands in the drafting and confirming of its clauses (to-be 55 §7). @@ -329,6 +332,9 @@ type Record struct { // AcceptedAt is when it was accepted here; zero for one imported accepted. AcceptedAt time.Time `json:"accepted-at,omitempty"` Reviews []Review `json:"reviews,omitempty"` + // CountFrom is how many of Reviews came before the round count last restarted: the operator's Decline of a + // round-5 ask restarts it (ADR 0665 §1, §3), so the next review is round 1 (review_round.go). + CountFrom int `json:"count-from,omitempty"` // Parts are the latest verdict of each part of the version (the head, each section), kept for a new version whose // part is unchanged (review_parts.go). Parts []PartReview `json:"parts,omitempty"` @@ -484,7 +490,7 @@ func (r *Record) Clone() *Record { // it. func (r *Record) Digest() string { c := r.Clone() - c.Reviews, c.Ask, c.Drafting, c.Judging, c.Parts, c.FormOnly = nil, nil, nil, nil, nil, false + c.Reviews, c.Ask, c.Drafting, c.Judging, c.Parts, c.FormOnly, c.CountFrom = nil, nil, nil, nil, nil, false, 0 raw, _ := json.Marshal(c) sum := sha256.Sum256(raw) return "sha256:" + hex.EncodeToString(sum[:]) diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go index c2208f5c3..12aecb349 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go @@ -121,7 +121,8 @@ func ReviewQuestion(r *Record, linked []*Record) string { "what it decides is checked; and every link fits: a record it changes says which clause and really changes that "+ "clause, a record it supersedes is really replaced whole, and no clause contradicts a clause in force of a record it "+ "links without changing or superseding it. Answer changes otherwise, with findings, each naming the section or "+ - "clause and saying what to change. Do not ask for changes of taste or wording that leave the meaning as it is.\n\n", what, r.Key()) + "clause and saying what to change. Do not ask for changes of taste or wording that leave the meaning as it is.\n", what, r.Key()) + b.WriteString(roundText(r) + "\n") b.WriteString("The rules every record was already checked against when it was proposed:\n") for _, rule := range PerRecordRules { fmt.Fprintf(&b, "- %s\n", rule.Holds) @@ -159,7 +160,7 @@ func ReviewQuestion(r *Record, linked []*Record) string { // graduated or abandoned, which it takes once accepted) are shown apart, each named for what it is (hq issue 492). func reviewed(r *Record) *Record { c := r.Clone() - c.Reviews, c.Ask, c.Judging, c.Drafting, c.Writers, c.Author, c.Parts = nil, nil, nil, nil, nil, "", nil + c.Reviews, c.Ask, c.Judging, c.Drafting, c.Writers, c.Author, c.Parts, c.CountFrom = nil, nil, nil, nil, nil, "", nil, 0 c.Extra = addExtra(c.Extra, "state in the knowledge base", c.Status+" (every record under review is; it is not the record's own status)") if c.Kind == KindResearch && c.Effort != "" { c.Extra = addExtra(c.Extra, "research status", c.Effort) @@ -536,7 +537,12 @@ func (k *KB) reviewOne(key Key) (string, error) { } } } + round := w.Round() w.Reviews = append(w.Reviews, Review{By: by, At: k.now(), Verdict: verdict, Findings: strings.Join(findings, "\n"), Version: version}) + if verdict == VerdictChanges && round >= MostReviewRounds { + // Round 5's changes are the operator's to decide (ADR 0665 §3): no review runs before the answer. + return k.askOperator(ctx, now, w, by, tries, round) + } if verdict == VerdictChanges { why := fmt.Sprintf("reviewed: changes asked (%d finding(s)); it stays proposed for its author to revise", len(findings)) if err := save(w, why); err != nil { @@ -546,7 +552,7 @@ func (k *KB) reviewOne(key Key) (string, error) { return key.String() + ": " + why, nil } if w.NeedsOperator { - return k.askOperator(ctx, now, w, by, tries) + return k.askOperator(ctx, now, w, by, tries, 0) } k.emit(EventReviewed, w, by, "approved", now.Versions+1) r, err := k.accept(ctx, now, w, by, "accepted after the knowledge base's review", "") diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review_parts.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review_parts.go index e55af336d..d4fa6b428 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/review_parts.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review_parts.go @@ -71,7 +71,8 @@ func introFor(r *Record, what string) string { "what it decides is checked; and every link fits: a record it changes says which clause and really changes that "+ "clause, a record it supersedes is really replaced whole, and no clause contradicts a clause in force of a record it "+ "links without changing or superseding it. Answer changes otherwise, with findings, each naming the section or "+ - "clause and saying what to change. Do not ask for changes of taste or wording that leave the meaning as it is.\n\n", about, r.Key(), what) + "clause and saying what to change. Do not ask for changes of taste or wording that leave the meaning as it is.\n", about, r.Key(), what) + b.WriteString(roundText(r) + "\n") b.WriteString("The rules every record was already checked against when it was proposed:\n") for _, rule := range PerRecordRules { fmt.Fprintf(&b, "- %s\n", rule.Holds) diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review_round.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review_round.go new file mode 100644 index 000000000..f6e0dc608 --- /dev/null +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review_round.go @@ -0,0 +1,66 @@ +package main + +// The review knows its round (novox/hq ADR 0665, issue 784). A record's round is the number of its reviews since the +// last of: it was accepted, it was first proposed, the operator declined it at round MostReviewRounds, plus one. +// Reviews belong to the version being worked (a pending version starts with none) and CountFrom marks the Decline, +// so the count is what is on the record. The question names the round and lists the earlier findings of the count; +// from round OnlyWrongFrom only a finding that makes the record wrong asks changes; at round MostReviewRounds a +// review that asks changes becomes an ask of the operator (askOperator with a round), and no review runs before the +// answer (Propose refuses a revision meanwhile). + +import ( + "fmt" + "strings" +) + +const ( + // MostReviewRounds is the round at which the operator decides. + MostReviewRounds = 5 + // OnlyWrongFrom is the first round in which only a finding that makes the record wrong asks changes. + OnlyWrongFrom = 3 + // MostEarlierFindings bounds the earlier findings a question lists, the latest rounds kept. + MostEarlierFindings = 6 << 10 +) + +// Round is the round of the next review of r. +func (r *Record) Round() int { + from := r.CountFrom + if from > len(r.Reviews) { + from = len(r.Reviews) + } + return len(r.Reviews) - from + 1 +} + +// roundText is what every part's question says of the round: its number, what the round allows, and the findings +// of the earlier rounds of the count. +func roundText(r *Record) string { + round := r.Round() + var b strings.Builder + fmt.Fprintf(&b, "\nThis is round %d of its review. At round %d the operator decides, so name every finding at once; "+ + "a finding an earlier round named and the record answered is not named again.\n", round, MostReviewRounds) + if round >= OnlyWrongFrom { + b.WriteString("From round 3 only a finding that makes the record wrong asks changes: two of its parts contradict each other, " + + "or it contradicts a clause in force of a record it links without changing or superseding that clause, or it breaks one of the " + + "rules above, or a link the whole-set rule needs is missing. Anything else, such as wording, a further case, a nicer form or a " + + "detail its design can hold, is a remark: answer approve and give it as a finding under approve.\n") + } + var earlier []string + for i := r.CountFrom; i < len(r.Reviews) && i >= 0; i++ { + if rv := r.Reviews[i]; rv.Verdict == VerdictChanges && strings.TrimSpace(rv.Findings) != "" { + earlier = append(earlier, fmt.Sprintf("Round %d (version %d) asked changes:\n%s\n", i-r.CountFrom+1, rv.Version, rv.Findings)) + } + } + size, from := 0, len(earlier) + for from > 0 && size+len(earlier[from-1]) <= MostEarlierFindings { + from-- + size += len(earlier[from]) + } + if from < len(earlier) { + b.WriteString("\nThe findings of the earlier rounds:\n") + if from > 0 { + fmt.Fprintf(&b, "(%d earlier round(s) are not shown: the question has no room for them.)\n", from) + } + b.WriteString(strings.Join(earlier[from:], "\n")) + } + return b.String() +} diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review_round_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review_round_test.go new file mode 100644 index 000000000..a87a7c693 --- /dev/null +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review_round_test.go @@ -0,0 +1,191 @@ +package main + +import ( + "fmt" + "strings" + "testing" + "time" + + occ "git.novox.be/novox/mesh-contracts/operator-and-conversation/go/clients/operatorchannelclient" +) + +// ADR 0665: the review knows its round; from round 3 only a record that is wrong asks changes; at round 5 changes +// become an ask of the operator, no review runs before the answer, and a Decline restarts the count. + +func changes(i int) string { + return fmt.Sprintf(`{"verdict":"changes","findings":["finding of review %d"]}`, i) +} + +// review is the i'th review of record n: its author revises it first (but for the first), then every part is +// judged with answer. It answers the questions the judge was asked. +func (w *world) review(t *testing.T, n, i int, answer string) string { + t.Helper() + p := valid(n) + p.Sections[2].Text = fmt.Sprintf("Merges land sooner; revision %d.", i) + if _, err := w.k.Propose("author@g14", p); err != nil { + t.Fatal(err) + } + w.judge.review, w.judge.reviews = answer, nil + w.reviewAll(t) + return strings.Join(w.judge.reviews, "\n") +} + +func TestTheReviewIsToldItsRoundAndRoundFiveIsTheOperators(t *testing.T) { + w := imported(t, newMemStore()) + n := w.reserved(t, "author@g14") + key := Key{KindDecision, n} + for i := 1; i <= 4; i++ { + q := w.review(t, n, i, changes(i)) + if !strings.Contains(q, fmt.Sprintf("This is round %d of its review", i)) || !strings.Contains(q, "At round 5 the operator decides") || + strings.Contains(q, "From round 3 only") != (i >= 3) { + t.Fatalf("round %d's question: %.600s", i, q) + } + for j := 1; j < i; j++ { // every earlier round's findings, none of this round's + if !strings.Contains(q, fmt.Sprintf("Round %d (version", j)) || !strings.Contains(q, fmt.Sprintf("finding of review %d", j)) { + t.Errorf("round %d's question lacks the findings of round %d", i, j) + } + } + if strings.Contains(q, fmt.Sprintf("finding of review %d", i)) { + t.Errorf("round %d's question holds its own finding", i) + } + } + if e := mustGet(t, w.k.Store, key); e.Current.Status != StatusProposed || len(e.Current.Reviews) != 4 || e.Current.Round() != 5 || len(w.router.asked) != 0 { + t.Fatalf("after four rounds: %s, %d reviews, %d asks", e.Current.Status, len(e.Current.Reviews), len(w.router.asked)) + } + + // Round 5 asks changes: the operator is asked, showing the findings; no review runs, and no revision is taken, before the answer. + if q := w.review(t, n, 5, changes(5)); !strings.Contains(q, "This is round 5 of its review") { + t.Fatalf("round 5's question: %.300s", q) + } + if len(w.router.asked) != 1 { + t.Fatalf("asks after round 5's changes: %d", len(w.router.asked)) + } + ask := w.router.asked[0] + if err := occ.Check(ask, w.clock); err != nil || !strings.Contains(ask.Explanation, "finding of review 5") || !strings.Contains(ask.Explanation, "round 5") { + t.Fatalf("the ask: %v %+v", err, ask) + } + if e := mustGet(t, w.k.Store, key); e.Current.Ask == nil || e.Current.Ask.Round != 5 || e.Current.Status != StatusProposed { + t.Fatalf("after round 5: %+v", e.Current.Ask) + } + w.judge.reviews = nil + if said := w.reviewAll(t); len(said) != 0 || len(w.judge.reviews) != 0 { + t.Fatalf("a review ran before the operator answered: %v", said) + } + p := valid(n) + p.Sections[2].Text = "Merges land sooner; revision 6." + if _, err := w.k.Propose("author@g14", p); err == nil || !strings.Contains(err.Error(), "the operator decides") { + t.Fatalf("a revision before the answer: %v", err) + } + + // Decline: it stays proposed, and the revision's review is round 1. + w.router.answer(ask.ID, OptionDecline, w.clock.Add(time.Minute)) + if _, err := w.k.Reconcile(); err != nil { + t.Fatal(err) + } + if q := w.review(t, n, 6, changes(6)); !strings.Contains(q, "This is round 1 of its review") || strings.Contains(q, "finding of review") { + t.Fatalf("after the Decline: %.400s", q) + } + if len(w.router.asked) != 1 { + t.Fatalf("round 1 asked the operator") + } +} + +func TestRoundFiveApprovedByTheOperatorAcceptsTheVersionAsWritten(t *testing.T) { + w := imported(t, newMemStore()) + n := w.reserved(t, "author@g14") + for i := 1; i <= 5; i++ { + w.review(t, n, i, changes(i)) + } + w.router.answer(w.router.asked[0].ID, OptionApprove, w.clock.Add(time.Minute)) + if _, err := w.k.Reconcile(); err != nil { + t.Fatal(err) + } + if e := mustGet(t, w.k.Store, Key{KindDecision, n}); e.Current.Status != StatusAccepted || e.Current.Ask.Said != AskApproved { + t.Fatalf("%s %+v", e.Current.Status, e.Current.Ask) + } +} + +// From round 3 a finding that is only a remark sits under approve, and the record is accepted. +func TestARemarkUnderApproveAtRoundFourAcceptsTheRecord(t *testing.T) { + w := imported(t, newMemStore()) + n := w.reserved(t, "author@g14") + for i := 1; i <= 3; i++ { + w.review(t, n, i, changes(i)) + } + q := w.review(t, n, 4, `{"verdict":"approve","findings":["wording: the second section could say it shorter"]}`) + if !strings.Contains(q, "answer approve and give it as a finding under approve") { + t.Fatalf("the question: %.400s", q) + } + if e := mustGet(t, w.k.Store, Key{KindDecision, n}); e.Current.Status != StatusAccepted || len(e.Current.Reviews) != 4 { + t.Fatalf("%s, %d reviews", e.Current.Status, len(e.Current.Reviews)) + } +} + +// The rounds of a pending version (an insight to an accepted record) go on across its revisions. +func TestTheRoundCountGoesOnAcrossRevisionsOfAPendingInsight(t *testing.T) { + w := imported(t, newMemStore()) + n := w.reserved(t, "author@g14") + w.accepted(t, "author@g14", valid(n)) + var q string + for i := 1; i <= 3; i++ { + p := valid(n) + p.Insight = "it said only that merges land sooner" + p.Sections[2].Text = fmt.Sprintf("Merges land sooner, revision %d.", i) + if _, err := w.k.Propose("author@g14", p); err != nil { + t.Fatal(err) + } + w.judge.review, w.judge.reviews = changes(i), nil + w.reviewAll(t) + q = strings.Join(w.judge.reviews, "\n") + } + if !strings.Contains(q, "This is round 3 of its review") { + t.Fatalf("the third revision of the insight: %.300s", q) + } +} + +// A pending version of an active effort: revisions and a closing keep the count (ADR 0665 §1), `get` says the round and +// the ask after round 5, and a close is refused while that ask waits (§3). +func TestARoundCountSurvivesAClosingAndAClosingWaitsForTheOperatorsAnswer(t *testing.T) { + w := imported(t, newMemStore()) + n := w.effort(t, StatusActive) + revise := func(i int, answer string) string { + next := effortAsk(n, StatusActive, Section{Name: "00-overview.md", Text: fmt.Sprintf("What a roadmap entry would be, revision %d.", i)}) + if _, err := w.k.Propose("author@g14", next); err != nil { + t.Fatal(err) + } + w.judge.review, w.judge.reviews = answer, nil + w.reviewAll(t) + return strings.Join(w.judge.reviews, "\n") + } + for i := 1; i <= 2; i++ { + revise(i, changes(i)) + } + if g, err := w.k.Get(GetAsk{Kind: KindResearch, Number: n}); err != nil || g.Round != 3 { + t.Fatalf("get after two rounds: %+v, %v", g, err) + } + if g, err := w.k.Get(GetAsk{Kind: KindDecision, Number: 276}); err != nil || g.Round != 0 { + t.Fatalf("get of an accepted record shows round %d, %v", g.Round, err) + } + if _, err := w.k.Close("lead@shanks", CloseAsk{Number: n, To: StatusIdea, Note: "Parked: nothing carries it this month."}); err != nil { + t.Fatal(err) + } + w.judge.review, w.judge.reviews = "", nil + w.reviewAll(t) + if q := strings.Join(w.judge.reviews, "\n"); !strings.Contains(q, "This is round 3 of its review") || !strings.Contains(q, "finding of review 2") { + t.Fatalf("the closing's question: %.500s", q) + } + + m := w.effort(t, StatusActive) + n = m + for i := 1; i <= 5; i++ { + revise(i, changes(i)) + } + g, err := w.k.Get(GetAsk{Kind: KindResearch, Number: n}) + if err != nil || !strings.Contains(g.Waiting, "after round 5 asked changes") { + t.Fatalf("get after round 5: %+v, %v", g.Waiting, err) + } + if _, err := w.k.Close("lead@shanks", CloseAsk{Number: n, To: StatusIdea, Note: "Parked: nothing carries it this month."}); err == nil || + !strings.Contains(err.Error(), "the operator decides") { + t.Fatalf("a close before the answer: %v", err) + } +}