kami/Maven
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Ask up to three times, and say so when she gives up (review of PR 1) #22

Closed
claude wants to merge 7 commits from overnight/clarify-rework into overnight/phrasing
pull from: overnight/clarify-rework
merge into: kami:overnight/phrasing
Conversation 2 Commits 7 Files Changed 7
+202 -91
claude commented 2026-07-31 10:35:10 +02:00
Contributor
Copy Link
Copy Source

Your review of PR 1, acted on. Three commits.

You were right on all of it, and five of your six comments were the same bug. MaxAttempts = 1, justified in the comment with "Maven is not a nag" — which is a misreading of the non-goal. Not a nag is about her interrupting you unprompted. A clarifying question is part of a conversation you started. That comment is deleted.

1. Three questions, not one. MaxAttempts is now a config value, voice.clarify_max_attempts, default 3. Three because after that the likely problem is she misheard the whole request, not one slot — change the number if it feels wrong in use.

2. She never gives up in silence. When she runs out of questions she says:

Прости, я не поняла. Скажи, пожалуйста, по-другому.

Feminine "поняла", and you're addressed with a genderless imperative. Three separate mute paths got fixed, not just the obvious one: attempts exhausted, a missing slot with no question template to ask with, and a finishClarified that returned an empty reply because both the action and the replier came back empty. That last one was a real mute bug independent of your review.

It also fixed an ordering bug that made your point 4 impossible to fix otherwise: the question was deleted before the answer was evaluated, so there was nothing left to re-ask against. A partial answer now re-parks with whatever it did manage to give, and the TTL clock restarts.

3. A restated slot wins. "в три" then "нет, в пять" now lands at five. Before, the second one was thrown away silently. A later correction in a fresh turn is still a separate path (followUpMerge) and is untouched — there's a comment saying so.

4. That assertion is one reflect.DeepEqual now. Two test cases had to flip to the new semantics, which is the point: they were asserting the behaviour you objected to.

5. New tests: three asks then a spoken give-up with no reminder written and no question left armed, the config knob honoured, and a restated answer landing at 15:00 instead of 11:00.

Two traps found on the way out, filed not fixed:

  • An expired question still dies in silence (#382). Answer after the 90s TTL and the parked request is discarded, your words routed as a fresh utterance, no word that the first one is gone. Same class as what you caught, one layer further out — expiry happens on read, before any of the logic above runs.
  • SlotText is now overwrite-capable and would clobber the original request (#383). The parked Text is your whole original utterance, so a text answer would replace the request instead of completing it. Unreachable today because only time, key and fn are ever asked about — but it breaks quietly the moment someone adds a text question.
Your review of PR 1, acted on. Three commits. You were right on all of it, and five of your six comments were the same bug. `MaxAttempts = 1`, justified in the comment with "Maven is not a nag" — which is a misreading of the non-goal. *Not a nag* is about her interrupting **you** unprompted. A clarifying question is part of a conversation **you started**. That comment is deleted. **1. Three questions, not one.** `MaxAttempts` is now a config value, `voice.clarify_max_attempts`, default 3. Three because after that the likely problem is she misheard the whole request, not one slot — change the number if it feels wrong in use. **2. She never gives up in silence.** When she runs out of questions she says: > Прости, я не поняла. Скажи, пожалуйста, по-другому. Feminine "поняла", and you're addressed with a genderless imperative. **Three separate mute paths** got fixed, not just the obvious one: attempts exhausted, a missing slot with no question template to ask with, and a `finishClarified` that returned an empty reply because both the action and the replier came back empty. That last one was a real mute bug independent of your review. It also fixed an ordering bug that made your point 4 impossible to fix otherwise: **the question was deleted before the answer was evaluated**, so there was nothing left to re-ask against. A partial answer now re-parks with whatever it did manage to give, and the TTL clock restarts. **3. A restated slot wins.** "в три" then "нет, в пять" now lands at five. Before, the second one was thrown away silently. A *later* correction in a fresh turn is still a separate path (`followUpMerge`) and is untouched — there's a comment saying so. **4. That assertion is one `reflect.DeepEqual` now.** Two test cases had to flip to the new semantics, which is the point: they were asserting the behaviour you objected to. **5. New tests:** three asks then a spoken give-up with no reminder written and no question left armed, the config knob honoured, and a restated answer landing at 15:00 instead of 11:00. **Two traps found on the way out, filed not fixed:** - **An expired question still dies in silence** (#382). Answer after the 90s TTL and the parked request is discarded, your words routed as a fresh utterance, no word that the first one is gone. Same class as what you caught, one layer further out — expiry happens on read, before any of the logic above runs. - **`SlotText` is now overwrite-capable and would clobber the original request** (#383). The parked `Text` is your whole original utterance, so a text answer would replace the request instead of completing it. Unreachable today because only time, key and fn are ever asked about — but it breaks quietly the moment someone adds a text question.
claude changed target branch from master to overnight/phrasing 2026-07-31 10:39:24 +02:00
claude added 7 commits 2026-07-31 10:39:24 +02:00
Work out which slot is missing and phrase one short question a54ebac0cb 
A table per intent (reminder needs a time, fact needs a key, act needs a fn)
plus one fixed Russian question per slot. Templates, not model output: a 0.8B
would wander and a question that rewords itself is harder to answer. Note,
query, chat and system get no question — for those a clarify decision keeps
the canned reply rather than inventing a question for noise.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Ask the question, then act on the answer fe0e654ab1 
On a clarify decision with one identifiable gap she now asks instead of saying
"не поняла", and parks the request. The next utterance is parsed as the answer
with the router's own extractor and the completed decision runs through
applyAction like any other — so a clarified act still needs the allowlist and
still hits the destructive confirm gate. An answer that does not fill the gap
drops the request; she never asks twice. Also pulls the session-store block
that HandlePushToTalk and handleText both had into rememberTurn, since the
clarify path needed a third copy.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Test the clarify round-trip end to end at the daemon level 0b3b8d0a9e 
Covers: a reminder with no time is asked about and completes on the answer; the
same for a fact; an answer past the TTL falls through as a fresh utterance; a
second unclear answer drops the request with no second question; a clarified act
off the allowlist neither runs nor gets enabled; a clarified destructive act
still parks a confirm; noise keeps the canned reply. No model, no network.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Let her ask three times, and let a restated answer win 62d320f93a 
MaxAttempts was 1, justified as "not a nag". Wrong reading: "not a nag" is about
interrupting unprompted, and a clarifying question is part of a conversation he
started. Now three, configurable via voice.clarify_max_attempts (default 3).
Three, because after that the likely problem is she misheard the whole request,
not one slot.

Answer used to keep the parked value, so "в три" then "нет, в пять" threw the
five away. Now a value the answer carries wins for the slot she asked about.
Only for the clarify answer — a correction in a fresh turn is followUpMerge.

The eight-field chained assertion in the Answer test is one DeepEqual now, so a
new field in Slots is covered without touching the test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Say out loud when she gives up instead of dropping the request d2be98ee2a 
An unclear answer used to end the request on the spot. Now she re-asks the same
question while attempts remain, and when they run out she says
"Прости, я не поняла. Скажи, пожалуйста, по-другому." — silence would leave him
thinking it was handled. Same reply when the missing slot has no question to
ask, and as a floor in finishClarified so an empty reply can never ship.

Tests: three questions allowed, the fourth gives up out loud, the cap is
configurable, and a restated time is the one that lands.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Merge commit 'd2be98e' into overnight-jul31 06aded6ab0 
# Conflicts:
#	cmd/mavend/clarify.go
#	cmd/mavend/clarify_test.go
#	cmd/mavend/voice.go
#	internal/config/config.go
Show clarify_max_attempts in the deployed config dc70a5a7ab 
The default is 3 either way. Writing it out means you can see the knob
without reading the Go.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
kami reviewed 2026-07-31 18:56:09 +02:00
cmd/mavend/clarify_test.go
@@ -148,0 +152,4 @@
if !handled {
t.Fatalf("answer %d must be consumed as an answer", i)
}
if reply != "На когда напомнить?" {
kami commented 2026-07-31 18:56:09 +02:00
Owner
Copy Link
Copy Source

same note from PR #1 - "На какое время" or plain "Когда" might be more natural.

same note from PR #1 - "На какое время" or plain "Когда" might be more natural.
kami commented 2026-07-31 20:22:07 +02:00
Owner
Copy Link
Copy Source

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:22:07 +02:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
kami
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
kami
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kami/Maven#22
Delete Branch "overnight/clarify-rework"

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?