Add the pending-question data layer for clarifying questions #1
Closed
claude
wants to merge 0 commits from
overnight/clarify-data-layer into master
pull from: overnight/clarify-data-layer
merge into: kami:master
kami:master
kami:task/725-capability-ledger-and-empirical-baseline
kami:task/692-heads-path-may-equal-model-path-and-noth
kami:task/694-staticcheck-and-deadcode-are-still-not-i
kami:task/682-go-1-25-5-and-x-text-0-14-0-carry-20-rea
kami:task/674-caveats
kami:task/673-mavgpud-serves-the-model-to-the-whole-la
kami:task/487-capture-device-doc
kami:task/487-capture-device
kami:task/487-wake-word-deploy
kami:task/487-wake-word-threshold
kami:task/487-wake-word-stage-two
kami:task/671-mavwaked-registers-as-a-voice-consumer-i
kami:task/670-cut-claude-md-to-200-lines
kami:task/515-deploy-mavwaked-workpc
kami:task/669-prune-claude-md
kami:task/668-e4b-phrasing
kami:task/668-title-capital
kami:task/668-kiwix-answers-a-question-it-cannot-answe
kami:task/666-only-a-stage-0-grammar-may-take-the-pers
kami:task/487-mavwaked-has-no-wake-word-only-an-energy
kami:task/486-deploy-the-workstation-transcriber
kami:task/486-move-stt-and-tts-to-the-workstation-wher
kami:task/665-crisperwhisper-2-russian
kami:task/664-routing-heads-in-go
kami:task/662-usage-harness-source-badge
kami:task/661-post-merge-usage-rerun
kami:task/661-routing-heads-step-3-train-the-multi-hea
kami:task/660-router-prompt-destination
kami:task/659-destination-fixture
kami:task/655-query-source-is-a-routing-decision-made
kami:task/654-a-pending-clarify-has-no-way-out-neither
kami:task/654-week-of-usage-eval-docs
kami:task/649-needs-kami-telegram-is-the-only-reach-an
kami:task/643-memorystore-search-decodes-and-unmarshal
kami:task/641-two-maps-grow-for-the-process-lifetime-w
kami:task/644-mavcaldav-is-built-documented-as-running
kami:task/642-the-store-caps-sqlite-at-one-connection
kami:task/647-factenrichmentworker-walks-the-pending-q
kami:task/646-v-637-follow-up-telegram-intake-has-no-d
kami:task/638-no-deadline-survives-the-turn-path-from
kami:task/637-inbound-telegram-turns-and-corrections-f
kami:task/636-correcting-a-turn-from-telegram-and-from
kami:task/634-an-act-alias-resolves-the-verb-but-not-t
kami:task/630-one-gesture-correction-on-chat-v-628
kami:task/629-persist-the-routing-trace-and-record-it
kami:task/631-mode-inventory-written-from-the-handlers
kami:task/586-defaultfactparser-uses-hand-written-russ
kami:task/633-reconcile-the-seed-labels-with-the-handl
kami:task/627-reminder-verbs-has-no-alarm-verb-so-an-a
kami:task/626-the-classifier-seeds-teach-an-older-inte
kami:task/546-route-with-a-fine-tuned-e5-small-instead
kami:task/586-measure-the-fact-parser
kami:fix/gofmt-ecosystem-acts
kami:task/584-media-store-a-failed-write-leaks-its-bud
kami:task/518-no-write-path-for-a-backdated-event-so-t
kami:task/287-qa-voice-session-quality-polish
kami:task/492-qa-plan-reconcile
kami:task/530-sweep-tail-four-files-the-russian-sweep
kami:task/405-score-how-often-a-real-utterance-reaches
kami:task/529-money-and-list-pick-a-mechanism
kami:task/528-sweep-tail-the-three-files-on-467
kami:task/527-embedder-open-set-phrasings-stop-being-r
kami:task/526-morphology-a-dictionary-answers-the-gram
kami:task/525-lexicons-the-finite-russian-sets-move-to
kami:task/524-entity-reference-ask-nexus-about-every-l
kami:task/523-risk-tiers-take-hexis-s-tier-for-a-hexis
kami:task/521-review-pr-111-query-strings-declension-h
kami:task/491-llama-server-core-dumps-on-every-sigterm
kami:task/479-bug-an-unconfigured-capability-does-not
kami:task/467-bug-spoken-task-capture-is-dead-the-rout
kami:task/463-deploy-mavwaked-and-mavenclient-run-nowh
kami:task/480-hearing-no-shipped-client-can-start-a-re
kami:task/432-ambient-calendar-intake-is-fragile-and-p
kami:task/431-board-surface-maven-holds-the-work-board
kami:task/433-reactivehandler-has-30-fields-and-is-pas
kami:task/371-swap-the-embedder-for-an-asymmetric-retr
kami:task/408-review-31-07-split-the-30-method-coreapi
kami:task/410-review-31-07-hand-rolled-string-enums-st
kami:task/423-review-pr50-split-internal-ipc-server-go
kami:task/422-review-pr50-split-cmd-mavend-tick-go-860
kami:task/409-review-31-07-finish-moving-mavweb-markup
kami:task/482-ambient-ingest-reads-a-notification-s-ti
kami:task/444-kuma-a-fact-per-monitor-so-she-can-name
kami:task/452-capability-model-homelab-docker-restart
kami:task/449-destructive-confirm-policy-risk-tiers-no
kami:task/453-grocery-list-items-table-fourth-append-o
kami:task/399-run-the-persona-checks-inside-the-daemon
kami:task/448-bounded-follow-up-state-pending-candidat
kami:task/455-conversation-repair-name-the-misroute-co
kami:task/454-go-mod-tidy
kami:task/458-pronunciation-dictionary-for-piper
kami:task/456-command-history-read-only-query-over-exi
kami:task/457-clarification-templates-for-the-router-s
kami:task/474-query-source-ordering-feeds-and-calendar
kami:task/469-reminders-spelled-out-times-fail-the-bod
kami:task/475-bug-the-praxis-attention-capability-is-u
kami:task/481-bug-a-transient-complaint-is-stored-as-a
kami:task/476-bug-the-router-transliterates-latin-enti
kami:task/385-decide-whether-a-parked-clarify-question
kami:task/377-backfill-routines
kami:task/421-weather-geocoder
kami:task/390-no-read-path-for-delivery-attempts
kami:task/386-recall-fixture-filler-note-ids
kami:task/473-bug-morning-item-has-no-required-flag
kami:task/465-bug-make-simulate-routes-with-an-empty
kami:task/467-bug-spoken-task-capture-is-dead
kami:task/466-bug-a-pending-clarify-is-global-so-one-u
kami:task/468-bug-pattern-detect-has-no-minimum-interv
kami:task/462-bug-checkfeminine-flags-second-person-ma
kami:task/443-safekey-drops-cyrillic-so-russian-calend
kami:task/471-bug-agendaquerygrammars-covers-today-but
kami:task/383-slottext-in-clarify-answer-would-clobber
kami:task/323-qa-phraser-coverage-is-65-3-but-the-llam
kami:task/498-bug-and-x-reach-the-model-with-no-determ
kami:task/506-strings-family-6-summaries-and-reports-i
kami:task/504-strings-family-4-act-and-smart-home-repl
kami:task/503-strings-family-3-query-answers-and-gaps
kami:task/502-strings-family-2-capture-acknowledgement
kami:task/501-strings-family-1-phrasing-fallbacks-into
kami:task/397-phrasechat-and-phrasequery-hide-model-fa
kami:task/396-the-reply-path-can-t-be-tested-llmreplie
kami:task/496-recall-a-cross-language-question-loses-i
kami:task/495-bug-x-escapes-the-personal-boundary-and
kami:task/499-llama-server-holds-7-9gb-rss-for-a-1-1gb
kami:task/470-bug-a-question-writes-invented-knowledge
kami:task/493-bug-the-memory-index-stores-the-raw-utte
kami:task/490-name-the-gap-world-questions-through-the
kami:task/485-run-the-big-model-on-the-workstation-wit
kami:task/489-workstation-deploy-mavgpud-on-workpc-and
kami:task/488-workstation-a-supervisor-that-keeps-llam
kami:task/483-docs-offload-design
kami:task/483-design-offload-ml-to-the-workstation-kee
kami:task/459-docs-refresh-the-qa-plan-against-the-liv
kami:task/446-doc-reorg-tier-the-tree-retire-the-three
kami:fix/367-voice-parks-routine-accept
kami:task/365-dialogue-slots-and-router-slots-are-hand
kami:task/364-snooze-does-nothing-at-runtime-the-gate
kami:task/447-retire-progress-md-the-backlog-and-the-f
kami:task/445-session-workflow
kami:overnight/eco-versioned-traces
kami:overnight/eco-entity-refs
kami:overnight/eco-degraded-suite
kami:overnight/netscan
kami:overnight/smarthome
kami:overnight/replay-simulator
kami:overnight/event-envelope
kami:overnight/coldstart-unlock
kami:overnight/voice-barge-in
kami:overnight/stt-golden-audio
kami:overnight/senses-speaker
kami:overnight/senses-hearing
kami:overnight/senses-media-vision
kami:overnight/mcp-tools
kami:overnight/mcp-client
kami:overnight/self-update
kami:overnight/model-swap
kami:overnight/web-crawler
kami:overnight/rss-feeds
kami:overnight/email-poller
kami:overnight/email-extract
kami:overnight/email-imap
kami:overnight/money-zenmoney
kami:overnight/task-priority
kami:overnight/task-capture
kami:overnight/behavior-profile
kami:overnight/day-plan
kami:overnight/ambient-calendar
kami:overnight/local-calendar
kami:overnight/memory-eval
kami:overnight/proactive-proposals
kami:overnight/split-voice-quiet
kami:overnight/nginx-maven-block
kami:overnight/stepup-chat-surface
kami:integration/small-batch
kami:docs/fix-drift
kami:fix/ru-wording
kami:integration/jul31
kami:overnight/resident-1.7b
kami:overnight/nudge-templates
kami:overnight/kiwix-rewrite
kami:overnight/eval-writeup
kami:overnight/fix-truncation
kami:overnight/kiwix-client
kami:overnight/ru-prompts
kami:overnight/external-data
kami:overnight/phrasing-grammar
kami:overnight/talk-eval
kami:overnight/prompt-context
kami:overnight/prompt-address
kami:overnight/eval-label-kill
kami:overnight/delivery-boundary
kami:overnight/address-check
kami:overnight/system-replies-pr
kami:overnight/clock-intent-pr
kami:overnight/embedder-backfill-pr
kami:overnight/embedder-marker-pr
kami:overnight/note-recall-pr
kami:overnight/thinking-off-pr
kami:overnight/dialogue-persist-pr
kami:overnight/persona-2p-pr
kami:overnight/clarify-expiry-pr
kami:overnight/clarify-rework
kami:overnight/phrasing
kami:overnight/bakeoff
kami:overnight/recall-margin
kami:overnight/router-on
kami:overnight/slot-extract
kami:overnight/embedder-e5
kami:overnight/router-refusal
kami:overnight/eval-rerun
kami:overnight/eval-harnesses
kami:overnight/eval-rerun-base
kami:overnight/fmt-gate
kami:overnight/routines-fire
kami:overnight/router-prompt
kami:overnight/away-leak
kami:overnight/recall-eval
kami:overnight/snooze-works
kami:overnight/clarify-wiring
kami:overnight/delivery-tests
kami:overnight/routine-accept
kami:overnight/llm-router-flag
kami:overnight/loop-rule-tests
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Delete Branch "overnight/clarify-data-layer"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Groundwork so Maven can ask one clarifying question and then use your answer. Data layer only — nothing is wired up yet, so this changes no behaviour.
The problem. When the router is not confident it sets
Decision.Clarifyand Maven replies "не разобрала". Your next sentence is then parsed from scratch, so an answer to her question cannot finish the original request. There are already three yes/no confirm slots invoice.go, but nothing for an open question.What this adds.
internal/dialogue/clarify.go:PendingQuestion— the intent she already guessed, the slots she already filled, which slots are still missing, and your original words.ClarifyStore— same shape and locking as the existingSessionStore, 90s TTL to matchconfirmTTL.Answer()— fills only the missing slots, never overwrites one that was already filled.MaxAttempts = 1. She asks once. If the answer still does not resolve it she drops the request. DESIGN.md says she is not a nag, so the number is 1 on purpose.Also in here (the small first commit).
dialogue.Slotswas missing theValuefield thatrouter.Slotshas, so a clarifying answer for a fact payload had nowhere to land. Added it.What to look at. Whether
MaxAttempts = 1is the behaviour you want, and whetherAnswer()'s never-overwrite rule reads right.TODO comments mark the three places the next commits hook in: storing a question, consuming the answer, and phrasing the question.
Follow-up worth a task:
dialogue.Slotsandrouter.Slotsare two hand-maintained copies of the same struct, and they had already drifted. Not refactored here.@@ -0,0 +17,4 @@SlotText Slot = "text" // Slots.Text)// MaxAttempts is 1 because Maven is not a nag (DESIGN.md § Non-goals). She asksI don't think that's how she supposed to operate. it's too... ignorant, I'd say. MaxAttempts should be higher - maybe 3 or 5. and if she fails - she tells about it and asks to try again with different request.
@@ -0,0 +28,4 @@type PendingQuestion struct {Intent Intent // what the router already guessedSlots Slots // what it already filledMissing []Slot // what is still empty, in the order to ask aboutam I getting it right?
we try to fill in all the slots: time, kv, fn, text, etc.
and we bash Maven's head against the user until he clears all of them?
that's stupid.
@@ -0,0 +101,4 @@}// Answer merges the slots parsed from the user's answer into the parked ones.// Only the slots listed in Missing are filled, and an already filled slot is@@ -0,0 +146,4 @@}// StillMissing lists the slots that are empty in s, out of the ones asked for.// The caller uses it to decide between acting and dropping the request.so, when user answered, but Maven didn't get it fully - we just ignore the user?
that's stupid.
@@ -0,0 +158,4 @@t.Run(tc.name, func(t *testing.T) {q := &PendingQuestion{Slots: tc.parked, Missing: tc.missing, Asked: base}got := q.Answer(tc.text, tc.answer)if got.Time != tc.want.Time || got.HasTime != tc.want.HasTime ||wow. why not 100 lines of if X != Y || Z != A?
@@ -0,0 +145,4 @@want: Slots{Text: "купить хлеб"},},{name: "empty answer leaves the slot missing",this one feels off.
if I get it right:
that's. stoo. pid.
You're right, and five of your six comments are the same bug wearing different hats. I'm fixing it — replies in order.
1. MaxAttempts should be 3 or 5, and she should say so when she gives up.
Agreed, and my justification for
1was wrong. I wrote "Maven is not a nag" in that comment, but not a nag is about her interrupting you unprompted. This is a conversation you started. Asking a second question about something you just asked her to do isn't nagging, it's listening. I conflated the two.Going to 3. Not 5, because after three failed attempts the problem is usually that she misheard the whole thing, not that one slot is missing — at that point re-asking is worse than admitting it. It'll be a config value, so change it if 3 feels wrong in use.
And on giving up: she'll say she didn't get it and ask you to say it differently, instead of going quiet.
2. "we bash Maven's head against the user until he clears all slots?"
No — but I see why it reads that way, and the code doesn't make it clear.
Missingisn't "every slot"; it's only the slots the router left empty for that intent. A reminder needs a time and a text, and if the router already got the text,Missingis just[time]— one question. In practice it's almost always one, occasionally two.The irony is the current code has the opposite problem from the one you're worried about: it asks once, ever, then drops it.
3. "already filled slot is never overwritten — can't that be controversial? user could've changed his mind."
Fair, and I'll change it. Right now if you say "в три" and then "нет, в пять", the second one is thrown away silently. Silently discarding something you just said is the worst option available. If your answer clearly names a value for a slot, that value wins.
The one thing I want to keep separate: correcting something later ("вообще-то перенеси на пять") should be its own turn, not a clarify answer. That's a different path and I don't want to blur them.
4. "when the user answered but Maven didn't get it fully, we just ignore him?"
Yes, today. That's the same MaxAttempts=1 bug. With three attempts she asks about whatever is still missing instead of dropping the whole thing.
5. "wow. why not 100 lines of if X != Y || Z != A?"
Deserved. That eight-clause condition is unreadable and it silently won't cover a new field when
Slotsgrows. Replacing it with a single struct comparison.6. The "не знаю" case.
Your reading is exactly right, and yes, that's stupid. You say "напомни", she asks when, you say "не знаю", and she drops it without a word — you're left thinking she's handling it. With the fix she'll tell you she can't set it without a time, so you know nothing happened.
I'll push these as a follow-up PR rather than amending this one, so the data layer stays reviewable on its own. Ignore the
MaxAttempts = 1comment in this PR — it's wrong and the follow-up deletes it.Fixed in PR 23. All five points, plus two traps found on the way out (#382, #383) and one mute path that had nothing to do with your review —
finishClarifiedcould return an empty reply and say nothing at all.Correction: it is PR 22, not 23.
kami referenced this pull request2026-07-31 18:43:58 +02:00
Reviewed all six of your comments against PR #22 (
overnight/clarify-rework), which is the rework written to answer this review — #1 and #22 are a pair, not independent PRs.All six are already addressed there. Details inline. Nothing new needed on this branch; the two should merge together, #1 then #22.
@@ -0,0 +17,4 @@SlotText Slot = "text" // Slots.Text)// MaxAttempts is 1 because Maven is not a nag (DESIGN.md § Non-goals). She asksRe your comment on
MaxAttempts— addressed in PR #22 (overnight/clarify-rework), commit62d320f.DefaultMaxAttemptsis now 3, and configurable viavoice.clarify_max_attemptsindeploy/mavend.json(commitdc70a5a). The give-up is no longer silent either —clarifyGaveUpincmd/mavend/clarify.go:46:That covers both halves of what you asked for: a higher cap, and she says so and asks you to rephrase.
@@ -0,0 +28,4 @@type PendingQuestion struct {Intent Intent // what the router already guessedSlots Slots // what it already filledMissing []Slot // what is still empty, in the order to ask aboutRe "we bash Maven's head against the user until he clears all of them" — you were reading it right, and PR #22 stopped doing that.
She no longer tries to fill every slot.
wantedSlots(cmd/mavend/clarify.go:24) asks about exactly one slot per intent:reminder→ timefact→ keyact→ fnnote,query,chatandsystemare not in the map at all — they get no question, because acting on the raw utterance beats inventing one. One question about the single load-bearing slot, not an interrogation.@@ -0,0 +101,4 @@}// Answer merges the slots parsed from the user's answer into the parked ones.// Only the slots listed in Missing are filled, and an already filled slot isRe "user could've changed his mind" — agreed, and PR #22 reversed this.
The answer now WINS over what was parked. From
PendingQuestion.Answerin PR #22, with your exact case as the justification:A correction in a fresh turn ("вообще-то перенеси на пять") is deliberately a separate path (
followUpMerge), so changing your mind works in both places.@@ -0,0 +146,4 @@}// StillMissing lists the slots that are empty in s, out of the ones asked for.// The caller uses it to decide between acting and dropping the request.Re "we just ignore the user?" — fixed in PR #22.
A partial answer is no longer discarded.
cmd/mavend/clarify.go:149:So what you said is kept, and she re-asks only about what is still missing, up to the cap. When the cap runs out she says
clarifyGaveUpinstead of going quiet.@@ -0,0 +145,4 @@want: Slots{Text: "купить хлеб"},},{name: "empty answer leaves the slot missing",Re the "не знаю" → radio silence trace — your walkthrough was exactly right, and that was the bug. PR #22 cut it.
cmd/mavend/clarify.go:144:"не знаю" now ends in "Прости, я не поняла. Скажи, пожалуйста, по-другому." rather than nothing.
Worth noting: PR #22 is the rework written to answer this review, so #1 and #22 are a pair — #1 lands the data layer, #22 fixes the behaviour you objected to. They should merge together.
@@ -0,0 +158,4 @@t.Run(tc.name, func(t *testing.T) {q := &PendingQuestion{Slots: tc.parked, Missing: tc.missing, Asked: base}got := q.Answer(tc.text, tc.answer)if got.Time != tc.want.Time || got.HasTime != tc.want.HasTime ||Re "why not 100 lines of if X != Y" — fair hit, and PR #22 threw it away.
The eight-field comparison is now a single whole-struct compare:
Which also means a new field on
Slotsis covered without anyone remembering to extend the assertion.Superseded by #47, which landed this whole stack on master as one reviewed integration merge. This PR head is an ancestor of master — its commits are in, nothing here is lost. Closing as merged-by-proxy rather than merged, since the merge came in through #47.
Review threads on this PR were answered or acted on before the merge; the Russian wording fixes went in as #48.
Pull request closed