diff --git a/CLAUDE.md b/CLAUDE.md index 26b9bf5..b8ec08b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -90,8 +90,11 @@ fallback. Any LLM error falls through to the classifier so a turn never breaks o Measured on the 77-case RU fixture (`MODEL-BAKEOFF-31-07-2026.md`): the classifier scores 36.8% full accuracy at p50 31ms; Qwen3-1.7B scores 67.5% intent-only / 72.7% through the cascade at p50 ≈2.7s. Accuracy roughly doubled, latency is ~90× worse, and that trade was -accepted deliberately. Still open: `Confidence: 1.0` is hardcoded in `llmrouter.go`, so the -LLM path never asks for clarification (6/6 refusal cases missed) — Vikunja #359. +accepted deliberately. `Confidence: 1.0` used to be hardcoded in `llmrouter.go`, so the LLM +path could never ask for clarification (6/6 refusal cases missed on the fixture) — Vikunja +#359. Fixed 31-07-2026 with structural signal (single-token utterance, keyless fact, act with +no allowlisted fn) feeding the same stage-3 gate the classifier path already had; the fixture +re-run to confirm the 6/6 moves is still outstanding, see `gateLLMDecision` in `router.go`. ## LLM output contract diff --git a/internal/router/llmrouter.go b/internal/router/llmrouter.go index 33c47c5..eec8014 100644 --- a/internal/router/llmrouter.go +++ b/internal/router/llmrouter.go @@ -118,6 +118,39 @@ const routeRepeatPenalty = 1.15 // Route return ok=false so the caller drops to the classifier cascade. const routeIntentUnknown = "unknown" +// llmFullConfidence / llmThinConfidence — Vikunja #359. Confidence used to be +// hardcoded to 1.0 for every LLM decision, so the stage-3 gate in router.go +// never had anything to bite on and the LLM path could never produce a +// Clarify: on the 77-case RU fixture, 6/6 want_clarify cases were missed by +// EVERY model in the 31-07-2026 bake-off (0.8B through 2B) — proof this was a +// code bug, not a capability ceiling. +// +// The fix does not touch the prompt (routeSystem is under +// llm/check_prompt_parity.py in the training workspace; changing its text +// creates a parity break that has to be fixed there too — see Vikunja #362). +// Instead it reads structural signal that is already free: +// - a single-token utterance is thin evidence for anything a grammar +// didn't already catch at stage 0 — "вода" and "бэкап" alone don't say +// fact-vs-query or act-vs-report; +// - a fact with no key, or an act that never resolves to an allowlisted fn +// (checked in router.go, after slot-fill has had its say), is a decision +// with a hole in the one slot that makes it actionable. +// +// A model self-reporting confidence in the JSON was considered and rejected: +// a sub-2B is not calibrated (nothing stops it saying "confident" on exactly +// the cases it gets wrong today), and true logprobs would need a response +// field internal/llm.Client's Complete does not currently return — see +// internal/llm/client.go. +// +// llmThinConfidence sits below config.DefaultRouterThreshold (0.55) so the +// existing stage-3 gate in Router.Route treats it exactly like a low-scoring +// classifier result — same lane, same daemon-side clarify machinery +// (cmd/mavend/clarify.go), no new consumer to build. +const ( + llmFullConfidence = 1.0 + llmThinConfidence = 0.3 +) + type routeAction struct { Intent string `json:"intent"` Key string `json:"key"` @@ -160,7 +193,15 @@ func (lr *LLMRouter) Route(ctx context.Context, utterance string, now time.Time) if a.Intent == routeIntentUnknown { return Decision{}, false, nil } - d := Decision{Utterance: utterance, Stage: 1, Confidence: 1.0} + d := Decision{Utterance: utterance, Stage: 1, Confidence: llmFullConfidence} + // A single-token utterance is thin evidence: the model had nothing to + // disambiguate on ("вода" is a fact-or-query coin flip, "бэкап" an + // act-or-report one) and stage 0 would already have won on anything + // that pattern-matches cleanly. Flag it now; router.go's stage-3 gate + // (Router.Route) decides whether that trips Clarify. + if len(strings.Fields(utterance)) <= 1 { + d.Confidence = llmThinConfidence + } switch Intent(a.Intent) { case IntentFact: d.Intent = IntentFact diff --git a/internal/router/llmrouter_test.go b/internal/router/llmrouter_test.go index 658dab7..49279c1 100644 --- a/internal/router/llmrouter_test.go +++ b/internal/router/llmrouter_test.go @@ -258,4 +258,101 @@ func TestLLMFactGetsKeyFromParser(t *testing.T) { if !d.Slots.HasKey || d.Slots.Key != "water" { t.Fatalf("want key=water, got %+v", d.Slots) } + if d.Clarify { + t.Fatalf("the parser resolved the key, this must not clarify: %+v", d) + } +} + +// --- confidence / stage-3 gate on the LLM path (Vikunja #359) ----------------- + +// A single-token utterance is thin evidence on its own — "вода" alone is a +// fact/query coin flip. The gate must ask rather than guess confidently. +func TestLLMRouterSingleTokenTripsClarify(t *testing.T) { + r := newLLMTestRouter(t, `{"intent":"query","text":"вода"}`) + d, err := r.Route(context.Background(), "вода", refNow()) + if err != nil { + t.Fatalf("route: %v", err) + } + if !d.Clarify { + t.Fatalf("a bare single-token decision must clarify, got %+v", d) + } +} + +// A multi-word utterance with a clean answer must not be punished — the +// whole point is not trading the confident cases away for clarify coverage. +func TestLLMRouterMultiWordStaysConfident(t *testing.T) { + r := newLLMTestRouter(t, `{"intent":"reminder","text":"позвонить маме"}`) + d, err := r.Route(context.Background(), "напомни позвонить маме", refNow()) + if err != nil { + t.Fatalf("route: %v", err) + } + if d.Clarify { + t.Fatalf("a clean multi-word decision must not clarify: %+v", d) + } + if d.Confidence != llmFullConfidence { + t.Fatalf("want full confidence, got %v", d.Confidence) + } +} + +// "бэкап" alone: the model guesses act, but nothing on the allowlist matches +// "бэкап" as a verb — that must not fire a tool blind. +func TestLLMRouterActWithoutFnTripsClarify(t *testing.T) { + r := newLLMTestRouter(t, `{"intent":"act","verb":"бэкап"}`) + d, err := r.Route(context.Background(), "бэкап сделай пожалуйста расписание", refNow()) + if err != nil { + t.Fatalf("route: %v", err) + } + if d.Slots.HasFn { + t.Fatalf("test setup drifted: %q now resolves to an fn", d.Slots.Fn) + } + if !d.Clarify { + t.Fatalf("an unresolved act must clarify rather than guess: %+v", d) + } +} + +// An act that DOES resolve to an allowlisted fn must stay confident even +// though its own verb is single-word-ish in spirit — guard against the fn +// check firing on the happy path. +func TestLLMRouterActWithFnStaysConfident(t *testing.T) { + r := newLLMTestRouter(t, `{"intent":"act","verb":"restart nginx"}`) + d, err := r.Route(context.Background(), "слушай, restart nginx пожалуйста", refNow()) + if err != nil { + t.Fatalf("route: %v", err) + } + if !d.Slots.HasFn { + t.Fatalf("test setup drifted, want fn resolved: %+v", d.Slots) + } + if d.Clarify { + t.Fatalf("a resolved act must not clarify: %+v", d) + } +} + +// A fact where NEITHER the model NOR the deterministic parser can name a key +// must clarify instead of silently writing under an empty/guessed key. +func TestLLMRouterFactWithoutKeyTripsClarify(t *testing.T) { + r := newLLMTestRouter(t, `{"intent":"fact","value":"что-то"}`) + d, err := r.Route(context.Background(), "у меня какая-то фигня случилась вот прямо только что", refNow()) + if err != nil { + t.Fatalf("route: %v", err) + } + if d.Slots.HasKey { + t.Fatalf("test setup drifted: parser now resolves a key for this utterance") + } + if !d.Clarify { + t.Fatalf("a keyless fact must clarify rather than guess: %+v", d) + } +} + +// The whole point of #359: the classifier cascade cannot be traded away for +// clarify coverage. A multi-word fact the parser CAN key must stay confident +// through the full Router.Route path, not just the raw LLMRouter. +func TestRouterLLMFactWithResolvedKeyStaysConfident(t *testing.T) { + r := newLLMTestRouter(t, `{"intent":"fact","text":"я выпил воду"}`) + d, err := r.Route(context.Background(), "я выпил воду", refNow()) + if err != nil { + t.Fatalf("route: %v", err) + } + if d.Clarify { + t.Fatalf("a fact the parser could key must not clarify: %+v", d) + } } diff --git a/internal/router/router.go b/internal/router/router.go index 4b8fd18..bf9b430 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -89,6 +89,7 @@ func (r *Router) Route(ctx context.Context, utterance string, now time.Time) (De if d, ok, err := r.llm.Route(ctx, utterance, now); err == nil && ok { d.Utterance = utterance r.fillSlots(ctx, &d, now) + r.gateLLMDecision(&d) return d, nil } else if err != nil { log.Printf("router: llm route fell back to classifier: %v", err) @@ -152,6 +153,35 @@ func (r *Router) fillSlots(ctx context.Context, d *Decision, now time.Time) { // Stage stays 1: it says who decided the route, and that was the LLM. } +// gateLLMDecision — stage 3 for the LLM path (Vikunja #359). This used to be +// the classifier's job alone (see the threshold check at the bottom of +// Route): the LLM branch returned straight from fillSlots and never touched +// r.threshold at all, so a hardcoded Confidence: 1.0 in llmrouter.go could +// never gate. Two more structural holes are checked here, after fillSlots +// has had a chance to fill them from the deterministic parsers — checking +// before fillSlots would flag e.g. every keyless fact the fact parser goes +// on to resolve (TestLLMFactGetsKeyFromParser): +// - a fact with no key even after the parser tried — nothing to write, or +// worse, a confident write under the wrong key; +// - an act that never resolved to an allowlisted fn — a confident guess +// here means either silently doing nothing or, if the daemon is lax, +// running something never on the allowlist. Don't guess; ask. +// +// Anything below threshold gets the exact same Clarify=true treatment the +// classifier path already produces — same field, same daemon-side consumer +// (cmd/mavend/clarify.go), nothing new to wire. +func (r *Router) gateLLMDecision(d *Decision) { + if d.Intent == IntentFact && !d.Slots.HasKey && d.Confidence > llmThinConfidence { + d.Confidence = llmThinConfidence + } + if d.Intent == IntentAct && !d.Slots.HasFn && d.Confidence > llmThinConfidence { + d.Confidence = llmThinConfidence + } + if d.Confidence < r.threshold { + d.Clarify = true + } +} + // CorrectMisroute — the user corrected a bad classification. Appends a new // example for the corrected intent (append-only — grows the classifier, no // retrain). Same shape as nudges.outcome tuning cooldowns: more reliable over