diff --git a/cmd/mavend/ecosystem_acts.go b/cmd/mavend/ecosystem_acts.go index dc1e6fc..700867d 100644 --- a/cmd/mavend/ecosystem_acts.go +++ b/cmd/mavend/ecosystem_acts.go @@ -533,6 +533,64 @@ func traceErrorFields(err error) map[string]any { return fields } +// entityResolution — what asking Nexus about a turn's candidate names came to. +// One shape rather than five return values, because the caller needs the +// reference that answered as well as the answer: it goes in the trace. +type entityResolution struct { + subject string // the reference Nexus answered about + entityID string // set when exactly one name resolved + displayName string // that entity's name as Nexus spells it + ambiguous []string // candidate display names to ask between + err error // a dependency failure, not a miss +} + +// resolveEntityCandidates asks Nexus about each name the turn offered and +// reports what it knows, stopping early where the answer is already decided. +// +// The rules, in the order they apply: +// +// - A dependency failure ends it. Nexus being down is not "no such entity", +// and asking about the next name would report the outage as a miss. +// - Nexus calling one name ambiguous ends it. It has the candidates and it is +// telling us to ask. +// - Two names resolving to different entities is a clarify too, this time ours: +// "перезапусти nginx на muzick-indexer" names both a service and its host, +// and picking either would be inventing an intent he did not state. +// - Nothing resolving returns the first name as the subject, so the trace says +// what was actually looked for. +func (h *reactiveHandler) resolveEntityCandidates(ctx context.Context, refs []string) entityResolution { + var out entityResolution + for _, ref := range refs { + entityID, displayName, ambiguous, err := h.ecosystem.resolveEntityReference(ctx, ref, nil) + if err != nil { + return entityResolution{subject: ref, err: err} + } + if len(ambiguous) > 0 { + return entityResolution{subject: ref, ambiguous: ambiguous} + } + if entityID == "" { + continue + } + if out.entityID == "" { + out = entityResolution{subject: ref, entityID: entityID, displayName: displayName} + continue + } + if entityID == out.entityID { + continue + } + // Both are real and they are not the same thing. Hand back the names + // Nexus spells, not the words he happened to say. + return entityResolution{ + subject: out.subject, + ambiguous: []string{out.displayName, displayName}, + } + } + if out.entityID == "" && len(refs) > 0 { + out.subject = refs[0] + } + return out +} + // handleHexisAct — resolves entity references through Nexus and executes // matching capabilities through Hexis. Returns a reply string when handled, // or "" to fall through to the system command executor. @@ -550,11 +608,11 @@ func (h *reactiveHandler) handleHexisAct(ctx context.Context, dec router.Decisio // Resolve the utterance text as an entity reference through Nexus. An // ambiguous match must stop and clarify — never guess a mutation target. - // The name comes from entityReferenceText, not straight from the Text slot: - // the model transliterates Latin names as it routes (Vikunja #476). - subject := entityReferenceText(dec) + // The names come from entityReferences, not straight from the Text slot: the + // model transliterates Latin names as it routes (Vikunja #476, #524). started := h.now() - entityID, displayName, ambiguous, err := h.ecosystem.resolveEntityReference(ctx, subject, nil) + res := h.resolveEntityCandidates(ctx, entityReferences(dec)) + subject, entityID, displayName, ambiguous, err := res.subject, res.entityID, res.displayName, res.ambiguous, res.err if err != nil { h.recordEcosystemTrace(ctx, "nexus", "resolve", traceStatusForError(err), started, mergeFields(traceErrorFields(err), map[string]any{"subject": redactSubject(subject)})) diff --git a/cmd/mavend/entityname.go b/cmd/mavend/entityname.go index bc42b75..9a9d544 100644 --- a/cmd/mavend/entityname.go +++ b/cmd/mavend/entityname.go @@ -13,6 +13,11 @@ import ( // ride along because "muzick-indexer" and "nginx.conf" are one name, not two. var latinRun = regexp.MustCompile(`[A-Za-z][A-Za-z0-9._-]*(?:\s+[A-Za-z][A-Za-z0-9._-]*)*`) +// maxEntityReferences caps how many names one utterance may send to Nexus. The +// cap is not about correctness, it is about one turn not fanning out into a +// dozen HTTP calls when the utterance is a paragraph of English. +const maxEntityReferences = 4 + // hasLatin reports whether s carries a Latin letter. func hasLatin(s string) bool { for _, r := range s { @@ -23,37 +28,54 @@ func hasLatin(s string) bool { return false } -// entityReferenceText is the name Nexus is asked to resolve. +// entityReferences returns the names Nexus is asked to resolve, in the order +// they were said. // -// Normally that is the router's Text slot, which is the verb phrase the model -// wrote. But the resident model rewrites a Russian utterance as it routes, and -// on the way it transliterates: "перезапусти muzick indexer" came back as +// Normally there is one, and it is the router's Text slot — the verb phrase the +// model wrote. But the resident model rewrites a Russian utterance as it routes, +// and on the way it transliterates: "перезапусти muzick indexer" came back as // "перезагрузить музик индексер" (Vikunja #476). Nexus is then asked for a -// service nobody has ever named, so the act cannot resolve its target even -// with every gate open. +// service nobody has ever named, so the act cannot resolve its target even with +// every gate open. // -// The recovery is deliberately narrow. Only when the utterance holds a Latin -// run and the model's Text holds none has a name certainly been rewritten — -// then the longest Latin run in his own words is the reference. Anything else -// keeps the Text slot, so an English utterance and a Russian entity name are -// both untouched. Un-transliterating the Cyrillic back is not attempted: the +// The recovery is deliberately narrow. Only when the utterance holds a Latin run +// and the model's Text holds none has a name certainly been rewritten. Anything +// else keeps the Text slot, so an English utterance and a Russian entity name +// are both untouched. Un-transliterating the Cyrillic back is not attempted: the // surface form he said is right there, and guessing at a reverse mapping would // invent a second name to be wrong about. -func entityReferenceText(dec router.Decision) string { +// +// What this does NOT do is pick. It used to return the longest run, and length +// is a guess: "перезапусти nginx на muzick-indexer" has two names in it and the +// longer one is not reliably the target. Nexus owns which names it knows +// (docs/ecosystem.md — ambiguous resolution asks the owner, it does not pick), +// so every run goes over and Nexus answers. Two runs that both resolve are a +// clarify, not a coin toss. +func entityReferences(dec router.Decision) []string { text := dec.Slots.Text if hasLatin(text) || !hasLatin(dec.Utterance) { - return text + return []string{text} } - longest := "" + var refs []string + seen := map[string]bool{} for _, m := range latinRun.FindAllString(dec.Utterance, -1) { - if len(m) > len(longest) { - longest = m + m = strings.TrimSpace(m) + // A single stray letter is not a name. + if len(m) < 2 { + continue + } + key := strings.ToLower(m) + if seen[key] { + continue + } + seen[key] = true + refs = append(refs, m) + if len(refs) == maxEntityReferences { + break } } - longest = strings.TrimSpace(longest) - // A single stray letter is not a name. - if len(longest) < 2 { - return text + if len(refs) == 0 { + return []string{text} } - return longest + return refs } diff --git a/cmd/mavend/entityname_test.go b/cmd/mavend/entityname_test.go index 05e7700..85fdc5b 100644 --- a/cmd/mavend/entityname_test.go +++ b/cmd/mavend/entityname_test.go @@ -2,61 +2,76 @@ package main import ( "context" + "net/http" "strings" + "sync" "testing" "github.com/kami/maven/internal/router" ) -// TestEntityReferenceText pins when his own words win over the model's. -func TestEntityReferenceText(t *testing.T) { +// TestEntityReferences pins when his own words win over the model's, and that +// every name he said goes over rather than one of them being picked. +func TestEntityReferences(t *testing.T) { for _, tc := range []struct { name string utterance string text string - want string + want []string }{ { name: "the model transliterated the name", utterance: "перезапусти muzick indexer", text: "перезагрузить музик индексер", - want: "muzick indexer", + want: []string{"muzick indexer"}, }, { name: "it kept the name, so nothing to repair", utterance: "перезапусти muzick indexer", text: "перезагрузить muzick indexer", - want: "перезагрузить muzick indexer", + want: []string{"перезагрузить muzick indexer"}, }, { name: "an all-Russian entity name is not a rewrite", utterance: "перезапусти домашний сервер", text: "перезагрузить домашний сервер", - want: "перезагрузить домашний сервер", + want: []string{"перезагрузить домашний сервер"}, }, { name: "an English turn never enters the recovery", utterance: "restart muzick indexer", text: "restart muzick indexer", - want: "restart muzick indexer", + want: []string{"restart muzick indexer"}, }, { - name: "the longest Latin run is the name", + name: "both names go over, in the order he said them", utterance: "а перезапусти-ка nginx на muzick-indexer, пожалуйста", text: "перезагрузить нгинкс", - want: "muzick-indexer", + want: []string{"nginx", "muzick-indexer"}, }, { name: "one stray letter is not a name", utterance: "перезапусти сервер a", text: "перезагрузить сервер", - want: "перезагрузить сервер", + want: []string{"перезагрузить сервер"}, + }, + { + name: "the same name twice is one question", + utterance: "перезапусти nginx, ну правда, nginx", + text: "перезагрузить нгинкс", + want: []string{"nginx"}, }, } { t.Run(tc.name, func(t *testing.T) { dec := router.Decision{Utterance: tc.utterance, Slots: router.Slots{Text: tc.text}} - if got := entityReferenceText(dec); got != tc.want { - t.Fatalf("entityReferenceText = %q, want %q", got, tc.want) + got := entityReferences(dec) + if len(got) != len(tc.want) { + t.Fatalf("entityReferences = %q, want %q", got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("entityReferences = %q, want %q", got, tc.want) + } } }) } @@ -131,3 +146,77 @@ func TestClarifyStillAsksWithoutHexis(t *testing.T) { t.Fatal("she must still ask what to do") } } + +// nexusInOrder serves one resolve answer per call, in order, so a test can say +// what Nexus knows about the first name and what it knows about the second. The +// last body repeats once the list runs out. +func nexusInOrder(t *testing.T, bodies ...string) *fakeServer { + t.Helper() + var mu sync.Mutex + n := 0 + return newFakeServer(t, map[string]http.HandlerFunc{ + "POST /api/v1/resolve": func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + body := bodies[min(n, len(bodies)-1)] + n++ + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + }, + }) +} + +// TestTwoResolvedNamesAsk — «перезапусти nginx на muzick-indexer» names a +// service and the host it runs on. Both are real, and which one he meant is not +// in the utterance, so she asks. Picking one by length was the old behaviour and +// length is not evidence (Vikunja #524). +func TestTwoResolvedNamesAsk(t *testing.T) { + ctx := context.Background() + nexus := nexusInOrder(t, + fixtureNexusResolved("ent_nginx", "nginx", "service"), + fixtureNexusResolved("ent_host", "Muzick indexer", "device"), + ) + hexis := newFakeHexis(t, restartCaps(), fixtureHexisExecuted("exec_1", "succeeded")) + h := ecoHandler(t, nexus, nil, hexis) + + dec := router.Decision{ + Utterance: "перезапусти nginx на muzick-indexer", + Intent: router.IntentAct, + Slots: router.Slots{Text: "перезагрузить нгинкс", Fn: "restart", HasFn: true}, + } + reply := h.handleHexisAct(ctx, dec) + if !strings.Contains(reply, "nginx") || !strings.Contains(reply, "Muzick indexer") { + t.Fatalf("reply = %q, want both names she found", reply) + } + if hexis.Count("POST", "/api/v1/execute") != 0 { + t.Fatal("she must not execute against a target she is still asking about") + } +} + +// TestTheNameNexusKnowsWins — the other half. Two names go over and only one is +// an entity, so there is nothing to ask about and the act runs. +func TestTheNameNexusKnowsWins(t *testing.T) { + ctx := context.Background() + nexus := nexusInOrder(t, + fixtureNexusNotFound(), + fixtureNexusResolved("ent_muzick", "Muzick indexer", "service"), + ) + hexis := newFakeHexis(t, restartCaps(), fixtureHexisExecuted("exec_1", "succeeded")) + h := ecoHandler(t, nexus, nil, hexis) + + dec := router.Decision{ + Utterance: "перезапусти nginx на muzick-indexer", + Intent: router.IntentAct, + Slots: router.Slots{Text: "перезагрузить нгинкс", Fn: "restart", HasFn: true}, + } + reply := h.handleHexisAct(ctx, dec) + if reply == "" { + t.Fatal("the resolvable name must carry the act") + } + if len(nexus.Requests()) != 2 { + t.Fatalf("nexus asked %d times, want both names", len(nexus.Requests())) + } + if hexis.Count("POST", "/api/v1/execute") == 0 { + t.Fatal("hexis was never asked to run it") + } +} diff --git a/internal/store/migrations_test.go b/internal/store/migrations_test.go index 7aa80ec..01910e4 100644 --- a/internal/store/migrations_test.go +++ b/internal/store/migrations_test.go @@ -114,8 +114,10 @@ func TestStuckRoutinesAreBackfilled(t *testing.T) { t.Fatal(err) } - if _, err := s.db.ExecContext(ctx, migrations[18]); err != nil { - t.Fatalf("migration 19: %v", err) + // Index 19, version 20: standing lists landed on the same number first + // (Vikunja #453), so this one moved down one. + if _, err := s.db.ExecContext(ctx, migrations[19]); err != nil { + t.Fatalf("migration 20: %v", err) } accepted, err := s.ListAcceptedRoutines(ctx)