diff --git a/internal/ipc/frame.go b/internal/ipc/frame.go index 311e7b4..14d35b1 100644 --- a/internal/ipc/frame.go +++ b/internal/ipc/frame.go @@ -36,6 +36,13 @@ const maxFrame = 4 << 20 // (we read the length but not the body), so the caller must close it. var ErrFrameTooLarge = errors.New("ipc: frame too large") +// The framing is hand-rolled and it stays that way (Vikunja #410): ninety +// lines, readable on the wire with socat, and every standard replacement +// brings schema machinery this boundary does not want. The condition is that +// it is defended by tests rather than inherited untested — truncated header, +// truncated body, partial reads, frame boundaries and a non-JSON body all +// live in frame_test.go. +// // writeFrame encodes v as JSON and frames it as a 4-byte big-endian length // prefix + body. length-prefixed JSON (not a tighter binary schema) is the // deferred-but-picked wire format: debuggable with `socat`/`nc`, trivial to diff --git a/internal/ipc/frame_test.go b/internal/ipc/frame_test.go new file mode 100644 index 0000000..f66c07f --- /dev/null +++ b/internal/ipc/frame_test.go @@ -0,0 +1,142 @@ +package ipc + +import ( + "bytes" + "encoding/binary" + "errors" + "io" + "testing" +) + +// The framing is hand-rolled, and it is the protocol all nine daemons depend +// on, so a bug in it is a bug everywhere (Vikunja #410). The review asked +// whether to replace it with something standard. It stays: length-prefixed +// JSON over a unix socket is ninety lines, it is readable with socat, and the +// alternatives (net/rpc, gRPC, a codec library) all buy schema machinery this +// boundary does not want. What was wrong was inheriting it untested. These +// are the paths a real socket produces that the round-trip test never does. + +// A short header is not EOF. EOF means the peer closed cleanly between +// frames, which the server treats as a normal disconnect; a header that stops +// halfway is a truncated frame and must be reported as an error, or a peer +// that dies mid-write looks like one that hung up politely. +func TestReadFrameTruncatedHeader(t *testing.T) { + var v any + err := readFrame(bytes.NewReader([]byte{0, 0, 4}), &v) + if err == nil { + t.Fatal("a three-byte header must fail") + } + if errors.Is(err, io.EOF) { + t.Fatalf("err = %v, want a truncation error, not EOF", err) + } +} + +// A header promising more body than follows. Same reasoning: the frame never +// arrived, so it must not decode into a zero value the caller then trusts. +func TestReadFrameTruncatedBody(t *testing.T) { + var buf bytes.Buffer + var hdr [4]byte + binary.BigEndian.PutUint32(hdr[:], 32) + buf.Write(hdr[:]) + buf.WriteString(`{"m":"pi`) + + var got map[string]string + if err := readFrame(&buf, &got); err == nil { + t.Fatal("a body shorter than its prefix must fail") + } + if len(got) != 0 { + t.Errorf("decoded %v from a truncated frame", got) + } +} + +// Nothing at all is EOF, and only this is. +func TestReadFrameEmptyIsEOF(t *testing.T) { + var v any + if err := readFrame(bytes.NewReader(nil), &v); !errors.Is(err, io.EOF) { + t.Fatalf("err = %v, want io.EOF", err) + } +} + +// byteAtATime returns one byte per Read, which is what a socket is allowed to +// do and what a bytes.Reader never does. readFrame uses io.ReadFull for both +// the header and the body; this is the test that would fail if either turned +// into a bare Read. +type byteAtATime struct { + b []byte + i int +} + +func (r *byteAtATime) Read(p []byte) (int, error) { + if r.i >= len(r.b) { + return 0, io.EOF + } + if len(p) == 0 { + return 0, nil + } + p[0] = r.b[r.i] + r.i++ + return 1, nil +} + +func TestReadFrameReassemblesPartialReads(t *testing.T) { + type payload struct { + Msg string `json:"m"` + N int `json:"n"` + } + want := payload{Msg: "привет", N: 7} + var buf bytes.Buffer + if err := writeFrame(&buf, want); err != nil { + t.Fatalf("writeFrame: %v", err) + } + + var got payload + if err := readFrame(&byteAtATime{b: buf.Bytes()}, &got); err != nil { + t.Fatalf("readFrame: %v", err) + } + if got != want { + t.Fatalf("got %+v, want %+v", got, want) + } +} + +// Two frames written back to back must come back as two frames. A reader that +// consumed more than one frame's body would desynchronize the connection, and +// the symptom would be a reply attributed to the wrong request. +func TestReadFrameStopsAtTheFrameBoundary(t *testing.T) { + var buf bytes.Buffer + for _, m := range []string{"first", "second"} { + if err := writeFrame(&buf, map[string]string{"m": m}); err != nil { + t.Fatalf("writeFrame: %v", err) + } + } + r := &byteAtATime{b: buf.Bytes()} + for _, want := range []string{"first", "second"} { + var got map[string]string + if err := readFrame(r, &got); err != nil { + t.Fatalf("readFrame(%s): %v", want, err) + } + if got["m"] != want { + t.Fatalf("got %q, want %q", got["m"], want) + } + } + var extra map[string]string + if err := readFrame(r, &extra); !errors.Is(err, io.EOF) { + t.Fatalf("after two frames: err = %v, want io.EOF", err) + } +} + +// A body that is not JSON is an error, not a zero value. The peer is either +// broken or not speaking this protocol; either way the caller must not read +// on as though it decoded. +func TestReadFrameRejectsNonJSONBody(t *testing.T) { + var buf bytes.Buffer + var hdr [4]byte + body := []byte("not json at all") + binary.BigEndian.PutUint32(hdr[:], uint32(len(body))) + buf.Write(hdr[:]) + buf.Write(body) + + var got map[string]string + if err := readFrame(&buf, &got); err == nil { + t.Fatal("a non-JSON body must fail") + } +} diff --git a/internal/ipc/storeapi.go b/internal/ipc/storeapi.go index 34bda94..f67ac90 100644 --- a/internal/ipc/storeapi.go +++ b/internal/ipc/storeapi.go @@ -356,7 +356,7 @@ func (a *storeAPI) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine, Action: r.Action, Object: r.Object, IntervalDays: r.IntervalDays, - Status: r.Status, + Status: string(r.Status), CreatedTs: r.CreatedTs.UnixMilli(), } if r.ReminderID != nil { diff --git a/internal/store/proposed_routines.go b/internal/store/proposed_routines.go index 67e43f5..2c61eb7 100644 --- a/internal/store/proposed_routines.go +++ b/internal/store/proposed_routines.go @@ -8,14 +8,37 @@ import ( "time" ) -// The three states a proposal can be in. A proposal starts 'proposed' and -// moves once, either way, and never moves again. +// RoutineStatus — the state a proposal is in. A defined type, not a bare +// string, because the legal set used to live in a comment: nothing caught a +// typo at compile time, nothing enumerated the set for a test, and a bad value +// surfaced as a /routines row that neither accepts nor dismisses (Vikunja #46, +// #410). +type RoutineStatus string + +// The three states a proposal can be in. A proposal starts proposed and moves +// once, either way, and never moves again. const ( - RoutineProposed = "proposed" - RoutineAccepted = "accepted" - RoutineDismissed = "dismissed" + RoutineProposed RoutineStatus = "proposed" + RoutineAccepted RoutineStatus = "accepted" + RoutineDismissed RoutineStatus = "dismissed" ) +// RoutineStatuses — the legal set, and the single source of truth a test can +// range over. Adding a state means adding it here. +var RoutineStatuses = []RoutineStatus{RoutineProposed, RoutineAccepted, RoutineDismissed} + +// Valid reports whether s is one of RoutineStatuses. +func (s RoutineStatus) Valid() bool { + for _, v := range RoutineStatuses { + if s == v { + return true + } + } + return false +} + +func (s RoutineStatus) String() string { return string(s) } + // ProposedRoutine — a detected pattern the system wants to nudge about on a // repeating interval. Status 'proposed' means awaiting human confirmation; // 'accepted' means the human confirmed and the tick loop now owns the schedule; @@ -30,7 +53,7 @@ type ProposedRoutine struct { Action string Object string IntervalDays float64 - Status string // proposed | accepted | dismissed + Status RoutineStatus CreatedTs time.Time ReminderID *int64 AcceptedTs *time.Time @@ -40,6 +63,7 @@ type ProposedRoutine struct { var ( ErrProposedRoutineNotFound = errors.New("store: proposed routine not found") ErrProposedRoutineExists = errors.New("store: proposed routine already exists for this action+object") + ErrRoutineStatus = errors.New("store: unknown routine status") ) // CreateProposedRoutine inserts a new proposed routine. Returns @@ -51,6 +75,16 @@ var ( // keep finding the pattern, and every re-propose is refused here. Maven is not // a nag. // +// The object stays a local string. It is not resolved against Nexus and it +// carries no canonical entity ref (asked on the PR 4 review, decided here, +// Vikunja #410). Nexus owns identity for things the ecosystem acts on, and +// nothing acts on a routine object: it is the word he used, replayed back to +// him in a nudge, and compared only against itself for the UNIQUE key. Two +// spellings of the same watering can are two routines, and that is the right +// answer when the point is to say the sentence he would say. Canonical refs +// arrive here only if a routine ever drives a Hexis call, which is Vikunja +// #272, not this. +// // Vikunja #43: this is called both from the voice fact-write path (for the // immediate spoken confirmation) and from the digestion tick's proactive // scan (cmd/mavend/tick.go's detectPatterns, via patterns.go's @@ -102,8 +136,13 @@ func (s *Store) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine, er } // ListProposedRoutinesByStatus returns routines in one status, newest first. -// An empty status returns every row. -func (s *Store) ListProposedRoutinesByStatus(ctx context.Context, status string) ([]ProposedRoutine, error) { +// An empty status returns every row; an unknown one is refused rather than +// silently answering with nothing, since a typo and a genuinely empty state +// read the same otherwise. +func (s *Store) ListProposedRoutinesByStatus(ctx context.Context, status RoutineStatus) ([]ProposedRoutine, error) { + if status != "" && !status.Valid() { + return nil, fmt.Errorf("%w: %q", ErrRoutineStatus, status) + } q := `SELECT id, action, object, interval_days, status, created_ts, reminder_id, accepted_ts, last_fired_ts FROM proposed_routines` var args []any diff --git a/internal/store/proposed_routines_test.go b/internal/store/proposed_routines_test.go index 81080d9..a72b4f6 100644 --- a/internal/store/proposed_routines_test.go +++ b/internal/store/proposed_routines_test.go @@ -235,7 +235,7 @@ func TestListProposedRoutinesByStatus(t *testing.T) { } cases := []struct { - status string + status RoutineStatus want int }{ {RoutineProposed, 0}, @@ -266,3 +266,59 @@ func TestLookupMissingProposedRoutine(t *testing.T) { t.Fatal("want nil for missing routine") } } + +// Every legal status must survive the database. The status column is written +// by three different UPDATE statements with the value spelled inline, so a +// constant that drifts from its SQL is exactly the failure this catches: the +// row would come back in a state no Go code compares equal to. +func TestRoutineStatusRoundTrip(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + now := time.Now().UTC() + + write := map[RoutineStatus]func(id int64) error{ + RoutineProposed: func(int64) error { return nil }, + RoutineAccepted: func(id int64) error { return s.AcceptProposedRoutine(ctx, id, now) }, + RoutineDismissed: func(id int64) error { return s.DismissProposedRoutine(ctx, id) }, + } + + for _, want := range RoutineStatuses { + if !want.Valid() { + t.Fatalf("%q is in RoutineStatuses but not Valid()", want) + } + object := "object-" + want.String() + id, err := s.CreateProposedRoutine(ctx, "полить", object, 3, now) + if err != nil { + t.Fatalf("CreateProposedRoutine(%s): %v", want, err) + } + if err := write[want](id); err != nil { + t.Fatalf("move to %s: %v", want, err) + } + got, err := s.LookupProposedRoutine(ctx, "полить", object) + if err != nil || got == nil { + t.Fatalf("LookupProposedRoutine(%s): %v", want, err) + } + if got.Status != want { + t.Errorf("status = %q, want %q", got.Status, want) + } + list, err := s.ListProposedRoutinesByStatus(ctx, want) + if err != nil { + t.Fatalf("ListProposedRoutinesByStatus(%s): %v", want, err) + } + if len(list) != 1 { + t.Errorf("status %s: listed %d rows, want 1", want, len(list)) + } + } +} + +// A typo used to read as "nothing is in that state", which is the same answer +// a correct query gives on an empty table. +func TestListByStatusRefusesAnUnknownStatus(t *testing.T) { + s := newTestStore(t) + if _, err := s.ListProposedRoutinesByStatus(context.Background(), "accpeted"); !errors.Is(err, ErrRoutineStatus) { + t.Fatalf("err = %v, want ErrRoutineStatus", err) + } + if RoutineStatus("accpeted").Valid() { + t.Error("a typo must not be Valid()") + } +}