Add the pending-question data layer for clarifying questions #1

Closed
claude wants to merge 0 commits from overnight/clarify-data-layer into master
Contributor

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.Clarify and 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 in voice.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 existing SessionStore, 90s TTL to match confirmTTL.
  • 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.Slots was missing the Value field that router.Slots has, so a clarifying answer for a fact payload had nowhere to land. Added it.

What to look at. Whether MaxAttempts = 1 is the behaviour you want, and whether Answer()'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.Slots and router.Slots are two hand-maintained copies of the same struct, and they had already drifted. Not refactored here.

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.Clarify` and 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 in `voice.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 existing `SessionStore`, 90s TTL to match `confirmTTL`. - `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.Slots` was missing the `Value` field that `router.Slots` has, so a clarifying answer for a fact payload had nowhere to land. Added it. **What to look at.** Whether `MaxAttempts = 1` is the behaviour you want, and whether `Answer()`'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.Slots` and `router.Slots` are two hand-maintained copies of the same struct, and they had already drifted. Not refactored here.
claude added 2 commits 2026-07-31 00:15:00 +02:00
router.Slots already carries the fact payload; the dialogue copy did not, so a clarifying answer had nowhere to put it. InheritSlots carries it like Key.
Reviewer: check the new inherit block does not overwrite a filled value.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
PendingQuestion plus ClarifyStore: same shape, locking and expiry as SessionStore. Answer fills only the missing slots and never overwrites a filled one. No wiring yet — TODOs mark the daemon hooks.
Reviewer: MaxAttempts is 1 on purpose (Maven asks once, she is not a nag).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
kami reviewed 2026-07-31 10:05:48 +02:00
@@ -0,0 +17,4 @@
SlotText Slot = "text" // Slots.Text
)
// MaxAttempts is 1 because Maven is not a nag (DESIGN.md § Non-goals). She asks
Owner

I 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.

I 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.
kami reviewed 2026-07-31 10:07:36 +02:00
@@ -0,0 +28,4 @@
type PendingQuestion struct {
Intent Intent // what the router already guessed
Slots Slots // what it already filled
Missing []Slot // what is still empty, in the order to ask about
Owner

am 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.

am 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.
kami reviewed 2026-07-31 10:10:42 +02:00
@@ -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
Owner

an already filled slot is never overwritten
can't it be controversial? or user could've changed his mind.

> an already filled slot is never overwritten can't it be controversial? or user could've changed his mind.
kami reviewed 2026-07-31 10:11:20 +02:00
@@ -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.
Owner

so, when user answered, but Maven didn't get it fully - we just ignore the user?
that's stupid.

so, when user answered, but Maven didn't get it fully - we just ignore the user? that's stupid.
kami reviewed 2026-07-31 10:13:31 +02:00
@@ -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 ||
Owner

wow. why not 100 lines of if X != Y || Z != A?

wow. why not 100 lines of if X != Y || Z != A?
kami reviewed 2026-07-31 10:15:44 +02:00
@@ -0,0 +145,4 @@
want: Slots{Text: "купить хлеб"},
},
{
name: "empty answer leaves the slot missing",
Owner

this one feels off.
if I get it right:

  • user asks "напомни".
  • Maven is stunned, cause "about what?", "when?", "huh?".
  • Maven asks questions.
  • user says - "не знаю"/"idk" - Maven goes radio silent and user's requests is ignored.

that's. stoo. pid.

this one feels off. if I get it right: - user asks "напомни". - Maven is stunned, cause "about what?", "when?", "huh?". - Maven asks questions. - user says - "не знаю"/"idk" - Maven goes radio silent and user's requests is ignored. that's. stoo. pid.
Author
Contributor

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 1 was 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. Missing isn'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, Missing is 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 Slots grows. 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 = 1 comment in this PR — it's wrong and the follow-up deletes it.

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 `1` was 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. `Missing` isn'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, `Missing` is 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 `Slots` grows. 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 = 1` comment in this PR — it's wrong and the follow-up deletes it.**
Author
Contributor

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 — finishClarified could return an empty reply and say nothing at all.

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 — `finishClarified` could return an empty reply and say nothing at all.
Author
Contributor

Correction: it is PR 22, not 23.

Correction: it is PR **22**, not 23.
kami reviewed 2026-07-31 19:27:53 +02:00
kami left a comment
Owner

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.

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 asks
Owner

Re your comment on MaxAttempts — addressed in PR #22 (overnight/clarify-rework), commit 62d320f.

DefaultMaxAttempts is now 3, and configurable via voice.clarify_max_attempts in deploy/mavend.json (commit dc70a5a). The give-up is no longer silent either — clarifyGaveUp in cmd/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.

**Re your comment on `MaxAttempts` — addressed in PR #22 (`overnight/clarify-rework`), commit 62d320f.** `DefaultMaxAttempts` is now **3**, and configurable via `voice.clarify_max_attempts` in `deploy/mavend.json` (commit dc70a5a). The give-up is no longer silent either — `clarifyGaveUp` in `cmd/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 guessed
Slots Slots // what it already filled
Missing []Slot // what is still empty, in the order to ask about
Owner

Re "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 → time
  • fact → key
  • act → fn

note, query, chat and system are 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.

**Re "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` → time - `fact` → key - `act` → fn `note`, `query`, `chat` and `system` are 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 is
Owner

Re "user could've changed his mind" — agreed, and PR #22 reversed this.

The answer now WINS over what was parked. From PendingQuestion.Answer in PR #22, with your exact case as the justification:

a value the answer carries WINS over what was parked: she asked about this slot, so «нет, в пять» after «в три» must replace the time, not be thrown away.

A correction in a fresh turn ("вообще-то перенеси на пять") is deliberately a separate path (followUpMerge), so changing your mind works in both places.

**Re "user could've changed his mind" — agreed, and PR #22 reversed this.** The answer now WINS over what was parked. From `PendingQuestion.Answer` in PR #22, with your exact case as the justification: > a value the answer carries WINS over what was parked: she asked about this slot, so «нет, в пять» after «в три» must replace the time, not be thrown away. 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.
Owner

Re "we just ignore the user?" — fixed in PR #22.

A partial answer is no longer discarded. cmd/mavend/clarify.go:149:

Re-park with whatever the answer DID give, the clock restarted and one more question spent.

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 clarifyGaveUp instead of going quiet.

**Re "we just ignore the user?" — fixed in PR #22.** A partial answer is no longer discarded. `cmd/mavend/clarify.go:149`: > Re-park with whatever the answer DID give, the clock restarted and one more question spent. 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 `clarifyGaveUp` instead of going quiet.
@@ -0,0 +145,4 @@
want: Slots{Text: "купить хлеб"},
},
{
name: "empty answer leaves the slot missing",
Owner

Re the "не знаю" → radio silence trace — your walkthrough was exactly right, and that was the bug. PR #22 cut it.

cmd/mavend/clarify.go:144:

if question == "" || !q.CanAsk() {
    h.clarifyStore.Delete(voiceDialogueID)
    log.Printf("voice: clarify — gave up on %v after %d question(s), answer was %q", ...)
    return clarifyGaveUp
}

"не знаю" 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.

**Re the "не знаю" → radio silence trace — your walkthrough was exactly right, and that was the bug. PR #22 cut it.** `cmd/mavend/clarify.go:144`: ```go if question == "" || !q.CanAsk() { h.clarifyStore.Delete(voiceDialogueID) log.Printf("voice: clarify — gave up on %v after %d question(s), answer was %q", ...) return clarifyGaveUp } ``` "не знаю" 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 ||
Owner

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:

// Whole-struct compare: a new field in Slots is covered for free.
if got := q.Answer(tc.text, tc.answer); !reflect.DeepEqual(got, tc.want) {

Which also means a new field on Slots is covered without anyone remembering to extend the assertion.

**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: ```go // Whole-struct compare: a new field in Slots is covered for free. if got := q.Answer(tc.text, tc.answer); !reflect.DeepEqual(got, tc.want) { ``` Which also means a new field on `Slots` is covered without anyone remembering to extend the assertion.
Owner

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.

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.
kami closed this pull request 2026-07-31 20:21:28 +02:00

Pull request closed

Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kami/Maven#1