Add accept to /routines, gated at step-up #4

Closed
claude wants to merge 0 commits from overnight/routine-accept into master
Contributor

Vikunja #46. Correction up front: I sent an agent to build this and most of it already existed on master (4c40a18). The store table, the migration, the IPC methods, the /routines page and the detector wiring were all there. So this PR fills the gaps instead of duplicating them, and it is smaller than planned.

Commit 1 — store gaps. Status constants, ListProposedRoutinesByStatus, and tests for the cases that matter: re-proposing a dismissed routine is refused, a dismissed routine stays dismissed and cannot be accepted, and listing by status works.

The dedup key is action+object and already UNIQUE, so once you dismiss something Maven cannot bring it back. That is the whole point — otherwise the detector re-proposes it every run and she becomes a nag.

Commit 2 — the accept button. /routines could only dismiss. Now it can accept: one line per proposal in her own words via pattern.PhraseRoutine, how long ago she noticed, two actions per row.

Accept is gated at step-up, dismiss is not. Accepting hands the trigger loop a new standing reason to speak to you, which moves the boundary — same tier as enabling a tool. Dismissing only removes a reason to speak, so it needs no gesture.

Status is an in-place UPDATE, not append-only. A fact is a claim and the old claim is history, so facts are append-only. A proposal is a question with one answer. tools.status already flips in place and this follows it. The AND status = 'proposed' guard keeps the move one-way.

Two things this PR found and did not fix (both filed):

  • Vikunja #366 — an accepted non-weekly routine fires exactly once, then goes silent forever. Accept turns the routine into a one-shot reminder. The proposal is then accepted, and the dedup key stops the detector re-proposing it. So a 3-day routine fires once and dies quietly. Accepting one is currently worse than not accepting it. The real fix is the tick loop reading accepted proposals directly, marked with a TODO(vikunja#46).
  • Vikunja #367 — the voice path can already accept a routine, while this PR gates the web button at L3. DESIGN.md says voice is structurally incapable of L3. The two surfaces disagree, so one of them is wrong.

Full tree green.

Vikunja #46. **Correction up front:** I sent an agent to build this and most of it already existed on master (`4c40a18`). The store table, the migration, the IPC methods, the `/routines` page and the detector wiring were all there. So this PR fills the gaps instead of duplicating them, and it is smaller than planned. **Commit 1 — store gaps.** Status constants, `ListProposedRoutinesByStatus`, and tests for the cases that matter: re-proposing a dismissed routine is refused, a dismissed routine stays dismissed and cannot be accepted, and listing by status works. The dedup key is `action+object` and already UNIQUE, so once you dismiss something Maven cannot bring it back. That is the whole point — otherwise the detector re-proposes it every run and she becomes a nag. **Commit 2 — the accept button.** `/routines` could only dismiss. Now it can accept: one line per proposal in her own words via `pattern.PhraseRoutine`, how long ago she noticed, two actions per row. **Accept is gated at step-up, dismiss is not.** Accepting hands the trigger loop a new standing reason to speak to you, which moves the boundary — same tier as enabling a tool. Dismissing only removes a reason to speak, so it needs no gesture. **Status is an in-place UPDATE, not append-only.** A fact is a claim and the old claim is history, so facts are append-only. A proposal is a question with one answer. `tools.status` already flips in place and this follows it. The `AND status = 'proposed'` guard keeps the move one-way. **Two things this PR found and did not fix** (both filed): - **Vikunja #366 — an accepted non-weekly routine fires exactly once, then goes silent forever.** Accept turns the routine into a one-shot reminder. The proposal is then `accepted`, and the dedup key stops the detector re-proposing it. So a 3-day routine fires once and dies quietly. Accepting one is currently *worse* than not accepting it. The real fix is the tick loop reading accepted proposals directly, marked with a `TODO(vikunja#46)`. - **Vikunja #367 — the voice path can already accept a routine**, while this PR gates the web button at L3. DESIGN.md says voice is structurally incapable of L3. The two surfaces disagree, so one of them is wrong. Full tree green.
claude added 2 commits 2026-07-31 00:24:39 +02:00
Look at internal/store/proposed_routines.go: status flips in place with an
`AND status = 'proposed'` guard, not append-only like facts/voids_id — a
proposal is a question with one answer, same shape as tools.status. The
UNIQUE(action, object) key is what stops a dismissed routine coming back.
New tests cover re-propose-after-dismiss and listing by status.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Accepting a routine gives the trigger loop a new standing reason to speak to
the human, so it is the same authority tier as enabling a tool and shares the
stepUpOK gate; dismiss only ever makes maven quieter, so it is ungated.
Look at handleRoutines and acceptRoutine in cmd/mavweb/main.go: accept creates
the recurring reminder, then links it via the new ipc AcceptProposedRoutine.
The page now says what maven noticed in her own words (pattern.PhraseRoutine).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
kami reviewed 2026-07-31 18:31:19 +02:00
@@ -166,6 +166,9 @@ func (l *lockedAPI) ListProposedRoutines(ctx context.Context) ([]ipc.ProposedRou
return nil, errLocked
}
func (l *lockedAPI) DismissProposedRoutine(ctx context.Context, id int64) error { return errLocked }
func (l *lockedAPI) AcceptProposedRoutine(ctx context.Context, id, remID int64) error {
Owner

can you explain to me what this signature means exactly?
and why do we have N amount of them here?

can you explain to me what this signature means exactly? and why do we have N amount of them here?
kami reviewed 2026-07-31 18:33:07 +02:00
@@ -682,3 +685,2 @@
// routinesHTML — proposed routine review surface. Lists detected patterns
// awaiting human confirmation, with accept (→ reminder) and dismiss buttons.
// routinesHTML — proposed routine review surface. One row per thing maven
Owner

sorry, I might be confused here, but don't we have a distinct typescript react frontend for maven?
even if we don't - I don't like the fact that we're using html in go.
well, I would understand this if we've used htmx - but we don't, right?

sorry, I might be confused here, but don't we have a distinct typescript react frontend for maven? even if we don't - I don't like the fact that we're using html in go. well, I would understand this if we've used htmx - but we don't, right?
kami reviewed 2026-07-31 18:33:33 +02:00
@@ -788,1 +793,3 @@
func handleRoutines(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) {
// routineRow is one line on the page: what maven noticed, in her words, and
// how long ago she noticed it.
type routineRow struct {
Owner

I don't like rows.

I don't like rows.
kami reviewed 2026-07-31 18:34:29 +02:00
@@ -831,0 +879,4 @@
// proposal accepted and links the reminder to it. Weekly patterns get a cron
// expression; any other interval fires once.
//
// TODO(vikunja#46): this mirrors the voice accept path in cmd/mavend/voice.go.
Owner

are we sure this isn't done already?

are we sure this isn't done already?
kami reviewed 2026-07-31 18:34:53 +02:00
@@ -446,6 +446,9 @@ func (r *recordingAPI) RevertFact(_ context.Context, _ string) (int64, error) {
func (r *recordingAPI) ListProposedRoutines(_ context.Context) ([]ipc.ProposedRoutine, error) {
return nil, nil
}
func (r *recordingAPI) AcceptProposedRoutine(_ context.Context, _, _ int64) error {
Owner

great test, actually. I like those. /s

great test, actually. I like those. /s
kami reviewed 2026-07-31 18:35:38 +02:00
@@ -478,6 +478,9 @@ func (a *chatTestAPI) DeleteTool(ctx context.Context, name string) error {
func (a *chatTestAPI) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine, error) {
return nil, ErrUnknownMethod
}
func (a *chatTestAPI) AcceptProposedRoutine(ctx context.Context, id, remID int64) error {
Owner

again. great tests.

again. great tests.
kami reviewed 2026-07-31 18:36:05 +02:00
@@ -254,2 +254,4 @@
}
func (a *storeAPI) AcceptProposedRoutine(ctx context.Context, id, reminderID int64) error {
return mapErr(a.s.AcceptProposedRoutine(ctx, id, reminderID))
Owner

what does mapErr do?

what does mapErr do?
kami reviewed 2026-07-31 18:36:33 +02:00
@@ -774,6 +778,13 @@ func (s *Server) dispatch(ctx context.Context, req Request) (json.RawMessage, er
}
return marshalResult(nil), api.DismissProposedRoutine(ctx, p.ID)
case MethodAcceptProposedRoutine:
Owner

is this thing 1000+ lines long and is pure switch-case spaghetti nightmare?

is this thing 1000+ lines long and is pure switch-case spaghetti nightmare?
kami reviewed 2026-07-31 18:37:06 +02:00
@@ -11,5 +11,5 @@
// a method is a core-authority change (every method is a new thing a module
// can ask for); do it deliberately.
type Method string
const (
Owner

is there really no other better way to do what we're doing here?

is there really no other better way to do what we're doing here?
kami reviewed 2026-07-31 18:38:07 +02:00
@@ -10,1 +10,4 @@
// The three states a proposal can be in. A proposal starts 'proposed' and
// moves once, either way, and never moves again.
const (
Owner

again - is there no better way to do this enum thing?

again - is there no better way to do this enum thing?
kami reviewed 2026-07-31 18:38:42 +02:00
@@ -31,2 +39,4 @@
// ErrProposedRoutineExists if one already exists for this action+object (any
// status) — the pattern detector should only propose once per pair.
//
// action+object is the "same routine" key. It is UNIQUE in the table, so a
Owner

do we call nexus for identity?

do we call nexus for identity?
Owner

overall I think there's too much logic in most of the files and they should be split.

overall I think there's too much logic in most of the files and they should be split.
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:34 +02:00
kami reviewed 2026-07-31 20:34:26 +02:00
kami left a comment
Owner

Answers to the four threads that were still open here, and they turn out to be one answer.

cmd/mavend/main.go — "what does this signature mean exactly? and why do we have N amount of them here?"

That block is lockedAPI — the pre-unlock state of the daemon. Every method has the same shape (func (l *lockedAPI) X(ctx, ...) (T, error) { return zero, errLocked }) because it implements the whole CoreAPI interface as "refuse everything": before the encryption key is unwrapped, mavend must answer the socket but must not touch the store. There are N of them because the interface has 30 methods, and Go requires every one to be present to satisfy it. None of them do anything.

internal/ipc/server.go:257 — "what does mapErr do?"

It translates store-layer sentinel errors into wire-layer ones: store.ErrNoFactipc.ErrNoFact, and the same for ErrConfidence, ErrVoidsMissing, ErrNudgeNotFound, ErrNudgeOutcome, ErrReminderNotFound, ErrReminderState, ErrToolNotFound. Anything unrecognised passes through unchanged.

The point is that a client (mavweb, mavpoll) can errors.Is(err, ipc.ErrNoFact) without importing internal/store — the wire protocol keeps its own error vocabulary instead of leaking the database layer's. That part is legitimate. What is not great is that the mapping is a hand-maintained switch with no test asserting the two lists stay in sync: add a sentinel to store and forget this function, and the client sees a generic error string instead of a typed one, silently.

internal/auth/auth_test.go:449 and internal/ipc/ipc_test.go:481 — "great tests, actually. I like those. /s"

Sarcasm noted, and fair, but these are not tests — no Test function, nothing asserted. They are test doubles (recordingAPI, chatTestAPI) padding out the same 30-method interface so they compile. recordingAPI genuinely records the handful of calls its tests care about (r.chats++); the surrounding two dozen return nil, nil methods exist only to satisfy the compiler. chatTestAPI is the same with ErrUnknownMethod as the filler.

So there is nothing to delete or improve in place — the noise is not a testing failure, it is the interface showing through.

internal/ipc/server.go:781 — "is this thing 1000+ lines long and is pure switch-case spaghetti nightmare?"

947 lines, and 42 case arms in the one dispatcher. Yes.

The common cause

All four are the same problem seen from four angles: CoreAPI is a 30-method interface with 7 implementations (internal/ipc/client.go, internal/ipc/server.go, cmd/mavend/main.go's real + locked APIs, and four test doubles). Every method added costs one arm in the dispatcher, one stub in lockedAPI, and one stub in every test double — which is exactly the boilerplate you flagged in three different files without knowing they were connected.

Splitting the interface by domain (facts/notes, reminders/nudges, routines/tools, system) fixes the dispatcher, deletes most of lockedAPI, and shrinks the test doubles to the surface each test actually uses. Tracked as Vikunja #408, now reframed around the interface rather than just the switch; mapErr's missing parity test is folded in there too.

The remaining two threads on this PR — "I don't like rows" (routineRows) and "are we sure this isn't done already?" — are being answered in the mavweb frontend task (#409), where there is also news: there is no TypeScript/React frontend in this repo. No package.json, no .tsx, no framework reference anywhere. Eight mavweb pages are already //go:embed-ed html/template files; only the shell partial and three page templates are still HTML-in-Go, and those are being moved out. Nothing needs a framework to fix.

Answers to the four threads that were still open here, and they turn out to be one answer. ## `cmd/mavend/main.go` — "what does this signature mean exactly? and why do we have N amount of them here?" That block is `lockedAPI` — the pre-unlock state of the daemon. Every method has the same shape (`func (l *lockedAPI) X(ctx, ...) (T, error) { return zero, errLocked }`) because it implements the whole `CoreAPI` interface as "refuse everything": before the encryption key is unwrapped, mavend must answer the socket but must not touch the store. There are N of them because the interface has **30 methods**, and Go requires every one to be present to satisfy it. None of them do anything. ## `internal/ipc/server.go:257` — "what does mapErr do?" It translates store-layer sentinel errors into wire-layer ones: `store.ErrNoFact` → `ipc.ErrNoFact`, and the same for `ErrConfidence`, `ErrVoidsMissing`, `ErrNudgeNotFound`, `ErrNudgeOutcome`, `ErrReminderNotFound`, `ErrReminderState`, `ErrToolNotFound`. Anything unrecognised passes through unchanged. The point is that a client (mavweb, mavpoll) can `errors.Is(err, ipc.ErrNoFact)` without importing `internal/store` — the wire protocol keeps its own error vocabulary instead of leaking the database layer's. That part is legitimate. What is not great is that the mapping is a hand-maintained `switch` with no test asserting the two lists stay in sync: add a sentinel to `store` and forget this function, and the client sees a generic error string instead of a typed one, silently. ## `internal/auth/auth_test.go:449` and `internal/ipc/ipc_test.go:481` — "great tests, actually. I like those. /s" Sarcasm noted, and fair, but these are not tests — no `Test` function, nothing asserted. They are **test doubles** (`recordingAPI`, `chatTestAPI`) padding out the same 30-method interface so they compile. `recordingAPI` genuinely records the handful of calls its tests care about (`r.chats++`); the surrounding two dozen `return nil, nil` methods exist only to satisfy the compiler. `chatTestAPI` is the same with `ErrUnknownMethod` as the filler. So there is nothing to delete or improve in place — the noise is not a testing failure, it is the interface showing through. ## `internal/ipc/server.go:781` — "is this thing 1000+ lines long and is pure switch-case spaghetti nightmare?" 947 lines, and 42 `case` arms in the one dispatcher. Yes. ## The common cause All four are the same problem seen from four angles: **`CoreAPI` is a 30-method interface with 7 implementations** (`internal/ipc/client.go`, `internal/ipc/server.go`, `cmd/mavend/main.go`'s real + locked APIs, and four test doubles). Every method added costs one arm in the dispatcher, one stub in `lockedAPI`, and one stub in every test double — which is exactly the boilerplate you flagged in three different files without knowing they were connected. Splitting the interface by domain (facts/notes, reminders/nudges, routines/tools, system) fixes the dispatcher, deletes most of `lockedAPI`, and shrinks the test doubles to the surface each test actually uses. Tracked as **Vikunja #408**, now reframed around the interface rather than just the `switch`; `mapErr`'s missing parity test is folded in there too. The remaining two threads on this PR — "I don't like rows" (`routineRows`) and "are we sure this isn't done already?" — are being answered in the mavweb frontend task (**#409**), where there is also news: there is **no** TypeScript/React frontend in this repo. No `package.json`, no `.tsx`, no framework reference anywhere. Eight mavweb pages are already `//go:embed`-ed `html/template` files; only the shell partial and three page templates are still HTML-in-Go, and those are being moved out. Nothing needs a framework to fix.

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#4