From 708a69375ff2c58ec18e294ce639d6f16e01dbb4 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:16:39 +0400 Subject: [PATCH 1/3] tasks: key derived captures by external id and record who resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A task extracted from mail deduped on the live-norm index only, so once he finished it the row left the live set and the next poll of the same immutable message re-extracted it as a fresh candidate. mavmaild is a read-only reader and marks nothing read, so that repeats forever. Derived rows now carry an ext_id built from the message uid and the extracted span, unique across every status, while voice keeps live-only norm dedupe because saying an errand again is the recurrence signal. A derived source can no longer capture straight to open, and saying a task out loud that Maven had only proposed promotes the candidate instead of answering that it is already in the list. SetTaskStatus was classified AuthRead. Resolving a task is not additive, it erases work off his list, so it is a write, and the row now records the caller that moved it. ListTasks was unbounded. The list-query matcher claimed any utterance with "что мне делать", including "с чем мне помочь", and the urgency stripper matched inside words. Found in review of #60. --- cmd/mavend/actions_task.go | 6 + cmd/mavend/actions_task_test.go | 29 ++++- cmd/mavend/mail.go | 26 +++- internal/auth/policy.go | 13 +- internal/ipc/api.go | 59 ++++++--- internal/ipc/client.go | 4 +- internal/ipc/server.go | 45 ++++--- internal/ipc/unimplemented.go | 2 +- internal/router/task.go | 103 ++++++++++++--- internal/router/task_phrases.json | 2 + internal/router/task_test.go | 19 +++ internal/store/migrations.go | 17 +++ internal/store/tasks.go | 169 +++++++++++++++++++----- internal/store/tasks_test.go | 209 ++++++++++++++++++++++++++---- 14 files changed, 571 insertions(+), 132 deletions(-) diff --git a/cmd/mavend/actions_task.go b/cmd/mavend/actions_task.go index 7b6d60e..99187f0 100644 --- a/cmd/mavend/actions_task.go +++ b/cmd/mavend/actions_task.go @@ -42,6 +42,12 @@ func (h *reactiveHandler) captureTaskFromNote(ctx context.Context, dec router.De log.Printf("voice: capture task: %v", err) return "не получилось записать задачу.", true } + if resp.Promoted { + // It was a candidate Maven derived from something she read, and he has + // now said it himself. Saying "уже в списке" here would be answering a + // confirmation with a shrug. + return "поняла, беру в работу: " + cap.Text, true + } if !resp.Created { return "это уже в списке.", true } diff --git a/cmd/mavend/actions_task_test.go b/cmd/mavend/actions_task_test.go index 252c41e..628b30d 100644 --- a/cmd/mavend/actions_task_test.go +++ b/cmd/mavend/actions_task_test.go @@ -19,6 +19,7 @@ type taskAPI struct { captured []ipc.CaptureTaskReq created bool + promoted bool capErr error tasks []ipc.Task @@ -31,7 +32,7 @@ func (a *taskAPI) CaptureTask(_ context.Context, req ipc.CaptureTaskReq) (ipc.Ca if a.capErr != nil { return ipc.CaptureTaskResp{}, a.capErr } - return ipc.CaptureTaskResp{ID: 1, Created: a.created}, nil + return ipc.CaptureTaskResp{ID: 1, Created: a.created, Promoted: a.promoted}, nil } func (a *taskAPI) ListTasks(_ context.Context, status string) ([]ipc.Task, error) { @@ -225,3 +226,29 @@ func TestQuerySourcesOrderTasksBeforeRecall(t *testing.T) { t.Errorf("tasks source at %d, after notes at %d", tasksAt, notesAt) } } + +// Saying a task out loud that Maven had only proposed is a confirmation. She +// used to answer "это уже в списке" and then read it back, in the same +// conversation, as something he had not confirmed. +func TestCaptureTaskFromNoteAcknowledgesAPromotion(t *testing.T) { + api := &taskAPI{promoted: true} + h := taskHandler(api) + reply, ok := h.captureTaskFromNote(context.Background(), router.Decision{ + Intent: router.IntentNote, Utterance: "добавь в задачи продлить страховку", + }) + if !ok { + t.Fatal("an explicit capture must claim the turn") + } + if strings.Contains(reply, "уже в списке") { + t.Errorf("reply = %q — he just confirmed it, that is not a duplicate", reply) + } + if !strings.Contains(reply, "продлить страховку") { + t.Errorf("reply = %q, want the task named back", reply) + } + // Persona: feminine, informal. + for _, bad := range []string{"рад ", "вы ", "ваш"} { + if strings.Contains(reply, bad) { + t.Errorf("reply %q contains %q", reply, bad) + } + } +} diff --git a/cmd/mavend/mail.go b/cmd/mavend/mail.go index 5167e71..d1d29d4 100644 --- a/cmd/mavend/mail.go +++ b/cmd/mavend/mail.go @@ -82,9 +82,11 @@ func newMailIntake(st *store.Store, phr phraser.Phraser, cfg *config.Config, bus // reader's header filter is what keeps the resident model off newsletters. // // Every candidate is captured with Status "candidate", Source "email:" -// and the subject as Evidence. CaptureTask dedupes on normalised text among -// live rows, so a mailbox re-read after a restart produces Created=0 rather -// than a second copy of every task. +// and the subject as Evidence, under an ExternalID naming the message and the +// span it was extracted from. That key is unique over every row whatever its +// status, so a mailbox re-read after a restart produces Created=0 — and, more +// to the point, a task he already marked done is not re-proposed the next time +// the same unread message is read again. func (m *mailIntake) ingest(ctx context.Context, req ipc.IngestMailReq) (ipc.IngestMailResp, error) { msg := email.Message{ UID: req.UID, @@ -124,15 +126,16 @@ func (m *mailIntake) ingest(ctx context.Context, req ipc.IngestMailReq) (ipc.Ing // something she read is a suggestion until he confirms it on /tasks. Status: store.TaskCandidate, } + t.ExternalID = mailExternalID(source, req.UID, c.Text) if due, ok := email.ParseDue(c.Due); ok { t.Due = &due } - id, created, err := m.st.CaptureTask(ctx, t) + res, err := m.st.CaptureTask(ctx, t) if err != nil { return resp, fmt.Errorf("mail intake: capture: %w", err) } - resp.TaskIDs = append(resp.TaskIDs, id) - if created { + resp.TaskIDs = append(resp.TaskIDs, res.ID) + if res.Created { resp.Created++ // Only a row that was actually created. CaptureTask dedupes on // normalised text among live rows, so a mailbox re-read after a @@ -165,3 +168,14 @@ func truncateRunes(s string, n int) string { } return string(r[:n]) + "…" } + +// mailExternalID names the message and the span a candidate was extracted +// from. The mailbox and UID identify the message; the normalised text +// identifies which of the candidates in it this is, so a message yielding two +// tasks gets two keys and a re-read of it gets neither twice. +// +// UIDs are stable per mailbox, and a mailbox that renumbers (UIDVALIDITY +// changing) re-proposes its tasks once, which is the safe direction. +func mailExternalID(source string, uid uint32, text string) string { + return fmt.Sprintf("%s#%d:%s", source, uid, store.NormalizeTaskText(text)) +} diff --git a/internal/auth/policy.go b/internal/auth/policy.go index f940f80..10a7290 100644 --- a/internal/auth/policy.go +++ b/internal/auth/policy.go @@ -93,6 +93,14 @@ func Requirement(m ipc.Method) Authority { return AuthWrite case ipc.MethodWriteFact: return AuthWrite + case ipc.MethodSetTaskStatus: + // Resolving a task is NOT additive, which is what separates it from + // capture. Capture at AuthRead can only put a line on a list he reads + // himself; SetTaskStatus at AuthRead would let any enrolled module — + // mavpoll, mavsttd — mark every open task done and clear the list out + // from under him. Same reasoning as WriteFact: a module gets to add to + // its own corner, not to erase his. + return AuthWrite case ipc.MethodAssertStepUp: return AuthRead case ipc.MethodLatestFact, @@ -109,10 +117,11 @@ func Requirement(m ipc.Method) Authority { // module write, not an allowlist mutation and not a new standing reason // for Maven to speak — nothing in the tick loop reads tasks. It stays // at AuthRead, the same rung as CreateReminder, which is the closest - // existing analogue. + // existing analogue. SetTaskStatus is NOT here: see the AuthWrite case + // above, because resolving is the one task move that destroys + // something. ipc.MethodCaptureTask, ipc.MethodListTasks, - ipc.MethodSetTaskStatus, // Mail ingestion (Vikunja #246). AuthRead because of what the method can // produce: candidate tasks and nothing else. It cannot write a fact, set a // reminder, or touch the tool allowlist, so a compromised mail reader can diff --git a/internal/ipc/api.go b/internal/ipc/api.go index 30cc4ab..e70e746 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -101,15 +101,17 @@ type WriteFactReq struct { // "tap:voice", "tap:web", "email:". Evidence is the trail a derived // task came from, empty for anything he stated himself. type Task struct { - ID int64 `json:"id"` - CreatedTs time.Time `json:"created_ts"` - Text string `json:"text"` - Source string `json:"source"` - Evidence string `json:"evidence,omitempty"` - Status string `json:"status"` - Due *time.Time `json:"due,omitempty"` - Weight int `json:"weight,omitempty"` - Resolved *time.Time `json:"resolved,omitempty"` + ID int64 `json:"id"` + CreatedTs time.Time `json:"created_ts"` + Text string `json:"text"` + Source string `json:"source"` + Evidence string `json:"evidence,omitempty"` + ExternalID string `json:"external_id,omitempty"` + Status string `json:"status"` + Due *time.Time `json:"due,omitempty"` + Weight int `json:"weight,omitempty"` + Resolved *time.Time `json:"resolved,omitempty"` + ResolvedBy string `json:"resolved_by,omitempty"` } // CaptureTaskReq — THE INTAKE SEAM. Everything that captures a task goes @@ -118,18 +120,26 @@ type Task struct { // // An extractor that reads mail sets Source "email:", Status // "candidate", and Evidence to whatever makes the task reviewable (the subject -// line). It must NOT set Status "open" — work Maven inferred from something she -// read is a suggestion until the owner confirms it on the /tasks page. Capture -// is idempotent on normalised text among live tasks, so re-reading the same -// mailbox is free. +// line). It cannot set Status "open" — work Maven inferred from something she +// read is a suggestion until the owner confirms it on the /tasks page, and the +// store refuses an open capture from a derived source rather than trusting the +// caller to have read this paragraph. +// +// ExternalID is what makes re-reading free for such a source, and it is +// REQUIRED of one. Text dedupe only covers live rows, because a voice capture +// of the same errand next week is a new task. A mailbox has no such signal: it +// hands back the same immutable message forever, so a task he already finished +// would come back as a fresh candidate on the next poll. ExternalID is unique +// over every row whatever its status: message id plus the extracted span. type CaptureTaskReq struct { - Text string `json:"text"` - Source string `json:"source"` - Evidence string `json:"evidence,omitempty"` - Status string `json:"status,omitempty"` // "" ⇒ open - Due *time.Time `json:"due,omitempty"` - Weight int `json:"weight,omitempty"` - Ts time.Time `json:"ts"` + Text string `json:"text"` + Source string `json:"source"` + Evidence string `json:"evidence,omitempty"` + ExternalID string `json:"external_id,omitempty"` + Status string `json:"status,omitempty"` // "" ⇒ open + Due *time.Time `json:"due,omitempty"` + Weight int `json:"weight,omitempty"` + Ts time.Time `json:"ts"` } // CaptureTaskResp — Created is false when the same live task already existed, @@ -138,6 +148,10 @@ type CaptureTaskReq struct { type CaptureTaskResp struct { ID int64 `json:"id"` Created bool `json:"created"` + // Promoted — this capture turned an existing candidate into open work. He + // stated out loud something Maven had only proposed, which is a + // confirmation, and the caller says so rather than "уже в списке". + Promoted bool `json:"promoted,omitempty"` } // IngestMailReq — one message a mail reader has fetched, handed to core for @@ -400,6 +414,9 @@ type setTaskStatusReq struct { ID int64 `json:"id"` Status string `json:"status"` Ts time.Time `json:"ts"` + // By — the caller making the move, in the source vocabulary. Recorded on + // the row so a resolved task says what resolved it. + By string `json:"by,omitempty"` } // idReq — methods keyed by a single id. @@ -626,7 +643,7 @@ type CoreAPI interface { ListTasks(ctx context.Context, status string) ([]Task, error) // SetTaskStatus moves a task forward once: candidate→open|dropped, // open→done|dropped. Any other move is refused. - SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time) error + SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string) error // TickTrace returns the most recent tick's rule trace. The daemon caches // this after every tick; the store adapter returns an error (trace is not diff --git a/internal/ipc/client.go b/internal/ipc/client.go index fae2f4d..8bf775d 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -450,8 +450,8 @@ func (c *Client) ListTasks(ctx context.Context, status string) ([]Task, error) { return r.Tasks, nil } -func (c *Client) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time) error { - return c.call(ctx, MethodSetTaskStatus, setTaskStatusReq{ID: id, Status: status, Ts: ts}, nil) +func (c *Client) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string) error { + return c.call(ctx, MethodSetTaskStatus, setTaskStatusReq{ID: id, Status: status, Ts: ts, By: by}, nil) } // IngestMail hands one fetched message to core for extraction. ErrUnknownMethod diff --git a/internal/ipc/server.go b/internal/ipc/server.go index cee5355..9afa839 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -242,19 +242,20 @@ func (a *storeAPI) DeleteTool(ctx context.Context, name string) error { } func (a *storeAPI) CaptureTask(ctx context.Context, req CaptureTaskReq) (CaptureTaskResp, error) { - id, created, err := a.s.CaptureTask(ctx, store.Task{ - CreatedTs: req.Ts, - Text: req.Text, - Source: req.Source, - Evidence: req.Evidence, - Status: req.Status, - Due: req.Due, - Weight: req.Weight, + res, err := a.s.CaptureTask(ctx, store.Task{ + CreatedTs: req.Ts, + Text: req.Text, + Source: req.Source, + Evidence: req.Evidence, + ExternalID: req.ExternalID, + Status: req.Status, + Due: req.Due, + Weight: req.Weight, }) if err != nil { return CaptureTaskResp{}, mapErr(err) } - return CaptureTaskResp{ID: id, Created: created}, nil + return CaptureTaskResp{ID: res.ID, Created: res.Created, Promoted: res.Promoted}, nil } func (a *storeAPI) ListTasks(ctx context.Context, status string) ([]Task, error) { @@ -265,22 +266,24 @@ func (a *storeAPI) ListTasks(ctx context.Context, status string) ([]Task, error) out := make([]Task, len(ts)) for i, t := range ts { out[i] = Task{ - ID: t.ID, - CreatedTs: t.CreatedTs, - Text: t.Text, - Source: t.Source, - Evidence: t.Evidence, - Status: t.Status, - Due: t.Due, - Weight: t.Weight, - Resolved: t.ResolvedTs, + ID: t.ID, + CreatedTs: t.CreatedTs, + Text: t.Text, + Source: t.Source, + Evidence: t.Evidence, + ExternalID: t.ExternalID, + Status: t.Status, + Due: t.Due, + Weight: t.Weight, + Resolved: t.ResolvedTs, + ResolvedBy: t.ResolvedBy, } } return out, nil } -func (a *storeAPI) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time) error { - return mapErr(a.s.SetTaskStatus(ctx, id, status, ts)) +func (a *storeAPI) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string) error { + return mapErr(a.s.SetTaskStatus(ctx, id, status, ts, by)) } func (a *storeAPI) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine, error) { @@ -849,7 +852,7 @@ var methodTable = map[Method]handlerFunc{ return listTasksResp{Tasks: out}, nil }), MethodSetTaskStatus: withParamsVoid(func(ctx context.Context, api CoreAPI, p setTaskStatusReq) error { - return api.SetTaskStatus(ctx, p.ID, p.Status, p.Ts) + return api.SetTaskStatus(ctx, p.ID, p.Status, p.Ts, p.By) }), MethodListProposedRoutines: withoutParams(func(ctx context.Context, api CoreAPI) (listProposedRoutinesResp, error) { out, err := api.ListProposedRoutines(ctx) diff --git a/internal/ipc/unimplemented.go b/internal/ipc/unimplemented.go index 20e1b9f..ffc8c36 100644 --- a/internal/ipc/unimplemented.go +++ b/internal/ipc/unimplemented.go @@ -95,7 +95,7 @@ func (UnimplementedCoreAPI) CaptureTask(ctx context.Context, req CaptureTaskReq) func (UnimplementedCoreAPI) ListTasks(ctx context.Context, status string) ([]Task, error) { return nil, ErrNotImplemented } -func (UnimplementedCoreAPI) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time) error { +func (UnimplementedCoreAPI) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string) error { return ErrNotImplemented } func (UnimplementedCoreAPI) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine, error) { diff --git a/internal/router/task.go b/internal/router/task.go index 4c7ace1..c85059b 100644 --- a/internal/router/task.go +++ b/internal/router/task.go @@ -47,7 +47,10 @@ func ParseTaskCapture(text string) (TaskCapture, bool) { rest := strings.TrimSpace(trimmed[len(best):]) rest = strings.TrimLeft(rest, ":—- ") rest = strings.TrimSpace(rest) - rest = strings.TrimRight(rest, ".!") + // The question mark goes too. Whisper punctuates dictated Russian, and + // "добавь в задачи позвонить в банк?" must not store the mark or carry it + // into the dedupe key. + rest = strings.TrimRight(rest, ".!?") rest, weight := stripUrgency(rest) if rest == "" { return TaskCapture{}, false @@ -55,30 +58,86 @@ func ParseTaskCapture(text string) (TaskCapture, bool) { return TaskCapture{Text: rest, Weight: weight}, true } +// urgencyIntensifiers — words that may sit between the edge and the marker. +// "очень срочно оплатить интернет" is the marker at the edge with one word in +// front of it, and it means exactly what "срочно оплатить интернет" means. +var urgencyIntensifiers = []string{"очень", "прям", "прямо", "really", "very", "super"} + +// urgencyEdgeTrim — punctuation to ignore around an edge token and to clean off +// the remainder afterwards. +const urgencyEdgeTrim = " .,;:!?—-" + // stripUrgency pulls a leading or trailing urgency word out of the task text // and returns the weight it implies. Only at the edges: "срочно оплатить // интернет" and "оплатить интернет срочно" are the same instruction, while // "позвонить в срочную помощь" is a task whose text happens to contain the // stem, and cutting a word out of the middle of it would mangle the task. // +// Matched as a TOKEN, not as a fixed prefix or suffix string. The old shape +// required exactly one space before a trailing marker, so "оплатить интернет, +// срочно" — which is what whisper produces from dictated Russian — kept weight +// 0 and stored the comma and the word as part of the task, polluting the dedupe +// key with the very flag he was trying to set. +// // The word is removed from the text, because the list should read "оплатить // интернет (важно)" and not "важно оплатить интернет (важно)". func stripUrgency(text string) (string, int) { + fields := strings.Fields(text) + if len(fields) == 0 { + return text, 0 + } for _, m := range urgencyMarkers { - lower := strings.ToLower(text) - switch { - case strings.HasPrefix(lower, m.Word+" "): - return strings.TrimSpace(text[len(m.Word):]), m.Weight - case strings.HasSuffix(lower, " "+m.Word): - return strings.TrimSpace(text[:len(text)-len(m.Word)]), m.Weight - case lower == m.Word: - // Nothing but the marker — no task in it. - return "", 0 + // Strongest marker first (task_phrases.go sorts them), leading edge + // before trailing, so a text carrying both keeps the stronger one. + if lo, hi, ok := urgencySpan(fields, m.Word); ok { + rest := strings.Join(append(append([]string{}, fields[:lo]...), fields[hi+1:]...), " ") + rest = strings.Trim(rest, urgencyEdgeTrim) + if rest == "" { + // Nothing but the marker — no task in it. + return "", 0 + } + return rest, m.Weight } } return text, 0 } +// urgencySpan finds the marker at either edge, allowing intensifiers between +// the edge and the marker, and returns the inclusive token range to cut. +func urgencySpan(fields []string, word string) (lo, hi int, ok bool) { + for i := 0; i < len(fields); i++ { + if isUrgencyToken(fields[i], word) { + return 0, i, true + } + if !isIntensifier(fields[i]) { + break + } + } + for i := len(fields) - 1; i >= 0; i-- { + if isUrgencyToken(fields[i], word) { + return i, len(fields) - 1, true + } + if !isIntensifier(fields[i]) { + break + } + } + return 0, 0, false +} + +func isUrgencyToken(tok, word string) bool { + return strings.Trim(strings.ToLower(tok), urgencyEdgeTrim) == word +} + +func isIntensifier(tok string) bool { + t := strings.Trim(strings.ToLower(tok), urgencyEdgeTrim) + for _, w := range urgencyIntensifiers { + if t == w { + return true + } + } + return false +} + // IsTaskListQuery reports whether an utterance asks for the outstanding task // list — "какие у меня задачи?", "что мне нужно сделать?", "список дел". // @@ -94,13 +153,23 @@ func IsTaskListQuery(text string) bool { if hasTok(toks, "как") && (hasTok(toks, "дела") || hasTok(toks, "делишки")) { return false } - // "что мне нужно сделать" / "что мне делать" — no task noun at all. - if (hasTok(toks, "что") || hasTok(toks, "чем")) && - (hasTok(toks, "сделать") || hasTok(toks, "заняться")) { - return true - } - if hasTok(toks, "what") && hasTok(toks, "do") { - return true + // "что мне нужно сделать" / "чем мне заняться" — no task noun at all, so + // the pronoun is what carries the meaning. Without it these rules claimed + // every question with a verb in them: "что нужно сделать чтобы перезапустить + // сервер?" and "what does docker do?" both answered "задач нет." from ahead + // of recall and the model, which is the failure the source ordering exists + // to avoid, pointed the other way. + // + // A "с"/"со" object excludes them too: "что мне сделать с этим файлом" has + // the pronoun and is still a question about a file. + if !hasTok(toks, "с") && !hasTok(toks, "со") { + if hasTok(toks, "мне") && (hasTok(toks, "что") || hasTok(toks, "чем")) && + (hasTok(toks, "сделать") || hasTok(toks, "делать") || hasTok(toks, "заняться")) { + return true + } + if hasTok(toks, "what") && hasTok(toks, "do") && hasTok(toks, "i") && !hasTok(toks, "you") { + return true + } } hasNoun := false for _, t := range toks { diff --git a/internal/router/task_phrases.json b/internal/router/task_phrases.json index 79e5b5c..f70eff3 100644 --- a/internal/router/task_phrases.json +++ b/internal/router/task_phrases.json @@ -21,6 +21,7 @@ ], "capture_prefixes": [ "добавь в задачи", + "добавь в тудушки", "добавь в список задач", "добавь в список дел", "добавь в список", @@ -28,6 +29,7 @@ "запиши в задачи", "запиши задачу", "новая задача", + "поставь задачу", "в задачи", "add a task", "add task", diff --git a/internal/router/task_test.go b/internal/router/task_test.go index 0f5f2b9..75503f5 100644 --- a/internal/router/task_test.go +++ b/internal/router/task_test.go @@ -20,6 +20,17 @@ func TestParseTaskCapture(t *testing.T) { {"новая задача важно позвонить маме", "позвонить маме", 2, true}, // The stem inside the task text is part of the task, not a marker. {"добавь в задачи позвонить в срочную помощь", "позвонить в срочную помощь", 0, true}, + // Whisper punctuates dictated Russian. The marker used to be missed as + // soon as anything sat next to it, and then it stayed in the task text + // and in the dedupe key — the exact task he was trying to flag. + {"добавь в задачи оплатить интернет, срочно", "оплатить интернет", 3, true}, + {"добавь в задачи очень срочно оплатить интернет", "оплатить интернет", 3, true}, + {"добавь в задачи оплатить интернет — важно", "оплатить интернет", 2, true}, + // A dictated question mark is not part of the task. + {"добавь в задачи позвонить в банк?", "позвонить в банк", 0, true}, + // The phrasings he uses that the prefix list did not have. + {"поставь задачу вынести мусор", "вынести мусор", 0, true}, + {"добавь в тудушки купить лампочки", "купить лампочки", 0, true}, // A marker with nothing after it files nothing. {"добавь в задачи", "", 0, false}, {"новая задача", "", 0, false}, @@ -47,6 +58,7 @@ func TestIsTaskListQuery(t *testing.T) { "задачи", "мои задачи", "what should I do", + "что мне делать?", } for _, s := range yes { if !IsTaskListQuery(s) { @@ -55,6 +67,13 @@ func TestIsTaskListQuery(t *testing.T) { } no := []string{ "как дела?", + // No task noun and no pronoun: these fired ahead of recall and the + // model, and answered a question about a file or a server with + // "задач нет." + "что нужно сделать чтобы перезапустить сервер?", + "что мне сделать с этим файлом?", + "what does docker do?", + "what do you do?", "какая погода?", "напомни мне позвонить маме в шесть", "я сделал зарядку", diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 163ad83..842cb97 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -160,6 +160,23 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 ); CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_live_norm ON tasks (norm) WHERE status IN ('candidate','open'); CREATE INDEX IF NOT EXISTS idx_tasks_status ON tasks (status, created_ts DESC);`, + + // #15 — external identity and resolution attribution for tasks. + // + // ext_id is the identity of the thing a derived task was extracted FROM + // (message id plus the extracted span), and its unique index covers EVERY + // row, not just the live ones. The live-only norm index is right for + // voice, where him saying the errand again is the recurrence signal. It is + // wrong for a mailbox: mavmaild is a read-only reader, nothing marks a + // message read, so a task he already finished would be re-extracted from + // the same immutable text on the next poll and land back on his list as a + // fresh candidate, forever. + // + // resolved_by records which caller moved the task. resolved_ts said when + // and never by what, so a wrong resolution left no trace at all. + `ALTER TABLE tasks ADD COLUMN ext_id TEXT; + ALTER TABLE tasks ADD COLUMN resolved_by TEXT NOT NULL DEFAULT ''; + CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_ext_id ON tasks (ext_id) WHERE ext_id IS NOT NULL;`, } // migrate applies every migration with a number greater than the DB's current diff --git a/internal/store/tasks.go b/internal/store/tasks.go index b211ff6..9552554 100644 --- a/internal/store/tasks.go +++ b/internal/store/tasks.go @@ -48,16 +48,34 @@ const ( // // Due is optional. Weight is an explicit importance hint (0 = none), which the // prioritiser reads; capture never invents one. +// ExternalID is the identity of the thing this task was derived FROM — a +// message id plus the extracted span, for a source that re-reads the same +// immutable text forever. Empty for anything he stated himself. +// +// ResolvedBy names the caller that moved the task to its terminal state, in the +// source vocabulary. Empty while the task is live. type Task struct { ID int64 CreatedTs time.Time Text string Source string Evidence string + ExternalID string Status string Due *time.Time Weight int ResolvedTs *time.Time + ResolvedBy string +} + +// CaptureResult — what CaptureTask did. Created is a new row. Promoted is an +// existing candidate this capture turned into open work: he stated out loud a +// task Maven had only proposed, which is a confirmation, and the caller says so +// instead of "уже в списке". +type CaptureResult struct { + ID int64 + Created bool + Promoted bool } var ( @@ -69,19 +87,49 @@ var ( // liveTaskStatuses — the two statuses that count as outstanding work. var liveTaskStatuses = []string{TaskCandidate, TaskOpen} -// CaptureTask inserts a task, or returns the existing live task when the same -// work is already outstanding. created reports which happened, so a caller can -// tell the owner "уже в списке" instead of pretending it wrote something. +// derivedSourcePrefixes — provenance that means "Maven read this somewhere", +// as opposed to "he said it". A task from one of these is a candidate and +// nothing else; see CaptureTask. +var derivedSourcePrefixes = []string{"email:"} + +// IsDerivedSource reports whether a task source means Maven inferred the task +// from something she read rather than being told it. +func IsDerivedSource(source string) bool { + for _, p := range derivedSourcePrefixes { + if strings.HasPrefix(source, p) { + return true + } + } + return false +} + +// CaptureTask inserts a task, or returns the existing one when the same work is +// already there. The result says which happened, so a caller can tell the owner +// "уже в списке" instead of pretending it wrote something. // -// Dedupe is on the normalised text among LIVE rows only (see the partial unique -// index in migration #14): a weekly errand can be captured again once the last -// one is done, but a mail that gets re-read produces no second row. This is the -// property the email intake depends on — it may call CaptureTask for every -// message it extracts from, as often as it likes, without growing the list. -func (s *Store) CaptureTask(ctx context.Context, t Task) (id int64, created bool, err error) { +// Two dedupe keys, because voice and mail have different intake semantics: +// +// - ExternalID, unique over EVERY row whatever its status. A source that +// re-reads the same immutable text forever must never resurrect work he has +// already finished. This is the property the email intake depends on: it +// may call CaptureTask for every message it extracts from, as often as it +// likes, without growing the list. +// - The normalised text among LIVE rows only (the partial unique index in +// migration #14), for anything with no external identity. A weekly errand +// captured again once the last one is done must produce a new row, because +// him saying it again IS the recurrence signal. +// +// A capture with Status open over an existing candidate PROMOTES it. Stating +// the work out loud is a confirmation, and leaving it a candidate would have +// Maven read it back as something he never confirmed. +// +// A derived source may only ever capture a candidate. The doc on the intake +// seam said "must NOT set Status open"; this is where that stops being an +// honour system, so a compromised reader cannot file work he never reviewed. +func (s *Store) CaptureTask(ctx context.Context, t Task) (CaptureResult, error) { text := strings.TrimSpace(t.Text) if text == "" { - return 0, false, ErrTaskEmpty + return CaptureResult{}, ErrTaskEmpty } status := t.Status if status == "" { @@ -90,7 +138,10 @@ func (s *Store) CaptureTask(ctx context.Context, t Task) (id int64, created bool if status != TaskCandidate && status != TaskOpen { // Capturing straight into a resolved state is meaningless — a task is // captured live and moved later. - return 0, false, fmt.Errorf("%w: capture status %q", ErrTaskStatus, status) + return CaptureResult{}, fmt.Errorf("%w: capture status %q", ErrTaskStatus, status) + } + if status == TaskOpen && IsDerivedSource(t.Source) { + return CaptureResult{}, fmt.Errorf("%w: derived source %q may only capture a candidate", ErrTaskStatus, t.Source) } norm := NormalizeTaskText(text) created2 := t.CreatedTs @@ -101,31 +152,69 @@ func (s *Store) CaptureTask(ctx context.Context, t Task) (id int64, created bool if t.Due != nil { due = sql.NullInt64{Int64: t.Due.UnixMilli(), Valid: true} } + var ext sql.NullString + if e := strings.TrimSpace(t.ExternalID); e != "" { + ext = sql.NullString{String: e, Valid: true} + } + // Untargeted DO NOTHING: either unique index may be the one that fires, and + // the lookup below sorts out which. res, err := s.db.ExecContext(ctx, - `INSERT INTO tasks (created_ts, text, norm, source, evidence, status, due_ts, weight) - VALUES (?,?,?,?,?,?,?,?) - ON CONFLICT (norm) WHERE status IN ('candidate','open') DO NOTHING`, - created2.UnixMilli(), text, norm, t.Source, t.Evidence, status, due, t.Weight) + `INSERT INTO tasks (created_ts, text, norm, source, evidence, ext_id, status, due_ts, weight) + VALUES (?,?,?,?,?,?,?,?,?) + ON CONFLICT DO NOTHING`, + created2.UnixMilli(), text, norm, t.Source, t.Evidence, ext, status, due, t.Weight) if err != nil { - return 0, false, fmt.Errorf("capture task: %w", err) + return CaptureResult{}, fmt.Errorf("capture task: %w", err) } if n, err := res.RowsAffected(); err != nil { - return 0, false, fmt.Errorf("capture task: rows affected: %w", err) + return CaptureResult{}, fmt.Errorf("capture task: rows affected: %w", err) } else if n > 0 { id, err := res.LastInsertId() if err != nil { - return 0, false, fmt.Errorf("capture task: last insert id: %w", err) + return CaptureResult{}, fmt.Errorf("capture task: last insert id: %w", err) } - return id, true, nil + return CaptureResult{ID: id, Created: true}, nil } - // Already live — hand back the row that won. - existing, err := s.lookupLiveTaskByNorm(ctx, norm) - if err != nil { - return 0, false, err + // Something already holds one of the keys. External identity first: that + // row may be resolved, in which case the answer is "already handled", not a + // new task. + var existing Task + if ext.Valid { + existing, err = s.lookupTaskByExternalID(ctx, ext.String) + if err != nil && !errors.Is(err, ErrTaskNotFound) { + return CaptureResult{}, err + } } - return existing.ID, false, nil + if existing.ID == 0 { + existing, err = s.lookupLiveTaskByNorm(ctx, norm) + if err != nil { + return CaptureResult{}, err + } + } + if status == TaskOpen && existing.Status == TaskCandidate { + if err := s.SetTaskStatus(ctx, existing.ID, TaskOpen, created2, t.Source); err != nil { + return CaptureResult{}, fmt.Errorf("capture task: promote candidate: %w", err) + } + return CaptureResult{ID: existing.ID, Promoted: true}, nil + } + return CaptureResult{ID: existing.ID}, nil +} + +// lookupTaskByExternalID finds a task by the identity of what it was derived +// from, in ANY status. Resolved rows count: the whole point of the key is that +// re-reading the mail that produced a finished task produces nothing. +func (s *Store) lookupTaskByExternalID(ctx context.Context, ext string) (Task, error) { + row := s.db.QueryRowContext(ctx, taskSelect+` WHERE ext_id = ?`, ext) + t, err := scanTask(row) + if errors.Is(err, sql.ErrNoRows) { + return Task{}, ErrTaskNotFound + } + if err != nil { + return Task{}, fmt.Errorf("lookup task by external id: %w", err) + } + return t, nil } // lookupLiveTaskByNorm finds the outstanding task with this normalised text. @@ -142,7 +231,7 @@ func (s *Store) lookupLiveTaskByNorm(ctx context.Context, norm string) (Task, er return t, nil } -const taskSelect = `SELECT id, created_ts, text, source, evidence, status, due_ts, weight, resolved_ts FROM tasks` +const taskSelect = `SELECT id, created_ts, text, source, evidence, COALESCE(ext_id,''), status, due_ts, weight, resolved_ts, resolved_by FROM tasks` // LookupTask returns one task by id. func (s *Store) LookupTask(ctx context.Context, id int64) (Task, error) { @@ -157,9 +246,15 @@ func (s *Store) LookupTask(ctx context.Context, id int64) (Task, error) { return t, nil } -// ListTasks returns tasks in one status, newest first. An empty status returns -// every row; "live" returns candidate + open, which is what every read path -// that means "outstanding work" wants. +// MaxTaskRows — the hard bound on one ListTasks read. The live set is a list a +// person keeps by hand and never approaches this; the resolved set grows for as +// long as the box runs, and an unbounded read of it is a page that gets slower +// every month. Newest first, so the bound drops the oldest finished work. +const MaxTaskRows = 500 + +// ListTasks returns tasks in one status, newest first, at most MaxTaskRows of +// them. An empty status returns every row; "live" returns candidate + open, +// which is what every read path that means "outstanding work" wants. func (s *Store) ListTasks(ctx context.Context, status string) ([]Task, error) { q := taskSelect var args []any @@ -172,7 +267,7 @@ func (s *Store) ListTasks(ctx context.Context, status string) ([]Task, error) { q += ` WHERE status = ?` args = append(args, status) } - q += ` ORDER BY created_ts DESC, id DESC` + q += fmt.Sprintf(` ORDER BY created_ts DESC, id DESC LIMIT %d`, MaxTaskRows) rows, err := s.db.QueryContext(ctx, q, args...) if err != nil { @@ -201,8 +296,14 @@ func (s *Store) ListTasks(ctx context.Context, status string) ([]Task, error) { // ErrTaskNotFound-wrapped detail, the same one-way shape proposed_routines and // tools use: an answered question is not answered twice. // -// Resolving frees the dedupe key, which is the point: the work can recur. -func (s *Store) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time) error { +// Resolving frees the NORM dedupe key, which is the point: the work can recur +// when he says it again. It does not free an external identity — see +// CaptureTask for why a re-read mailbox must not resurrect finished work. +// +// by names the caller making the move, in the source vocabulary ("tap:web", +// "tap:voice"). It is recorded on the row, so a task that turns up resolved +// says what resolved it. +func (s *Store) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string) error { var from []string switch status { case TaskOpen: @@ -222,9 +323,9 @@ func (s *Store) SetTaskStatus(ctx context.Context, id int64, status string, ts t resolved = sql.NullInt64{Int64: ts.UnixMilli(), Valid: true} } - q := `UPDATE tasks SET status = ?, resolved_ts = ? WHERE id = ? AND status IN (?` + + q := `UPDATE tasks SET status = ?, resolved_ts = ?, resolved_by = ? WHERE id = ? AND status IN (?` + strings.Repeat(",?", len(from)-1) + `)` - args := []any{status, resolved, id} + args := []any{status, resolved, by, id} for _, f := range from { args = append(args, f) } @@ -271,7 +372,7 @@ func scanTask(sc scanner) (Task, error) { var t Task var created int64 var due, resolved sql.NullInt64 - if err := sc.Scan(&t.ID, &created, &t.Text, &t.Source, &t.Evidence, &t.Status, &due, &t.Weight, &resolved); err != nil { + if err := sc.Scan(&t.ID, &created, &t.Text, &t.Source, &t.Evidence, &t.ExternalID, &t.Status, &due, &t.Weight, &resolved, &t.ResolvedBy); err != nil { return Task{}, err } t.CreatedTs = time.UnixMilli(created).UTC() diff --git a/internal/store/tasks_test.go b/internal/store/tasks_test.go index 9cf92c8..dadc48e 100644 --- a/internal/store/tasks_test.go +++ b/internal/store/tasks_test.go @@ -3,6 +3,7 @@ package store import ( "context" "errors" + "fmt" "testing" "time" ) @@ -12,24 +13,24 @@ func TestCaptureTaskDedupesLiveWork(t *testing.T) { st := newTestStore(t) now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) - id, created, err := st.CaptureTask(ctx, Task{Text: "купить молоко", Source: "tap:voice", CreatedTs: now}) + first, err := st.CaptureTask(ctx, Task{Text: "купить молоко", Source: "tap:voice", CreatedTs: now}) if err != nil { t.Fatal(err) } - if !created { + if !first.Created { t.Fatal("first capture must create a row") } // Same work, different casing and punctuation — one task, not two. - again, created, err := st.CaptureTask(ctx, Task{Text: "Купить молоко!", Source: "email:kami", CreatedTs: now}) + again, err := st.CaptureTask(ctx, Task{Text: "Купить молоко!", Source: "tap:web", CreatedTs: now}) if err != nil { t.Fatal(err) } - if created { + if again.Created { t.Error("second capture of the same live work must not create a row") } - if again != id { - t.Errorf("dedupe returned id %d, want the existing %d", again, id) + if again.ID != first.ID { + t.Errorf("dedupe returned id %d, want the existing %d", again.ID, first.ID) } live, err := st.ListTasks(ctx, "live") @@ -46,20 +47,22 @@ func TestCaptureTaskAfterDoneIsANewTask(t *testing.T) { st := newTestStore(t) now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) - id, _, err := st.CaptureTask(ctx, Task{Text: "полить цветы", Source: "tap:voice", CreatedTs: now}) + first, err := st.CaptureTask(ctx, Task{Text: "полить цветы", Source: "tap:voice", CreatedTs: now}) if err != nil { t.Fatal(err) } - if err := st.SetTaskStatus(ctx, id, TaskDone, now.Add(time.Hour)); err != nil { + id := first.ID + if err := st.SetTaskStatus(ctx, id, TaskDone, now.Add(time.Hour), "tap:web"); err != nil { t.Fatal(err) } // The dedupe key is free again: a recurring errand must be capturable. - id2, created, err := st.CaptureTask(ctx, Task{Text: "полить цветы", Source: "tap:voice", CreatedTs: now.AddDate(0, 0, 7)}) + second, err := st.CaptureTask(ctx, Task{Text: "полить цветы", Source: "tap:voice", CreatedTs: now.AddDate(0, 0, 7)}) if err != nil { t.Fatal(err) } - if !created || id2 == id { - t.Fatalf("re-capture after done: created=%v id=%d (previous %d)", created, id2, id) + id2 := second.ID + if !second.Created || id2 == id { + t.Fatalf("re-capture after done: created=%v id=%d (previous %d)", second.Created, id2, id) } live, err := st.ListTasks(ctx, "live") if err != nil { @@ -76,7 +79,7 @@ func TestCaptureTaskCandidateKeepsEvidence(t *testing.T) { now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) due := now.Add(48 * time.Hour) - id, _, err := st.CaptureTask(ctx, Task{ + res, err := st.CaptureTask(ctx, Task{ Text: "продлить страховку", Source: "email:kami", Evidence: "Re: страховой полис истекает", @@ -88,7 +91,7 @@ func TestCaptureTaskCandidateKeepsEvidence(t *testing.T) { if err != nil { t.Fatal(err) } - got, err := st.LookupTask(ctx, id) + got, err := st.LookupTask(ctx, res.ID) if err != nil { t.Fatal(err) } @@ -114,22 +117,23 @@ func TestSetTaskStatusMovesOnceForwardOnly(t *testing.T) { st := newTestStore(t) now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) - cand, _, err := st.CaptureTask(ctx, Task{Text: "записаться к врачу", Source: "email:kami", Status: TaskCandidate, CreatedTs: now}) + res, err := st.CaptureTask(ctx, Task{Text: "записаться к врачу", Source: "email:kami", Status: TaskCandidate, CreatedTs: now}) if err != nil { t.Fatal(err) } + cand := res.ID // candidate → done is not a legal move: he has to confirm it first. - if err := st.SetTaskStatus(ctx, cand, TaskDone, now); !errors.Is(err, ErrTaskNotFound) { + if err := st.SetTaskStatus(ctx, cand, TaskDone, now, "tap:web"); !errors.Is(err, ErrTaskNotFound) { t.Errorf("candidate→done err = %v, want ErrTaskNotFound", err) } - if err := st.SetTaskStatus(ctx, cand, TaskOpen, now); err != nil { + if err := st.SetTaskStatus(ctx, cand, TaskOpen, now, "tap:web"); err != nil { t.Fatal(err) } - if err := st.SetTaskStatus(ctx, cand, TaskDone, now.Add(time.Hour)); err != nil { + if err := st.SetTaskStatus(ctx, cand, TaskDone, now.Add(time.Hour), "tap:web"); err != nil { t.Fatal(err) } // Already resolved — a second resolve must not move it again. - if err := st.SetTaskStatus(ctx, cand, TaskDropped, now.Add(2*time.Hour)); !errors.Is(err, ErrTaskNotFound) { + if err := st.SetTaskStatus(ctx, cand, TaskDropped, now.Add(2*time.Hour), "tap:web"); !errors.Is(err, ErrTaskNotFound) { t.Errorf("second resolve err = %v, want ErrTaskNotFound", err) } got, err := st.LookupTask(ctx, cand) @@ -147,14 +151,15 @@ func TestSetTaskStatusMovesOnceForwardOnly(t *testing.T) { func TestSetTaskStatusRejectsUnknownStatus(t *testing.T) { ctx := context.Background() st := newTestStore(t) - id, _, err := st.CaptureTask(ctx, Task{Text: "что-то", Source: "tap:web"}) + res, err := st.CaptureTask(ctx, Task{Text: "что-то", Source: "tap:web"}) if err != nil { t.Fatal(err) } - if err := st.SetTaskStatus(ctx, id, "candidate", time.Now()); !errors.Is(err, ErrTaskStatus) { + id := res.ID + if err := st.SetTaskStatus(ctx, id, "candidate", time.Now(), "tap:web"); !errors.Is(err, ErrTaskStatus) { t.Errorf("→candidate err = %v, want ErrTaskStatus", err) } - if err := st.SetTaskStatus(ctx, id, "urgent", time.Now()); !errors.Is(err, ErrTaskStatus) { + if err := st.SetTaskStatus(ctx, id, "urgent", time.Now(), "tap:web"); !errors.Is(err, ErrTaskStatus) { t.Errorf("→urgent err = %v, want ErrTaskStatus", err) } } @@ -162,7 +167,7 @@ func TestSetTaskStatusRejectsUnknownStatus(t *testing.T) { func TestCaptureTaskRejectsEmptyText(t *testing.T) { ctx := context.Background() st := newTestStore(t) - if _, _, err := st.CaptureTask(ctx, Task{Text: " ", Source: "tap:voice"}); !errors.Is(err, ErrTaskEmpty) { + if _, err := st.CaptureTask(ctx, Task{Text: " ", Source: "tap:voice"}); !errors.Is(err, ErrTaskEmpty) { t.Errorf("err = %v, want ErrTaskEmpty", err) } } @@ -172,10 +177,10 @@ func TestListTasksFiltersByStatus(t *testing.T) { st := newTestStore(t) now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) - open1, _, _ := st.CaptureTask(ctx, Task{Text: "первая", Source: "tap:voice", CreatedTs: now}) - _, _, _ = st.CaptureTask(ctx, Task{Text: "вторая", Source: "email:kami", Status: TaskCandidate, CreatedTs: now.Add(time.Minute)}) - done, _, _ := st.CaptureTask(ctx, Task{Text: "третья", Source: "tap:voice", CreatedTs: now.Add(2 * time.Minute)}) - if err := st.SetTaskStatus(ctx, done, TaskDone, now.Add(time.Hour)); err != nil { + open1, _ := st.CaptureTask(ctx, Task{Text: "первая", Source: "tap:voice", CreatedTs: now}) + _, _ = st.CaptureTask(ctx, Task{Text: "вторая", Source: "email:kami", Status: TaskCandidate, CreatedTs: now.Add(time.Minute)}) + done, _ := st.CaptureTask(ctx, Task{Text: "третья", Source: "tap:voice", CreatedTs: now.Add(2 * time.Minute)}) + if err := st.SetTaskStatus(ctx, done.ID, TaskDone, now.Add(time.Hour), "tap:web"); err != nil { t.Fatal(err) } @@ -190,7 +195,7 @@ func TestListTasksFiltersByStatus(t *testing.T) { if err != nil { t.Fatal(err) } - if len(opens) != 1 || opens[0].ID != open1 { + if len(opens) != 1 || opens[0].ID != open1.ID { t.Fatalf("open = %+v", opens) } all, err := st.ListTasks(ctx, "") @@ -219,3 +224,153 @@ func TestNormalizeTaskText(t *testing.T) { } } } + +// A mail the reader keeps seeing must not resurrect work he already finished. +// The norm key alone frees on resolve, which is right for voice and wrong for a +// mailbox: mavmaild never marks anything read, so the same message is extracted +// again on every poll, forever. +func TestCaptureTaskExternalIDSurvivesResolution(t *testing.T) { + ctx := context.Background() + st := newTestStore(t) + now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) + mail := Task{ + Text: "продлить страховку", Source: "email:kami", Status: TaskCandidate, + ExternalID: "email:kami#412:продлить страховку", CreatedTs: now, + } + + first, err := st.CaptureTask(ctx, mail) + if err != nil { + t.Fatal(err) + } + if !first.Created { + t.Fatal("first capture must create a row") + } + // He confirms it and does it. + if err := st.SetTaskStatus(ctx, first.ID, TaskOpen, now.Add(time.Hour), "tap:web"); err != nil { + t.Fatal(err) + } + if err := st.SetTaskStatus(ctx, first.ID, TaskDone, now.Add(2*time.Hour), "tap:web"); err != nil { + t.Fatal(err) + } + + // The next poll reads the same message again. + mail.CreatedTs = now.AddDate(0, 0, 1) + again, err := st.CaptureTask(ctx, mail) + if err != nil { + t.Fatal(err) + } + if again.Created { + t.Error("re-reading the same mail created a second task after the first was done") + } + if again.ID != first.ID { + t.Errorf("id = %d, want the resolved row %d", again.ID, first.ID) + } + live, err := st.ListTasks(ctx, "live") + if err != nil { + t.Fatal(err) + } + if len(live) != 0 { + t.Fatalf("live = %+v, want nothing: he already did this", live) + } +} + +// Stating out loud a task Maven only proposed is a confirmation. Leaving it a +// candidate had her read it straight back as something he had not confirmed. +func TestCaptureTaskPromotesCandidate(t *testing.T) { + ctx := context.Background() + st := newTestStore(t) + now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) + + cand, err := st.CaptureTask(ctx, Task{ + Text: "продлить страховку", Source: "email:kami", Status: TaskCandidate, + ExternalID: "email:kami#7:продлить страховку", CreatedTs: now, + }) + if err != nil { + t.Fatal(err) + } + spoken, err := st.CaptureTask(ctx, Task{ + Text: "продлить страховку", Source: "tap:voice", Status: TaskOpen, + CreatedTs: now.Add(time.Minute), + }) + if err != nil { + t.Fatal(err) + } + if spoken.Created { + t.Error("capture over a live candidate must not create a second row") + } + if !spoken.Promoted { + t.Error("capture with status open over a candidate must promote it") + } + got, err := st.LookupTask(ctx, cand.ID) + if err != nil { + t.Fatal(err) + } + if got.Status != TaskOpen { + t.Errorf("status = %q, want open", got.Status) + } + if got.ResolvedTs != nil { + t.Errorf("resolved_ts = %v, want nil: the task is still live", got.ResolvedTs) + } +} + +// A derived source may only ever file a candidate. The intake seam documented +// this and nothing enforced it, so a caller could skip review entirely. +func TestCaptureTaskRefusesOpenFromDerivedSource(t *testing.T) { + ctx := context.Background() + st := newTestStore(t) + _, err := st.CaptureTask(ctx, Task{Text: "оплатить счёт", Source: "email:kami", Status: TaskOpen}) + if !errors.Is(err, ErrTaskStatus) { + t.Errorf("err = %v, want ErrTaskStatus", err) + } +} + +// resolved_ts said when a task was resolved and never by what. +func TestSetTaskStatusRecordsWho(t *testing.T) { + ctx := context.Background() + st := newTestStore(t) + now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) + res, err := st.CaptureTask(ctx, Task{Text: "выкинуть мусор", Source: "tap:voice", CreatedTs: now}) + if err != nil { + t.Fatal(err) + } + if err := st.SetTaskStatus(ctx, res.ID, TaskDone, now.Add(time.Hour), "tap:voice"); err != nil { + t.Fatal(err) + } + got, err := st.LookupTask(ctx, res.ID) + if err != nil { + t.Fatal(err) + } + if got.ResolvedBy != "tap:voice" { + t.Errorf("resolved_by = %q, want tap:voice", got.ResolvedBy) + } +} + +// The resolved history only grows; an unbounded read of it is a page that gets +// slower every month. +func TestListTasksIsBounded(t *testing.T) { + ctx := context.Background() + st := newTestStore(t) + now := time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC) + for i := 0; i < MaxTaskRows+5; i++ { + res, err := st.CaptureTask(ctx, Task{ + Text: fmt.Sprintf("задача %d", i), Source: "tap:voice", + CreatedTs: now.Add(time.Duration(i) * time.Minute), + }) + if err != nil { + t.Fatal(err) + } + if err := st.SetTaskStatus(ctx, res.ID, TaskDone, now.Add(time.Hour), "tap:web"); err != nil { + t.Fatal(err) + } + } + all, err := st.ListTasks(ctx, "") + if err != nil { + t.Fatal(err) + } + if len(all) != MaxTaskRows { + t.Fatalf("all = %d rows, want the %d-row bound", len(all), MaxTaskRows) + } + if all[0].Text != fmt.Sprintf("задача %d", MaxTaskRows+4) { + t.Errorf("first = %q, want the newest", all[0].Text) + } +} From 88d25d31ac3bc571435abfdde1789927340d8b54 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:16:56 +0400 Subject: [PATCH 2/3] tasks: compare due dates in the caller's day, not in UTC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dayDelta truncated both instants to a UTC day. A task due at 02:00 Moscow time tonight read as due tomorrow, and one due at 23:00 last night read as due today, so the two classes that decide the whole order were assigned from the wrong calendar. Both sides are now truncated in now's location. Dated work also lost to age alone because the later-due score sat below the age cap, and the tail said "и ещё 3" with no noun and no Russian plural agreement. The page hardcoded time.Now, so none of this was testable from a fixed clock. It now takes an injectable clock, parses the due date in that clock's location, parses ids and weights with strconv instead of a hand-rolled scan, caps the resolved table and says so, shows who resolved each row, and reports a promotion as the confirmation it is. Found in review of #61. --- cmd/mavweb/main.go | 70 ++++++++++++++------- cmd/mavweb/tasks.html | 9 ++- cmd/mavweb/tasks_test.go | 122 +++++++++++++++++++++++++++++++++++- internal/tasks/rank.go | 56 +++++++++++++---- internal/tasks/rank_test.go | 68 ++++++++++++++++++++ 5 files changed, 287 insertions(+), 38 deletions(-) diff --git a/cmd/mavweb/main.go b/cmd/mavweb/main.go index 17e49af..f0c8b42 100644 --- a/cmd/mavweb/main.go +++ b/cmd/mavweb/main.go @@ -18,6 +18,7 @@ import ( "net/url" "os" "os/signal" + "strconv" "strings" "time" @@ -865,17 +866,29 @@ func handleReminders(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { } } +// now — the wall clock, indirected so the task page can be rendered at a fixed +// instant in a test. internal/tasks is pure and the daemon path already ranks +// through a clock it is handed; the page had no reason to be the one surface +// that could only be tested at whatever time it happened to run. +var now = time.Now + +// resolvedShown — how many finished tasks the page renders. The list is +// history, it only grows, and the rows below the first screen are read by +// nobody. +const resolvedShown = 50 + // taskRow is one line on /tasks, with every timestamp already formatted so the // template holds no date logic. type taskRow struct { - ID int64 - Text string - Source string - Evidence string - Status string - Due string - Created string - Resolved string + ID int64 + Text string + Source string + Evidence string + Status string + Due string + Created string + Resolved string + ResolvedBy string // Why — the ranker's reason for this row's position (Vikunja #129), in // Russian, empty when nothing distinguished the task. Blank is the honest // rendering: he never said this one mattered more. @@ -923,6 +936,7 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { // rows keep store order (newest first) — ranking finished work is pointless. var live []tasks.Item var resolved []taskRow + resolvedTotal := 0 for _, t := range all { switch t.Status { case "candidate", "open": @@ -931,10 +945,18 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { Created: t.CreatedTs, Due: t.Due, Weight: t.Weight, }) default: + resolvedTotal++ + // Finished work is history, and the history only grows. The page + // showed every row that ever existed, which is a page that gets + // slower every month for a section nobody reads past the top of. + if len(resolved) >= resolvedShown { + continue + } resolved = append(resolved, taskRow{ ID: t.ID, Text: t.Text, Source: t.Source, Evidence: t.Evidence, Status: t.Status, Created: fmtTaskTime(&t.CreatedTs), Due: fmtTaskDate(t.Due), Resolved: fmtTaskTime(t.Resolved), + ResolvedBy: t.ResolvedBy, }) } } @@ -943,7 +965,7 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { byID[t.ID] = t } var cands, open []taskRow - for _, r := range tasks.Rank(live, time.Now()) { + for _, r := range tasks.Rank(live, now()) { t := byID[r.ID] row := taskRow{ ID: t.ID, Text: t.Text, Source: t.Source, Evidence: t.Evidence, @@ -962,11 +984,12 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { } w.Header().Set("Content-Type", "text/html; charset=utf-8") if err := tasksTmpl.Execute(w, struct { - Msg, Err string - Candidates []taskRow - Open []taskRow - Resolved []taskRow - }{msg, errMsg, cands, open, resolved}); err != nil { + Msg, Err string + Candidates []taskRow + Open []taskRow + Resolved []taskRow + ResolvedMore bool + }{msg, errMsg, cands, open, resolved, resolvedTotal > len(resolved)}); err != nil { log.Printf("tasks render: %v", err) } } @@ -981,12 +1004,14 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri if text == "" { return "", errors.New("empty task text") } - req := ipc.CaptureTaskReq{Text: text, Source: "tap:web", Status: "open", Ts: time.Now()} + req := ipc.CaptureTaskReq{Text: text, Source: "tap:web", Status: "open", Ts: now()} // Importance is his, stated on the form. Out-of-range values are // clamped rather than rejected — a bad select is not worth a 400. if v := r.FormValue("weight"); v != "" { - var wgt int - if n, _ := fmt.Sscanf(v, "%d", &wgt); n != 1 || wgt < 0 { + // strconv, not Sscanf: Sscanf("3junk", "%d") succeeds with 3, and a + // form value is not a place to accept trailing garbage. + wgt, err := strconv.Atoi(v) + if err != nil || wgt < 0 { return "", fmt.Errorf("bad weight %q", v) } if wgt > tasks.MaxWeight { @@ -995,7 +1020,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri req.Weight = wgt } if d := r.FormValue("due"); d != "" { - due, err := time.ParseInLocation("2006-01-02", d, time.Local) + due, err := time.ParseInLocation("2006-01-02", d, now().Location()) if err != nil { return "", fmt.Errorf("bad due date %q", d) } @@ -1005,14 +1030,17 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri if err != nil { return "", err } + if resp.Promoted { + return "confirmed a candidate maven had found", nil + } if !resp.Created { return "already on the list", nil } return "added task", nil } - var id int64 - if n, _ := fmt.Sscanf(r.FormValue("id"), "%d", &id); n != 1 { + id, err := strconv.ParseInt(r.FormValue("id"), 10, 64) + if err != nil { return "", errors.New("invalid id") } var status, msg string @@ -1026,7 +1054,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri default: return "", fmt.Errorf("unknown action %q", action) } - if err := core.SetTaskStatus(ctx, id, status, time.Now()); err != nil { + if err := core.SetTaskStatus(ctx, id, status, now(), "tap:web"); err != nil { return "", err } return msg, nil diff --git a/cmd/mavweb/tasks.html b/cmd/mavweb/tasks.html index 8194506..70f6797 100644 --- a/cmd/mavweb/tasks.html +++ b/cmd/mavweb/tasks.html @@ -9,6 +9,9 @@ +