diff --git a/CLAUDE.md b/CLAUDE.md index 14cf634..f65026b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -296,11 +296,18 @@ beside it, writing `routing_traces` (migration #23). The utterance is stored in clear, because a 384-dimension vector of a short sentence is substantially recoverable and storing vectors instead would be a privacy claim we cannot support. What makes it safe is the same thing that makes the fact store safe. -Retention is 14 days, enforced on write. Nothing reads it outward, and the rule +Retention is 14 days, enforced on write and again on start, so a box that goes +quiet does not keep every row. Nothing reads it outward, and the rule that his notes and facts are never search input covers this table. `Store.Wipe` deletes it with everything else. A correction (V-630) is promoted out into a -seed-shaped row and kept, because a label is not a transcript. The transcript -still expires. Adding a rung to the ladder +seed-shaped row in `routing_labels` (migration #24) and kept, because a label is +not a transcript. The transcript still expires. The gesture that writes one is +two buttons beside the reply on `/chat`, reached over `ipc.CorrectTurn` and the +trace id that now rides back on `ipc.ChatReply`. A turn marked wrong with no +target is a usable negative, so naming the intent is never required. The target +is one of the seven intents and never free text. Only `/chat` offers it: the wire +op assumes no browser, but telegram and voice do not call it yet, and +`docs/plans/22-correcting-a-turn.md` says why voice is the hard one. Adding a rung to the ladder in `runTurn` means adding its name to `preRouteLadder` in `cmd/mavend/decisiontrace.go`, or that rung is silently missing from the record. diff --git a/cmd/mavend/routingtrace.go b/cmd/mavend/routingtrace.go index 02e531b..5c6832e 100644 --- a/cmd/mavend/routingtrace.go +++ b/cmd/mavend/routingtrace.go @@ -16,6 +16,7 @@ import ( "encoding/json" "log" "strings" + "sync" "time" "github.com/kami/maven/internal/decision" @@ -38,6 +39,44 @@ func traceSink(s *store.Store) traceWriter { return s } +// The trace id rides the context, the same seam querysource.go uses and for the +// same reason: handleText answers every reach through one string, and threading +// a second value through the whole action dispatch would change a signature the +// mic, telegram and the web all share. A caller that wants the id asks for a +// sink; the mic path does not, and pays nothing. +type traceIDKey struct{} + +type traceIDSink struct { + mu sync.Mutex + id int64 +} + +func (s *traceIDSink) note(id int64) { + s.mu.Lock() + defer s.mu.Unlock() + s.id = id +} + +// ID is the persisted trace for the turn, or 0 when nothing was persisted. +func (s *traceIDSink) ID() int64 { + s.mu.Lock() + defer s.mu.Unlock() + return s.id +} + +// withTraceIDSink returns a context that collects the persisted trace id, and +// the sink to read after the turn has answered. +func withTraceIDSink(ctx context.Context) (context.Context, *traceIDSink) { + sink := &traceIDSink{} + return context.WithValue(ctx, traceIDKey{}, sink), sink +} + +func noteTraceID(ctx context.Context, id int64) { + if sink, ok := ctx.Value(traceIDKey{}).(*traceIDSink); ok { + sink.note(id) + } +} + // pruneTracesOnStart enforces retention once at wiring time. Pruning on write // alone is not enough: a box that goes quiet for a month keeps every row until // the next sixty-fourth turn, and "kept for fourteen days" would then be true @@ -58,7 +97,8 @@ func pruneTracesOnStart(s *store.Store, now time.Time) { // // Errors are logged and swallowed. A trace is diagnostic and training data, and // a failed insert must never change what the owner hears. -func (h *reactiveHandler) persistDecision(ctx context.Context, rec *decision.Record, src turnSource) { +func (h *reactiveHandler) persistDecision(turnCtx context.Context, rec *decision.Record, src turnSource) { + ctx := turnCtx if h.traces == nil || rec == nil || strings.TrimSpace(rec.Utterance) == "" { return } @@ -85,9 +125,15 @@ func (h *reactiveHandler) persistDecision(ctx context.Context, rec *decision.Rec Outcome: wonAt(rec, decision.StageAction), Claims: claims, } - if _, err := h.traces.WriteRoutingTrace(ctx, tr); err != nil { + id, err := h.traces.WriteRoutingTrace(ctx, tr) + if err != nil { log.Printf("routing trace: write: %v", err) + return } + // The id goes back to whoever asked for it, so /chat can offer a correction + // on the turn it is already showing (V-630). Noted on the ORIGINAL context, + // not the detached one above: the sink belongs to the caller's turn. + noteTraceID(turnCtx, id) } // wonIntent — what the winning claimant made the turn. Read from the claim diff --git a/cmd/mavend/routingtrace_test.go b/cmd/mavend/routingtrace_test.go index 2ed45fb..88aa251 100644 --- a/cmd/mavend/routingtrace_test.go +++ b/cmd/mavend/routingtrace_test.go @@ -77,3 +77,49 @@ func TestEmptyUtteranceIsNotPersisted(t *testing.T) { } var _ traceWriter = (*store.Store)(nil) + +// The trace id rides back to the caller, which is what makes a correction one +// gesture: /chat already has the id, so saying "that was wrong" costs a button +// and no lookup (V-630). +func TestTurnHandsBackItsTraceID(t *testing.T) { + h := traceHandler(t, decision.NewRing()) + h.traces = traceSink(h.dataStore) + + ctx, sink := withTraceIDSink(context.Background()) + if reply := h.handleText(ctx, "web", "сколько сейчас времени"); reply == "" { + t.Fatal("turn produced no reply") + } + id := sink.ID() + if id == 0 { + t.Fatal("no trace id came back, so /chat can offer no correction") + } + // And it names the turn that just ran, so the correction lands on the right + // utterance. + if err := h.dataStore.CorrectTurn(context.Background(), id, "query", h.now()); err != nil { + t.Fatal(err) + } + labels, err := h.dataStore.RoutingLabels(context.Background(), 5) + if err != nil { + t.Fatal(err) + } + if len(labels) != 1 || labels[0].Utterance != "сколько сейчас времени" { + t.Fatalf("labels %+v, want the turn that just ran", labels) + } +} + +// A turn nobody asked the id of costs nothing, which is the mic path. +func TestTurnWithNoSinkStillPersists(t *testing.T) { + h := traceHandler(t, decision.NewRing()) + h.traces = traceSink(h.dataStore) + + if reply := h.handleText(context.Background(), "web", "сколько сейчас времени"); reply == "" { + t.Fatal("turn produced no reply") + } + got, err := h.dataStore.RecentRoutingTraces(context.Background(), 5) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 { + t.Fatalf("persisted %d traces, want 1", len(got)) + } +} diff --git a/cmd/mavend/tick_api.go b/cmd/mavend/tick_api.go index 236b521..adb63e4 100644 --- a/cmd/mavend/tick_api.go +++ b/cmd/mavend/tick_api.go @@ -97,10 +97,19 @@ func (d *daemonAPI) Chat(ctx context.Context, conversation, text string) (ipc.Ch return ipc.ChatReply{}, errors.New("mavend: chat not available") } ctx, sink := withQuerySourceSink(ctx) + // The trace id rides back the same way (V-630), so /chat can offer a + // correction on the turn it is already showing. 0 when nothing persisted. + ctx, traces := withTraceIDSink(ctx) reply := d.chatFn(ctx, conversation, text) - return ipc.ChatReply{Reply: reply, Source: sink.Name()}, nil + return ipc.ChatReply{Reply: reply, Source: sink.Name(), TraceID: traces.ID()}, nil } +// CorrectTurn is NOT overridden here, and that is deliberate (V-630). Every other +// diagnostic on this type exists because the daemon holds something the store +// cannot answer from a table. A correction is a table, so the embedded store +// adapter is already the right answer and a second implementation here would be +// a second place for it to drift. + // MCPServers — the configured MCP servers and their health (Vikunja #251). // Empty, not an error, when the mcp block is absent: "not configured" is the // default state and the web surface renders it as such. diff --git a/cmd/mavweb/chat.go b/cmd/mavweb/chat.go index 383245c..6091f5a 100644 --- a/cmd/mavweb/chat.go +++ b/cmd/mavweb/chat.go @@ -2,12 +2,15 @@ package main import ( _ "embed" + "errors" "log" "net/http" "net/url" + "strconv" "strings" "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/router" "github.com/kami/maven/internal/webauthn" ) @@ -25,6 +28,21 @@ type chatMsg struct { // Source — the query source that claimed the turn, shown as a badge beside // the reply. Empty for a turn no source claimed (V-539). Source string + // TraceID anchors the correction gesture (V-630). Non-zero ⇒ the turn was + // persisted and can be corrected in one click. 0 ⇒ no correction is offered, + // which is honest: a box with no database has no turn to correct. + TraceID int64 + // Corrected — the owner already corrected this turn, so the page says thank + // you instead of offering the buttons again. + Corrected string +} + +// correctionTargets — the seven public intents, in the order the buttons are +// shown. Read from internal/router rather than typed out, so a new intent cannot +// exist without a way to correct a turn into it. +var correctionTargets = []router.Intent{ + router.IntentFact, router.IntentNote, router.IntentReminder, + router.IntentQuery, router.IntentAct, router.IntentChat, router.IntentSystem, } // handleChatPage renders the chat conversation page. @@ -38,12 +56,21 @@ func handleChatPage(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { msgs = append(msgs, chatMsg{Role: "user", Text: q}) } if reply := r.URL.Query().Get("r"); reply != "" { - msgs = append(msgs, chatMsg{Role: "assistant", Text: reply, Source: r.URL.Query().Get("s")}) + id, _ := strconv.ParseInt(r.URL.Query().Get("t"), 10, 64) + msgs = append(msgs, chatMsg{ + Role: "assistant", Text: reply, Source: r.URL.Query().Get("s"), + TraceID: id, Corrected: r.URL.Query().Get("c"), + }) } + // UserText rides beside the messages so the correction form can hand the + // conversation back on the redirect: this page has no session and no JS, so + // what is on screen is what the query params carry. renderPage(w, chatTmpl, struct { Error string Messages []chatMsg - }{Messages: msgs}) + Targets []router.Intent + UserText string + }{Messages: msgs, Targets: correctionTargets, UserText: r.URL.Query().Get("q")}) } // handleChatAPI processes a chat message POST and redirects back to /chat. @@ -88,5 +115,81 @@ func handleChatAPI(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, ses if reply.Source != "" { dest += "&s=" + url.QueryEscape(reply.Source) } + // The trace id rides along so the reply can carry a correction gesture + // (V-630). Absent when nothing persisted, and the page then offers none. + if reply.TraceID != 0 { + dest += "&t=" + strconv.FormatInt(reply.TraceID, 10) + } http.Redirect(w, r, dest, http.StatusSeeOther) } + +// handleCorrectAPI records that the last turn was routed wrongly (V-630). +// +// A correction is the only supervised signal this box gets, and everything else +// in the trace accumulates on its own. So the gesture has to cost nothing: one +// POST from the reply he is already looking at, carrying the trace id and +// optionally the intent it should have been. An unstated target is accepted, +// because a turn marked wrong with no target is still a usable negative. +// +// Step-up gated like POST /api/chat, and that costs the gesture nothing: he +// tapped to send the turn he is now correcting, so the session is already up. +// It is gated because trace ids are sequential integers and this writes the one +// table the routing heads (V-546) will be fitted on. A caller who can guess an +// id could otherwise mislabel turns he never corrected. +func handleCorrectAPI(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, session *webauthn.PasskeySession, requireStepUp bool) { + if r.Method != http.MethodPost { + http.Error(w, "POST only", http.StatusMethodNotAllowed) + return + } + if !requireCore(w, core, "correct") { + return + } + if !stepUpGate(w, session, requireStepUp) { + return + } + id, err := strconv.ParseInt(strings.TrimSpace(r.FormValue("trace_id")), 10, 64) + if err != nil || id <= 0 { + http.Error(w, "trace_id required", http.StatusBadRequest) + return + } + shouldBe := strings.TrimSpace(r.FormValue("should_be")) + // Only one of the seven, or nothing. Free text here would put an unroutable + // label in the one table V-632 fits prototypes from. + if shouldBe != "" && !isCorrectionTarget(shouldBe) { + http.Error(w, "should_be must be one of the seven intents", http.StatusBadRequest) + return + } + if err := core.CorrectTurn(r.Context(), id, shouldBe); err != nil { + log.Printf("correct turn %d: %v", id, err) + // A turn past the retention bound is gone, and saying so is different + // from saying the write broke. + if errors.Is(err, ipc.ErrNoSuchTrace) { + http.Error(w, "that turn is no longer stored", http.StatusNotFound) + return + } + http.Error(w, "correction failed", http.StatusBadGateway) + return + } + stamp := shouldBe + if stamp == "" { + stamp = "wrong" + } + // Back to the conversation he was in, with the turn still on screen. The + // query params carry it, so the correction is preserved by re-sending them. + dest := "/chat?q=" + url.QueryEscape(r.FormValue("q")) + + "&r=" + url.QueryEscape(r.FormValue("rep")) + "&c=" + url.QueryEscape(stamp) + if s := r.FormValue("s"); s != "" { + dest += "&s=" + url.QueryEscape(s) + } + http.Redirect(w, r, dest, http.StatusSeeOther) +} + +// isCorrectionTarget — one of the seven, and nothing else. +func isCorrectionTarget(s string) bool { + for _, t := range correctionTargets { + if string(t) == s { + return true + } + } + return false +} diff --git a/cmd/mavweb/chat.html b/cmd/mavweb/chat.html index ac35c0e..88dccc1 100644 --- a/cmd/mavweb/chat.html +++ b/cmd/mavweb/chat.html @@ -5,6 +5,21 @@
{{range .Messages}}
{{if eq .Role "user"}}you{{else}}maven{{end}}: {{.Text}}{{if .Source}} {{.Source}}{{end}}
+ {{if and (eq .Role "assistant") .TraceID}} + {{if .Corrected}} +
corrected: {{.Corrected}}
+ {{else}} +
+ + + + + + should have been: + {{range $.Targets}}{{end}} +
+ {{end}} + {{end}} {{else}}
diff --git a/cmd/mavweb/correct_test.go b/cmd/mavweb/correct_test.go new file mode 100644 index 0000000..ef33f2d --- /dev/null +++ b/cmd/mavweb/correct_test.go @@ -0,0 +1,160 @@ +package main + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/kami/maven/internal/ipc" +) + +// correctCore records the correction the handler sends. +type correctCore struct { + ipc.UnimplementedCoreAPI + traceID int64 + shouldBe string + called bool + err error +} + +func (c *correctCore) CorrectTurn(_ context.Context, traceID int64, shouldBe string) error { + c.called, c.traceID, c.shouldBe = true, traceID, shouldBe + return c.err +} + +func postCorrect(form url.Values) *http.Request { + req := httptest.NewRequest(http.MethodPost, "/api/correct", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + return req +} + +// The full gesture: wrong, and it should have been a fact. +func TestCorrectAPIWithTarget(t *testing.T) { + core := &correctCore{} + rr := httptest.NewRecorder() + handleCorrectAPI(rr, postCorrect(url.Values{ + "trace_id": {"42"}, "should_be": {"fact"}, "q": {"поужинал"}, "rep": {"поняла"}, + }), core, stepUpSession(), false) + + if rr.Code != http.StatusSeeOther { + t.Fatalf("status %d, want 303; body=%s", rr.Code, rr.Body.String()) + } + if core.traceID != 42 || core.shouldBe != "fact" { + t.Errorf("corrected trace %d to %q", core.traceID, core.shouldBe) + } + // The turn stays on screen, and the page says it was corrected. + loc := rr.Header().Get("Location") + if !strings.Contains(loc, "c=fact") || !strings.Contains(loc, "q=") { + t.Errorf("redirect %q loses the turn or the correction", loc) + } +} + +// The cheap half. A turn marked wrong with no target is still a usable negative, +// and it must not cost more to give than the full answer. +func TestCorrectAPIWithNoTarget(t *testing.T) { + core := &correctCore{} + rr := httptest.NewRecorder() + handleCorrectAPI(rr, postCorrect(url.Values{"trace_id": {"7"}}), core, stepUpSession(), false) + + if rr.Code != http.StatusSeeOther { + t.Fatalf("status %d, want 303", rr.Code) + } + if !core.called || core.shouldBe != "" { + t.Errorf("called=%v shouldBe=%q, want an untargeted negative recorded", core.called, core.shouldBe) + } + if !strings.Contains(rr.Header().Get("Location"), "c=wrong") { + t.Errorf("redirect %q does not say the turn was marked wrong", rr.Header().Get("Location")) + } +} + +// Free text here would put an unroutable label in the one table V-632 fits +// prototypes from. +func TestCorrectAPIRejectsUnknownTarget(t *testing.T) { + core := &correctCore{} + rr := httptest.NewRecorder() + handleCorrectAPI(rr, postCorrect(url.Values{"trace_id": {"7"}, "should_be": {"погода"}}), core, stepUpSession(), false) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("status %d, want 400", rr.Code) + } + if core.called { + t.Error("wrote a label for a target that is not one of the seven") + } +} + +func TestCorrectAPINeedsTraceID(t *testing.T) { + for _, form := range []url.Values{{}, {"trace_id": {"0"}}, {"trace_id": {"nope"}}} { + core := &correctCore{} + rr := httptest.NewRecorder() + handleCorrectAPI(rr, postCorrect(form), core, stepUpSession(), false) + if rr.Code != http.StatusBadRequest { + t.Errorf("form %v: status %d, want 400", form, rr.Code) + } + if core.called { + t.Errorf("form %v: reached the core", form) + } + } +} + +// A write that broke is not a turn that expired, and the two must not read the +// same to the owner deciding whether to correct again. +func TestCorrectAPIReportsFailure(t *testing.T) { + core := &correctCore{err: errors.New("disk is full")} + rr := httptest.NewRecorder() + handleCorrectAPI(rr, postCorrect(url.Values{"trace_id": {"9"}, "should_be": {"note"}}), core, stepUpSession(), false) + if rr.Code != http.StatusBadGateway { + t.Fatalf("status %d, want 502", rr.Code) + } +} + +// A trace past the retention bound is gone, and the surface says that. +func TestCorrectAPIExpiredTurn(t *testing.T) { + core := &correctCore{err: ipc.ErrNoSuchTrace} + rr := httptest.NewRecorder() + handleCorrectAPI(rr, postCorrect(url.Values{"trace_id": {"9"}, "should_be": {"note"}}), core, stepUpSession(), false) + if rr.Code != http.StatusNotFound { + t.Fatalf("status %d, want 404", rr.Code) + } +} + +func TestCorrectAPIPostOnly(t *testing.T) { + rr := httptest.NewRecorder() + handleCorrectAPI(rr, httptest.NewRequest(http.MethodGet, "/api/correct", nil), &correctCore{}, stepUpSession(), false) + if rr.Code != http.StatusMethodNotAllowed { + t.Fatalf("status %d, want 405", rr.Code) + } +} + +// Every one of the seven intents has a button, so a new intent cannot exist with +// no way to correct a turn into it. +func TestCorrectionTargetsAreTheSeven(t *testing.T) { + if len(correctionTargets) != 7 { + t.Fatalf("%d targets, want the seven public intents", len(correctionTargets)) + } + for _, want := range []string{"fact", "note", "reminder", "query", "act", "chat", "system"} { + if !isCorrectionTarget(want) { + t.Errorf("%s is not offered", want) + } + } + if isCorrectionTarget("") { + t.Error("empty is not a target: it is the absence of one, handled separately") + } +} + +// Trace ids are sequential, so a caller who cannot assert step-up must not be +// able to label a turn the owner never corrected. +func TestCorrectAPINeedsStepUp(t *testing.T) { + core := &correctCore{} + rr := httptest.NewRecorder() + handleCorrectAPI(rr, postCorrect(url.Values{"trace_id": {"9"}, "should_be": {"note"}}), core, nil, true) + if rr.Code != http.StatusForbidden { + t.Fatalf("status %d, want 403", rr.Code) + } + if core.called { + t.Error("wrote a label with no step-up") + } +} diff --git a/cmd/mavweb/main.go b/cmd/mavweb/main.go index ea913b9..22578cd 100644 --- a/cmd/mavweb/main.go +++ b/cmd/mavweb/main.go @@ -210,6 +210,7 @@ func main() { mux.HandleFunc("/routines", gatedPage(handleRoutines)) mux.HandleFunc("/api/chat", gatedPage(handleChatAPI)) mux.HandleFunc("/api/revert", gatedPage(handleRevert)) + mux.HandleFunc("/api/correct", gatedPage(handleCorrectAPI)) mux.HandleFunc("/models", func(w http.ResponseWriter, r *http.Request) { handleModels(w, r, core, swapConn, stepUpSession, *requireStepUp) }) diff --git a/cmd/mavweb/static/ui.css b/cmd/mavweb/static/ui.css index ee1eb0b..d7029c1 100644 --- a/cmd/mavweb/static/ui.css +++ b/cmd/mavweb/static/ui.css @@ -702,6 +702,11 @@ details[open] > summary { margin-bottom: var(--space-1); } .chat-form { display: flex; gap: var(--space-2); } .chat-form input { flex: 1; } .chat-scroll { max-height: 60vh; overflow-y: auto; margin-bottom: var(--space-4); } +/* The correction gesture (V-630). Wraps on a phone rather than scrolling: it is + one row of small buttons, and a gesture that has to be panned to is not one. */ +.chat-correct { display: flex; flex-wrap: wrap; align-items: center; gap: var(--space-1); + padding: 0 var(--space-3) var(--space-2); margin-top: calc(-1 * var(--space-1)); margin-bottom: var(--space-2); } +.chat-correct-label { font-size: var(--fs-xs); color: var(--text-machine); margin-left: var(--space-2); } /* ── Key-value grid ── */ .kv { display: grid; grid-template-columns: auto 1fr; gap: var(--space-1) var(--space-3); font-size: var(--fs-sm); } diff --git a/docs/plans/22-correcting-a-turn.md b/docs/plans/22-correcting-a-turn.md new file mode 100644 index 0000000..220dceb --- /dev/null +++ b/docs/plans/22-correcting-a-turn.md @@ -0,0 +1,64 @@ +# Correcting a turn + +Last verified: 06-08-2026 @ 0d5bd0a + +V-630, under V-628. Reads with `21-persisting-the-routing-trace.md`. + +## Why a gesture and not a form + +The routing trace (V-629) stores every turn. Almost all of them routed correctly, so +almost all of them teach nothing. A correction is the only high-value supervised signal +the box produces. It is also the only one that costs the owner something to give. + +So the design constraint is the cost, not the schema. One gesture beside the reply. No +form and no separate page. + +It is step-up gated like the chat POST beside it, which costs nothing: he tapped to send +the turn he is correcting. It is gated because trace ids are sequential integers, and this +is the one table the routing heads will be fitted on. + +## Two things to capture, and only one of them is required + +A correction has two halves. + +- This turn was wrong. +- It should have been *this*. + +The second is worth much more. It names which boundary moved, and it is what a fitted +head trains against. But requiring it would price out the first, and a turn marked wrong +with no target is still a usable negative. So the target is optional. The trace carries +`wrong` when he did not say. + +The target is one of the seven intents and never free text. V-632 fits prototypes from +that table. An unroutable label would enter it, and a label nothing can score is worse +than no label. + +## Where the label lives + +`routing_labels`, migration #24, keyed unique on the utterance. A second correction of +the same sentence replaces the first, because his later answer is the one he meant. + +It is a separate table from `routing_traces` on purpose. The transcript expires after 14 +days. The label does not. A label is a sentence, an intent and an encoder id. That is not +a transcript, and the reversal in doc 21 rests on the distinction. + +`was` is stored beside `should_be`. The pair is what names the confusion. A label with no +`was` cannot say which boundary moved. + +## Reach + +`CorrectTurn(traceID, shouldBe)` takes no browser and no session. The trace id rides back +on `ipc.ChatReply` through the same context sink the query source badge uses. Nothing in +the seam assumes the web. + +Only `/chat` offers the gesture today. That is a gap, named rather than closed. If the web +is the only place to correct a turn, the sample skews to whatever the owner types at. Voice +is where the hard cases are. Telegram has the obvious shape, an inline keyboard on the +reply. Voice does not. Inventing a spoken correction grammar would put a recogniser in +front of the one signal that exists to fix recognisers. Both are follow-on work. + +## What is not decided + +Whether the owner ever wants to see the labels he gave. Nothing reads the table outward +yet. `/trace` shows the ring, which is 25 turns and in memory, and a labels view is a +different page with a different question. diff --git a/internal/ipc/api.go b/internal/ipc/api.go index 113c825..db90185 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -724,8 +724,15 @@ type chatReq struct { Conversation string `json:"conversation,omitempty"` } type chatResp struct { - Reply string `json:"reply"` - Source string `json:"source,omitempty"` + Reply string `json:"reply"` + Source string `json:"source,omitempty"` + TraceID int64 `json:"trace_id,omitempty"` +} + +// correctTurnReq — the owner correcting one persisted turn (V-630). +type correctTurnReq struct { + TraceID int64 `json:"trace_id"` + ShouldBe string `json:"should_be,omitempty"` } // ChatReply — one text turn's answer plus which query source claimed it. @@ -738,6 +745,12 @@ type chatResp struct { type ChatReply struct { Reply string Source string + // TraceID is the persisted routing trace for this turn (V-629), and it is + // what makes a correction one gesture: the surface already has the id, so + // saying "that was wrong" costs a button and no lookup. 0 ⇒ nothing was + // persisted, which is a box with no database, and the surface offers no + // correction rather than a broken one. + TraceID int64 } type proposeToolReq struct { @@ -937,6 +950,12 @@ var ErrTaskDuplicate = errors.New("ipc: another live task already has this text" // is down" must not read the same to a caller deciding whether to store an id. var ErrNoEntity = errors.New("ipc: no such entity") +// ErrNoSuchTrace — the turn a correction names is not in routing_traces. Given +// a wire twin because it is the expected outcome of correcting a turn older than +// the retention bound, and "that turn is gone" and "the database is broken" must +// not read the same to the surface offering the gesture. +var ErrNoSuchTrace = errors.New("ipc: no such routing trace") + // ErrTaskResolved — a resolved task is not editable. var ErrTaskResolved = errors.New("ipc: task is resolved") diff --git a/internal/ipc/client.go b/internal/ipc/client.go index 946ed37..5cf1fc9 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -247,6 +247,8 @@ func hydrate(e *RpcError) error { return fmt.Errorf("%w: %s", ErrReminderState, e.Message) case codeToolNotFound: return fmt.Errorf("%w: %s", ErrToolNotFound, e.Message) + case codeNoSuchTrace: + return fmt.Errorf("%w: %s", ErrNoSuchTrace, e.Message) case codeUnknownMethod: return fmt.Errorf("%w: %s", ErrUnknownMethod, e.Message) case codeBadParams: @@ -661,7 +663,11 @@ func (c *Client) Chat(ctx context.Context, conversation, text string) (ChatReply if err := c.call(ctx, MethodChat, chatReq{Text: text, Conversation: conversation}, &r); err != nil { return ChatReply{}, err } - return ChatReply{Reply: r.Reply, Source: r.Source}, nil + return ChatReply{Reply: r.Reply, Source: r.Source, TraceID: r.TraceID}, nil +} + +func (c *Client) CorrectTurn(ctx context.Context, traceID int64, shouldBe string) error { + return c.call(ctx, MethodCorrectTurn, correctTurnReq{TraceID: traceID, ShouldBe: shouldBe}, nil) } func (c *Client) TickTrace(ctx context.Context) (TickTrace, error) { diff --git a/internal/ipc/coreapi.go b/internal/ipc/coreapi.go index d3d952d..cf3d5dc 100644 --- a/internal/ipc/coreapi.go +++ b/internal/ipc/coreapi.go @@ -176,6 +176,14 @@ type SystemAPI interface { // has run since the daemon started. TurnDecisions(ctx context.Context, n int) ([]TurnDecision, error) + // CorrectTurn records that one persisted turn was routed wrongly, and what + // it should have been (V-630). shouldBe empty means "wrong, target + // unstated", which is a usable negative and must not cost more to give than + // the full answer. Unlike TurnDecisions this DOES reach a table, because a + // correction is the only supervised signal the box gets and it has to + // outlive the trace that carried it. + CorrectTurn(ctx context.Context, traceID int64, shouldBe string) error + // RecentEcosystemTraces reads the ecosystem call log, which lives in its // own table so machine-rate traces never crowd out human-rate facts. RecentEcosystemTraces(ctx context.Context, n int) ([]EcosystemTrace, error) diff --git a/internal/ipc/maperr_test.go b/internal/ipc/maperr_test.go index 7bdc4b3..6ff31fb 100644 --- a/internal/ipc/maperr_test.go +++ b/internal/ipc/maperr_test.go @@ -31,6 +31,7 @@ var mapErrPairs = []struct { {"ErrReminderNotFound", store.ErrReminderNotFound, ErrReminderNotFound}, {"ErrReminderState", store.ErrReminderState, ErrReminderState}, {"ErrToolNotFound", store.ErrToolNotFound, ErrToolNotFound}, + {"ErrNoSuchTrace", store.ErrNoSuchTrace, ErrNoSuchTrace}, {"ErrTaskNoDoneWhen", store.ErrTaskNoDoneWhen, ErrTaskNoDoneWhen}, {"ErrTaskDuplicate", store.ErrTaskDuplicate, ErrTaskDuplicate}, {"ErrTaskResolved", store.ErrTaskResolved, ErrTaskResolved}, diff --git a/internal/ipc/server.go b/internal/ipc/server.go index 4d035a3..97ee006 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -514,7 +514,10 @@ var methodTable = map[Method]handlerFunc{ }), MethodChat: withParams(func(ctx context.Context, api CoreAPI, p chatReq) (chatResp, error) { reply, err := api.Chat(ctx, p.Conversation, p.Text) - return chatResp{Reply: reply.Reply, Source: reply.Source}, err + return chatResp{Reply: reply.Reply, Source: reply.Source, TraceID: reply.TraceID}, err + }), + MethodCorrectTurn: withParams(func(ctx context.Context, api CoreAPI, p correctTurnReq) (struct{}, error) { + return struct{}{}, api.CorrectTurn(ctx, p.TraceID, p.ShouldBe) }), MethodTickTrace: withoutParams(func(ctx context.Context, api CoreAPI) (TickTrace, error) { return api.TickTrace(ctx) diff --git a/internal/ipc/storeapi.go b/internal/ipc/storeapi.go index 21fd7da..a0fe3f2 100644 --- a/internal/ipc/storeapi.go +++ b/internal/ipc/storeapi.go @@ -213,6 +213,13 @@ func (a *storeAPI) TickTrace(ctx context.Context) (TickTrace, error) { return TickTrace{}, errors.New("store: tick trace not available via direct store API") } +// CorrectTurn — unlike TickTrace and TurnDecisions this one is a table, so the +// store adapter answers it for real (V-630). A correction has to land whether +// the caller reached the daemon or the store directly. +func (a *storeAPI) CorrectTurn(ctx context.Context, traceID int64, shouldBe string) error { + return mapErr(a.s.CorrectTurn(ctx, traceID, shouldBe, time.Now())) +} + // TurnDecisions — same story as TickTrace: the arbitration record is a daemon // ring, not a table, so there is nothing here to read it from (V-564). func (a *storeAPI) TurnDecisions(ctx context.Context, n int) ([]TurnDecision, error) { @@ -412,6 +419,8 @@ func mapErr(err error) error { return ErrReminderState case errors.Is(err, store.ErrToolNotFound): return ErrToolNotFound + case errors.Is(err, store.ErrNoSuchTrace): + return ErrNoSuchTrace case errors.Is(err, store.ErrTaskNoDoneWhen): return ErrTaskNoDoneWhen case errors.Is(err, store.ErrTaskDuplicate): diff --git a/internal/ipc/unimplemented.go b/internal/ipc/unimplemented.go index 69681cb..e5837bc 100644 --- a/internal/ipc/unimplemented.go +++ b/internal/ipc/unimplemented.go @@ -144,6 +144,10 @@ func (UnimplementedCoreAPI) RevertFact(ctx context.Context, key string) (int64, func (UnimplementedCoreAPI) TickTrace(ctx context.Context) (TickTrace, error) { return TickTrace{}, ErrNotImplemented } +func (UnimplementedCoreAPI) CorrectTurn(ctx context.Context, traceID int64, shouldBe string) error { + return ErrNotImplemented +} + func (UnimplementedCoreAPI) TurnDecisions(ctx context.Context, n int) ([]TurnDecision, error) { return nil, ErrNotImplemented } diff --git a/internal/ipc/wire.go b/internal/ipc/wire.go index 4b6ac78..a2058b0 100644 --- a/internal/ipc/wire.go +++ b/internal/ipc/wire.go @@ -49,6 +49,7 @@ const ( MethodRevertFact Method = "revert_fact" MethodTickTrace Method = "tick_trace" MethodTurnDecisions Method = "turn_decisions" + MethodCorrectTurn Method = "correct_turn" MethodMorningStatus Method = "morning_status" MethodMCPServers Method = "mcp_servers" MethodDayPlan Method = "day_plan" @@ -125,6 +126,7 @@ const ( codeReminderMissing = "reminder_not_found" codeReminderState = "reminder_state" codeToolNotFound = "tool_not_found" + codeNoSuchTrace = "no_such_trace" codeUnknownMethod = "unknown_method" codeBadParams = "bad_params" codeForbidden = "forbidden" @@ -160,6 +162,8 @@ func codeOf(err error) string { return codeReminderState case errors.Is(err, ErrToolNotFound): return codeToolNotFound + case errors.Is(err, ErrNoSuchTrace): + return codeNoSuchTrace case errors.Is(err, ErrUnknownMethod): return codeUnknownMethod case errors.Is(err, ErrBadParams): diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 720b822..3381064 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -330,6 +330,27 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 claims TEXT NOT NULL DEFAULT '[]' ); CREATE INDEX IF NOT EXISTS idx_routing_traces_ts ON routing_traces (ts DESC);`, + // #24 — the corrected pairs (V-630). Separate from routing_traces on + // purpose, and this is the whole retention argument: a trace is a transcript + // and expires in 14 days, while a correction is a label the owner wrote by + // hand and is the only supervised signal the box will ever get. Promoting it + // out at the moment he writes it means the label survives the transcript + // that carried it. + // + // should_be may be empty. "That was wrong" with no target is a usable + // negative and must not cost more to give than the full answer would. + // + // UNIQUE(utterance) so correcting the same sentence twice replaces the + // label rather than stacking two. His second answer is the one he meant. + `CREATE TABLE IF NOT EXISTS routing_labels ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + ts INTEGER NOT NULL, + utterance TEXT NOT NULL UNIQUE, + was TEXT NOT NULL DEFAULT '', + should_be TEXT NOT NULL DEFAULT '', + source TEXT NOT NULL DEFAULT '', + encoder_id TEXT NOT NULL DEFAULT '' + );`, } // migrate applies every migration with a number greater than the DB's current diff --git a/internal/store/routinglabels.go b/internal/store/routinglabels.go new file mode 100644 index 0000000..b21c584 --- /dev/null +++ b/internal/store/routinglabels.go @@ -0,0 +1,111 @@ +package store + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" + "time" +) + +// ErrNoSuchTrace — the trace the correction names is gone or never existed. +// Held apart from a write failure because it is the expected outcome of +// correcting a turn older than the 14-day bound, and the surface should say that +// rather than report a broken database. +var ErrNoSuchTrace = errors.New("no such routing trace") + +// RoutingLabel is one correction: what he said, what she made of it, and what it +// should have been. It is the only supervised signal in the box, so it outlives +// the trace it came from (V-630, docs/plans/22-correcting-a-turn.md). +type RoutingLabel struct { + ID int64 `json:"id"` + Ts time.Time `json:"ts"` + Utterance string `json:"utterance"` + // Was is the intent the cascade chose. Kept beside the target because the + // pair is what names the confusion, and a label with no "was" cannot say + // which boundary moved. + Was string `json:"was"` + // ShouldBe is the owner's target, and may be empty. "That was wrong, I am + // not going to tell you what it was" is a usable negative, and requiring the + // target would cost the cheap half of the gesture. + ShouldBe string `json:"should_be"` + Source string `json:"source"` + EncoderID string `json:"encoder_id"` +} + +// CorrectTurn records the owner's correction of one persisted turn. It promotes +// the pair into routing_labels and stamps the trace, both in one transaction: +// a stamped trace with no label would lose the signal when the trace expires, +// and a label with no stamp would let the same turn be corrected twice. +// +// shouldBe empty is allowed and means "wrong, target unstated". +func (s *Store) CorrectTurn(ctx context.Context, traceID int64, shouldBe string, now time.Time) error { + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("correct turn: begin: %w", err) + } + defer func() { _ = tx.Rollback() }() + + var utterance, was, source, encoderID string + err = tx.QueryRowContext(ctx, ` + SELECT utterance, intent, source, encoder_id FROM routing_traces WHERE id = ?`, + traceID).Scan(&utterance, &was, &source, &encoderID) + if errors.Is(err, sql.ErrNoRows) { + return ErrNoSuchTrace + } + if err != nil { + return fmt.Errorf("correct turn: read trace: %w", err) + } + + shouldBe = strings.TrimSpace(shouldBe) + if _, err := tx.ExecContext(ctx, ` + INSERT INTO routing_labels (ts, utterance, was, should_be, source, encoder_id) + VALUES (?,?,?,?,?,?) + ON CONFLICT(utterance) DO UPDATE SET + ts = excluded.ts, was = excluded.was, should_be = excluded.should_be, + source = excluded.source, encoder_id = excluded.encoder_id`, + now.UnixMilli(), utterance, was, shouldBe, source, encoderID); err != nil { + return fmt.Errorf("correct turn: write label: %w", err) + } + // The stamp is what the trace itself carries: "corrected", or the target he + // gave. It expires with the trace, and that is fine — the label above is the + // durable half. + stamp := shouldBe + if stamp == "" { + stamp = "wrong" + } + if _, err := tx.ExecContext(ctx, + `UPDATE routing_traces SET correction = ? WHERE id = ?`, stamp, traceID); err != nil { + return fmt.Errorf("correct turn: stamp trace: %w", err) + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("correct turn: commit: %w", err) + } + return nil +} + +// RoutingLabels returns the newest n corrections, newest first. Nothing prunes +// them: 31 modes and 9 of them with no example at all is the problem this table +// exists to solve, and a label is a few dozen bytes. +func (s *Store) RoutingLabels(ctx context.Context, n int) ([]RoutingLabel, error) { + rows, err := s.db.QueryContext(ctx, ` + SELECT id, ts, utterance, was, should_be, source, encoder_id + FROM routing_labels ORDER BY id DESC LIMIT ?`, n) + if err != nil { + return nil, fmt.Errorf("routing labels: %w", err) + } + defer rows.Close() + var out []RoutingLabel + for rows.Next() { + var l RoutingLabel + var tsMilli int64 + if err := rows.Scan(&l.ID, &tsMilli, &l.Utterance, &l.Was, &l.ShouldBe, + &l.Source, &l.EncoderID); err != nil { + return nil, err + } + l.Ts = time.UnixMilli(tsMilli).UTC() + out = append(out, l) + } + return out, rows.Err() +} diff --git a/internal/store/routinglabels_test.go b/internal/store/routinglabels_test.go new file mode 100644 index 0000000..c858569 --- /dev/null +++ b/internal/store/routinglabels_test.go @@ -0,0 +1,118 @@ +package store + +import ( + "context" + "errors" + "testing" + "time" +) + +func seedTrace(t *testing.T, s *Store, utterance, intent string, now time.Time) int64 { + t.Helper() + id, err := s.WriteRoutingTrace(context.Background(), RoutingTrace{ + Ts: now, Utterance: utterance, Intent: intent, Source: "tap:text", EncoderID: "e5-small", + }) + if err != nil { + t.Fatal(err) + } + return id +} + +// The label carries the pair, and it is what survives the transcript. +func TestCorrectTurnPromotesTheLabel(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 6, 12, 0, 0, 0, time.UTC) + id := seedTrace(t, s, "поужинал", "query", now) + + if err := s.CorrectTurn(ctx, id, "fact", now); err != nil { + t.Fatal(err) + } + labels, err := s.RoutingLabels(ctx, 10) + if err != nil { + t.Fatal(err) + } + if len(labels) != 1 { + t.Fatalf("got %d labels, want 1", len(labels)) + } + l := labels[0] + if l.Utterance != "поужинал" || l.Was != "query" || l.ShouldBe != "fact" { + t.Errorf("label %+v: the pair is what names the confusion", l) + } + if l.EncoderID != "e5-small" { + t.Errorf("encoder_id %q: a fitted distance means nothing without the body", l.EncoderID) + } + // The trace is stamped too, so the same turn cannot be corrected twice into + // two labels without the surface knowing. + traces, err := s.RecentRoutingTraces(ctx, 10) + if err != nil { + t.Fatal(err) + } + if traces[0].Correction != "fact" { + t.Errorf("trace correction %q, want fact", traces[0].Correction) + } +} + +// "Wrong, and I am not telling you what it was" is the cheap half of the +// gesture, and it must not cost more than the full answer. +func TestCorrectTurnWithNoTarget(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 6, 12, 0, 0, 0, time.UTC) + id := seedTrace(t, s, "закрывай", "act", now) + + if err := s.CorrectTurn(ctx, id, " ", now); err != nil { + t.Fatal(err) + } + labels, err := s.RoutingLabels(ctx, 10) + if err != nil { + t.Fatal(err) + } + if len(labels) != 1 || labels[0].ShouldBe != "" { + t.Fatalf("labels %+v: an untargeted negative is still a label", labels) + } + traces, _ := s.RecentRoutingTraces(ctx, 10) + if traces[0].Correction != "wrong" { + t.Errorf("trace correction %q, want wrong", traces[0].Correction) + } +} + +// His second answer is the one he meant, so a re-correction replaces. +func TestCorrectTurnTwiceReplaces(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Date(2026, 8, 6, 12, 0, 0, 0, time.UTC) + first := seedTrace(t, s, "поужинал", "query", now) + second := seedTrace(t, s, "поужинал", "chat", now.Add(time.Minute)) + + if err := s.CorrectTurn(ctx, first, "note", now); err != nil { + t.Fatal(err) + } + if err := s.CorrectTurn(ctx, second, "fact", now.Add(time.Minute)); err != nil { + t.Fatal(err) + } + labels, err := s.RoutingLabels(ctx, 10) + if err != nil { + t.Fatal(err) + } + if len(labels) != 1 { + t.Fatalf("got %d labels for one sentence, want 1", len(labels)) + } + if labels[0].ShouldBe != "fact" || labels[0].Was != "chat" { + t.Errorf("label %+v, want the second correction", labels[0]) + } +} + +// A turn past the 14-day bound cannot be corrected, and the surface has to be +// able to say that rather than report a broken database. +func TestCorrectTurnUnknownTrace(t *testing.T) { + s := newTestStore(t) + err := s.CorrectTurn(context.Background(), 999, "fact", time.Now()) + if !errors.Is(err, ErrNoSuchTrace) { + t.Fatalf("err %v, want ErrNoSuchTrace", err) + } + labels, _ := s.RoutingLabels(context.Background(), 10) + if len(labels) != 0 { + t.Errorf("wrote %d labels for a trace that does not exist", len(labels)) + } +}