Fix Feature 12 review findings F2–F8 on Regular grounding and onboarding

Grounding (F2–F5): - Each ground_dinner job carries the dinner's new grounding_revision (migration 0010). Renaming a pending Regular raises the revision, and a job only grounds and writes a dinner still at its revision, so a queued original job no longer grounds the new name or decides the outcome. - Background grounding keeps a method, minutes or difficulty a member saved while the Regular was pending, and fills only those left empty. - Grounding by hand clears the pasted recipe_url, and link deduplication uses the source_url of the attached recipe, else the pending link. - Archived dinners are not grounded: the job checks before any search, fetch or model call, and its final update requires an unarchived row. Onboarding (F6, F7): - Step 3 shows "Add Regulars" back to step 2 when the server refuses to finish with fewer than three Regulars. - Common picks are hidden by a copy of harness Normalize/Similar, checked against the same shared cases as the Go code. Docs (F8): ARCHITECTURE.md covers the three-Regular minimum, step-3 recovery, the grounding revision and the dedupe rules. F1 was fixed in 1304487; its test now also asserts the old link is never grounded and recipe_url stays cleared. Maestro task 5037ff86-181d-448e-927b-c2fd2258a25d. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-06 19:24:47 -04:00
parent 13044879f6
commit 837c9924d1
22 changed files with 875 additions and 232 deletions
@@ -2,6 +2,8 @@ package dinners_test

 import (
 	"context"
+	"database/sql"
+	"encoding/json"
 	"errors"
 	"io"
 	"log/slog"
@@ -385,6 +387,168 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) {
 	if got := e.search.Queries(); !slices.Equal(got, []string{"Beef stew"}) {
 		t.Errorf("searched %q, want only the new name", got)
 	}
+	if row.RecipeUrl.Valid {
+		t.Errorf("recipe_url = %q, want the old link dropped", row.RecipeUrl.String)
+	}
+	if _, err := db.New(e.st.Reader).GetRecipeBySourceURL(t.Context(), curryA); !errors.Is(err, sql.ErrNoRows) {
+		t.Errorf("old link's recipe lookup err = %v, want it never grounded (sql.ErrNoRows)", err)
+	}
+}
+
+// queuedJob reads the payload of queued KindGroundDinner job id.
+func (e *env) queuedJob(t *testing.T, id string) dinners.GroundJob {
+	t.Helper()
+	var payload string
+	if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT payload FROM jobs WHERE id = ?`, id).Scan(&payload); err != nil {
+		t.Fatal(err)
+	}
+	var p dinners.GroundJob
+	if err := json.Unmarshal([]byte(payload), &p); err != nil {
+		t.Fatal(err)
+	}
+	return p
+}
+
+// TestRenameBeforeJobStartsSkipsStaleJob: a Regular renamed before its
+// first job starts is grounded only by the job its rename enqueued. The
+// first job, though it would read the new name, does no work and writes
+// nothing, so its failure cannot leave the dinner "Needs a recipe" while
+// the rename's job finds a recipe.
+func TestRenameBeforeJobStartsSkipsStaleJob(t *testing.T) {
+	t.Parallel()
+	e := newUnstartedEnv(t)
+	e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}}
+
+	c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member)
+	if err != nil {
+		t.Fatal(err)
+	}
+	first := e.queuedJob(t, *c.GroundingJobID)
+	if first.DinnerID != c.ID || first.Revision != e.dinner(t, c.ID).GroundingRevision {
+		t.Fatalf("first job = %+v, want dinner %s at its revision", first, c.ID)
+	}
+	if _, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil {
+		t.Fatal(err)
+	}
+	renamed := e.dinner(t, c.ID)
+	if renamed.GroundingRevision != first.Revision+1 {
+		t.Fatalf("revision after rename = %d, want %d", renamed.GroundingRevision, first.Revision+1)
+	}
+
+	// The first job would fail if it searched.
+	e.search.Err = recipes.ErrSearchRateLimited
+	status, err := e.svc.RunGroundJob(t.Context(), first)
+	if err != nil || status != dinners.GroundingPending {
+		t.Fatalf("first job = %q, %v; want pending, no error", status, err)
+	}
+	if q := e.search.Queries(); len(q) != 0 {
+		t.Errorf("first job searched %q, want no work for a stale revision", q)
+	}
+	if row := e.dinner(t, c.ID); row != renamed {
+		t.Fatalf("first job wrote the dinner: %+v, was %+v", row, renamed)
+	}
+
+	e.search.Err = nil
+	e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150))
+	status, err = e.svc.RunGroundJob(t.Context(), dinners.GroundJob{DinnerID: c.ID, Revision: renamed.GroundingRevision})
+	if err != nil || status != dinners.GroundingGrounded {
+		t.Fatalf("rename's job = %q, %v; want grounded", status, err)
+	}
+	row := e.dinner(t, c.ID)
+	rec, err := db.New(e.st.Reader).GetRecipe(t.Context(), row.RecipeID.String)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if row.GroundingStatus != dinners.GroundingGrounded || rec.SourceUrl != stew {
+		t.Errorf("dinner = %+v recipe %s, want grounded in %s", row, rec.SourceUrl, stew)
+	}
+
+	// A stale job finishing last changes nothing either.
+	if status, err := e.svc.RunGroundJob(t.Context(), first); err != nil || status != dinners.GroundingGrounded {
+		t.Errorf("first job run last = %q, %v; want grounded reported, no error", status, err)
+	}
+	if after := e.dinner(t, c.ID); after != row {
+		t.Errorf("stale job changed the grounded dinner: %+v, was %+v", after, row)
+	}
+}
+
+// TestEditWhilePendingIsKept: a method or minutes a member saves while a
+// Regular is grounding is kept when grounding succeeds; what the member
+// did not set comes from the recipe.
+func TestEditWhilePendingIsKept(t *testing.T) {
+	t.Parallel()
+	e := newEnv(t)
+	e.search.Gate = make(chan struct{})
+	e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40))
+
+	c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member)
+	if err != nil {
+		t.Fatal(err)
+	}
+	e.waitQuery(t, "Chicken curry")
+	if _, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Method: ptr("grill"), MinutesTotal: ptr(15)}, member); err != nil {
+		t.Fatal(err)
+	}
+	close(e.search.Gate)
+
+	row := e.waitGrounding(t, c.ID)
+	if row.GroundingStatus != dinners.GroundingGrounded || !row.RecipeID.Valid || !row.PhotoID.Valid {
+		t.Fatalf("dinner = %+v, want grounded", row)
+	}
+	if row.Method.String != "grill" || row.MinutesTotal.Int64 != 15 || row.Difficulty.String != "easy" {
+		t.Errorf("dinner = %s, %d minutes, %s; want the member's grill and 15 minutes, the recipe's easy",
+			row.Method.String, row.MinutesTotal.Int64, row.Difficulty.String)
+	}
+}
+
+// TestArchiveWhilePendingStopsGrounding: a pending Regular archived before
+// its job starts is not searched for, and one archived while its job runs
+// is not written; neither ends up grounded.
+func TestArchiveWhilePendingStopsGrounding(t *testing.T) {
+	t.Parallel()
+	t.Run("before the job starts", func(t *testing.T) {
+		t.Parallel()
+		e := newUnstartedEnv(t)
+		e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40))
+		c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err := e.svc.Archive(t.Context(), c.ID, member); err != nil {
+			t.Fatal(err)
+		}
+		e.start(t)
+		if job := e.waitJob(t, *c.GroundingJobID); job.Status != jobs.StatusSucceeded {
+			t.Fatalf("job = %+v, want succeeded with nothing to do", job)
+		}
+		if q := e.search.Queries(); len(q) != 0 {
+			t.Errorf("searched %q for an archived dinner", q)
+		}
+		if row := e.dinner(t, c.ID); row.Status != dinners.StatusArchived || row.GroundingStatus == dinners.GroundingGrounded || row.RecipeID.Valid {
+			t.Errorf("dinner = %+v, want archived and not grounded", row)
+		}
+	})
+	t.Run("while the job runs", func(t *testing.T) {
+		t.Parallel()
+		e := newEnv(t)
+		e.search.Gate = make(chan struct{})
+		e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40))
+		c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member)
+		if err != nil {
+			t.Fatal(err)
+		}
+		e.waitQuery(t, "Chicken curry")
+		if _, err := e.svc.Archive(t.Context(), c.ID, member); err != nil {
+			t.Fatal(err)
+		}
+		close(e.search.Gate)
+		if job := e.waitJob(t, *c.GroundingJobID); job.Status != jobs.StatusSucceeded {
+			t.Fatalf("job = %+v", job)
+		}
+		if row := e.dinner(t, c.ID); row.Status != dinners.StatusArchived || row.GroundingStatus == dinners.GroundingGrounded || row.RecipeID.Valid {
+			t.Errorf("dinner = %+v, want archived and not grounded", row)
+		}
+	})
 }

 // TestRenameWhilePendingEnqueueFailure: a pending Regular renamed when the
@@ -530,6 +694,59 @@ func TestFixViaCandidate(t *testing.T) {
 	}
 }

+// TestFixReplacesPendingLink: fixing a Regular added from a link that held
+// no recipe drops that link, so it no longer blocks adding the link again,
+// while the link of the recipe chosen does.
+func TestFixReplacesPendingLink(t *testing.T) {
+	t.Parallel()
+	const lost = "https://gone.example/recipes/lamb-tagine"
+	tests := []struct {
+		name string
+		fix  dinners.Fix
+		// linkTaken is the recipe link the fixed dinner now has, if any.
+		linkTaken string
+	}{
+		{"candidate", dinners.Fix{CandidateID: ptr(curryB)}, curryB},
+		{"manual", dinners.Fix{Manual: &dinners.ManualRecipe{Ingredients: []string{"1 lamb"}, Minutes: ptr(90), Method: ptr("oven")}}, ""},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			e := newEnv(t)
+			c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if job := e.waitJob(t, *c.GroundingJobID); job.Status != jobs.StatusFailed {
+				t.Fatalf("job = %+v, want failed", job)
+			}
+			// Renamed, so only the link could repeat it.
+			if _, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Weeknight dinner")}, member); err != nil {
+				t.Fatal(err)
+			}
+			e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25))
+			d, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if d.RecipeURL != nil {
+				t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", *d.RecipeURL)
+			}
+
+			if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member); err != nil {
+				t.Errorf("adding the dropped link again: %v, want added", err)
+			}
+			if tt.linkTaken != "" {
+				_, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(tt.linkTaken)}, member)
+				var dup *dinners.DuplicateError
+				if !errors.As(err, &dup) || dup.Existing.ID != c.ID {
+					t.Errorf("adding the fixed dinner's recipe link: err = %v, want a duplicate of %s", err, c.ID)
+				}
+			}
+		})
+	}
+}
+
 func TestFixViaRecipeURL(t *testing.T) {
 	t.Parallel()
 	e := newEnv(t)