Hide the recipe link, pick the creating suggestion, refuse duplicate positions

Dinner detail review fixes for GET /api/v1/dinners/{id}:

- source is now {site_name} with no url. A stored source_url may be a
  pasted or offered link carrying a personal token, and nothing can show
  a stored link is free of one; the candidate flow keeps every page URL
  on the server for the same reason, so the detail does too.
- why_picked comes from GetDinnerSuggestionReasons, which reads only
  pending or accepted suggestions: a suggestion is only ever created with
  a new dinner and declining one archives that dinner, so a declined row
  never created a live dinner. More than one candidate row is an error
  rather than a guess. sqlc regenerated.
- Two recipe ingredients or steps at one position leave recipe order
  unknown, so the read fails rather than ordering them by id.

Maestro task 099e9952-7f98-4700-8369-241c589a1e9d.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs/ARCHITECTURE.md

@@ -213,10 +213,15 @@ automatic** (D-021).
   raw_text}]` and `steps` the step texts, both in recipe position order and
   empty (`[]`) with no recipe, as for a dinner whose grounding is
   `pending` or `failed`; a recipe entered by hand has only `raw_text`.
-  `source` is `{url, site_name}` of the page the recipe was fetched from,
-  null for a manual recipe or none; `why_picked` is the reason of the
-  suggestion that created the dinner (its earliest), null for one no
-  suggestion created. An unknown or archived dinner is 404.
+  Two items at one position leave recipe order unknown, so such a recipe
+  is 500 rather than shown out of order. `source` is `{site_name}` of the
+  page the recipe was fetched from (`site_name` null when the page names
+  none), null for a manual recipe or none; it has no `url`, as the page's
+  link may carry a personal token. `why_picked` is the reason of the
+  suggestion that created the dinner — its pending or accepted one, as
+  declining a suggestion archives its dinner — null for one no suggestion
+  created; a dinner with more than one such suggestion is 500, as nothing
+  records which created it. An unknown or archived dinner is 404.
 - Dinner grounding. `POST /api/v1/dinners` also enqueues a `ground_dinner`
   job and answers with its `grounding_job_id` without waiting for it
   (`GET /api/v1/jobs/{id}` reports it); if the job cannot be enqueued it

internal/db/querier.go

@@ -103,9 +103,13 @@ type Querier interface {
 	// Returns sql.ErrNoRows until the row exists.
 	GetChatSettings(ctx context.Context) (ChatSetting, error)
 	GetDinner(ctx context.Context, id string) (Dinner, error)
-	// The reason of the suggestion that created dinner_id, its earliest.
-	// Returns sql.ErrNoRows for a dinner no suggestion created.
-	GetDinnerSuggestionReason(ctx context.Context, dinnerID string) (string, error)
+	// The reason of each suggestion of dinner_id that can have created it,
+	// none for a dinner no suggestion created. A suggestion is only ever
+	// created with a new dinner, and declining one archives its dinner, so a
+	// live dinner's creating suggestion is pending (the dinner still
+	// suggested) or accepted; a declined one did not create it. More than one
+	// row means nothing records which did.
+	GetDinnerSuggestionReasons(ctx context.Context, dinnerID string) ([]string, error)
 	// Returns sql.ErrNoRows until the row exists.
 	GetHouseholdSettings(ctx context.Context) (HouseholdSetting, error)
 	// Returns sql.ErrNoRows until the row exists.

internal/db/suggestions.sql.go

@@ -187,20 +187,39 @@ func (q *Queries) GetActiveSuggestionBatch(ctx context.Context) (SuggestionBatch
 	return i, err
 }
 
-const getDinnerSuggestionReason = `-- name: GetDinnerSuggestionReason :one
+const getDinnerSuggestionReasons = `-- name: GetDinnerSuggestionReasons :many
 SELECT reason FROM suggestions
-WHERE dinner_id = ?
-ORDER BY created_at, id
-LIMIT 1
+WHERE dinner_id = ? AND status IN ('pending', 'accepted')
+ORDER BY id
 `
 
-// The reason of the suggestion that created dinner_id, its earliest.
-// Returns sql.ErrNoRows for a dinner no suggestion created.
-func (q *Queries) GetDinnerSuggestionReason(ctx context.Context, dinnerID string) (string, error) {
-	row := q.db.QueryRowContext(ctx, getDinnerSuggestionReason, dinnerID)
-	var reason string
-	err := row.Scan(&reason)
-	return reason, err
+// The reason of each suggestion of dinner_id that can have created it,
+// none for a dinner no suggestion created. A suggestion is only ever
+// created with a new dinner, and declining one archives its dinner, so a
+// live dinner's creating suggestion is pending (the dinner still
+// suggested) or accepted; a declined one did not create it. More than one
+// row means nothing records which did.
+func (q *Queries) GetDinnerSuggestionReasons(ctx context.Context, dinnerID string) ([]string, error) {
+	rows, err := q.db.QueryContext(ctx, getDinnerSuggestionReasons, dinnerID)
+	if err != nil {
+		return nil, err
+	}
+	defer rows.Close()
+	var items []string
+	for rows.Next() {
+		var reason string
+		if err := rows.Scan(&reason); err != nil {
+			return nil, err
+		}
+		items = append(items, reason)
+	}
+	if err := rows.Close(); err != nil {
+		return nil, err
+	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}
+	return items, nil
 }
 
 const getLatestSuggestionBatch = `-- name: GetLatestSuggestionBatch :one

internal/dinners/detail.go

@@ -30,15 +30,16 @@ type Detail struct {
 	// the dinner has no recipe.
 	Ingredients []Ingredient `json:"ingredients"`
 	Steps       []string     `json:"steps"`
-	// Source is the recipe page the dinner is grounded in; null for a
-	// recipe entered by hand or a dinner with no recipe.
+	// Source is the site of the recipe page the dinner is grounded in;
+	// null for a recipe entered by hand or a dinner with no recipe.
 	Source          *Source `json:"source"`
 	GroundingStatus string  `json:"grounding_status"`
 	// GroundingError is why grounding failed, one of the Reason codes;
 	// null unless GroundingStatus is failed.
 	GroundingError *string `json:"grounding_error"`
-	// WhyPicked is the reason of the suggestion that created the dinner;
-	// null for a dinner no suggestion created.
+	// WhyPicked is the reason of the suggestion that created the dinner
+	// (see GetDinnerSuggestionReasons); null for a dinner no suggestion
+	// created.
 	WhyPicked *string `json:"why_picked"`
 }
 
@@ -54,9 +55,12 @@ type Ingredient struct {
 	RawText string  `json:"raw_text"`
 }
 
-// Source is the page a recipe was fetched from.
+// Source is the site a recipe was fetched from. It has no url: the page's
+// link, pasted or offered as a candidate, may carry a personal token, and
+// no stored link can be shown to be free of one, so like a dinner's
+// recipe_url and a candidate's page it never leaves the server. SiteName
+// is null when the page names no site.
 type Source struct {
-	URL      string  `json:"url"`
 	SiteName *string `json:"site_name"`
 }
 
@@ -113,13 +117,19 @@ func (s *Service) Get(ctx context.Context, id string) (d Detail, err error) {
 			return Detail{}, fmt.Errorf("dinners: read recipe of %s: %w", id, err)
 		}
 	}
-	reason, err := q.GetDinnerSuggestionReason(ctx, id)
-	switch {
-	case err == nil:
-		d.WhyPicked = &reason
-	case !errors.Is(err, sql.ErrNoRows):
+	reasons, err := q.GetDinnerSuggestionReasons(ctx, id)
+	if err != nil {
 		return Detail{}, fmt.Errorf("dinners: read suggestion reason of %s: %w", id, err)
 	}
+	switch len(reasons) {
+	case 0:
+	case 1:
+		d.WhyPicked = &reasons[0]
+	default:
+		// Nothing records which of them created the dinner, and any
+		// one picked would be a guess.
+		return Detail{}, fmt.Errorf("dinners: %s has %d suggestions that could have created it", id, len(reasons))
+	}
 	return d, nil
 }
 
@@ -130,13 +140,16 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail)
 		return fmt.Errorf("recipe %s: %w", recipeID, err)
 	}
 	if !IsManualRecipe(rec.SourceUrl) {
-		d.Source = &Source{URL: rec.SourceUrl, SiteName: nullString(rec.SiteName)}
+		d.Source = &Source{SiteName: nullString(rec.SiteName)}
 	}
 	ings, err := q.ListRecipeIngredients(ctx, recipeID)
 	if err != nil {
 		return fmt.Errorf("ingredients: %w", err)
 	}
-	for _, ing := range ings {
+	for i, ing := range ings {
+		if i > 0 && ing.Position == ings[i-1].Position {
+			return duplicatePosition("ingredient", ing.Position)
+		}
 		d.Ingredients = append(d.Ingredients, Ingredient{
 			Name:    nullString(ing.Name),
 			Qty:     nullString(ing.Qty),
@@ -149,8 +162,19 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail)
 	if err != nil {
 		return fmt.Errorf("steps: %w", err)
 	}
-	for _, step := range steps {
+	for i, step := range steps {
+		if i > 0 && step.Position == steps[i-1].Position {
+			return duplicatePosition("step", step.Position)
+		}
 		d.Steps = append(d.Steps, step.Text)
 	}
 	return nil
 }
+
+// duplicatePosition is the error for two recipe items of kind at position.
+// Position is the only record of recipe order, and the id that breaks the
+// tie says nothing about it, so a recipe with a duplicate is refused rather
+// than shown in an order nobody can trust.
+func duplicatePosition(kind string, position int64) error {
+	return fmt.Errorf("two %ss at position %d: recipe order is unknown", kind, position)
+}

internal/server/dinnerdetail_test.go

@@ -1,10 +1,12 @@
 package server
 
 import (
+	"database/sql"
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
+	"strings"
 	"testing"
 
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db"
@@ -42,7 +44,7 @@ func wantNull(t *testing.T, raw map[string]any, keys ...string) {
 
 // TestGetDinnerGroundedSideQuest: a Side quest grounded in a fetched page
 // shows its photo at both stored sizes, its recipe's ingredients and steps
-// in recipe order (not row order), its source page and the reason it was
+// in recipe order (not row order), its source site and the reason it was
 // suggested.
 func TestGetDinnerGroundedSideQuest(t *testing.T) {
 	t.Parallel()
@@ -109,7 +111,7 @@ func TestGetDinnerGroundedSideQuest(t *testing.T) {
 			{Name: str("onion"), Qty: str("1"), RawText: "1 onion"},
 		},
 		Steps:           []string{"Fry with the garlic.", "Chop the onion."},
-		Source:          &dinners.Source{URL: curryPage, SiteName: str("Curry House")},
+		Source:          &dinners.Source{SiteName: str("Curry House")},
 		GroundingStatus: "grounded",
 		WhyPicked:       str(reason),
 	}
@@ -213,3 +215,116 @@ func TestGetDinnerNotFound(t *testing.T) {
 		t.Errorf("GET without a session = %d %v, want 401", rec.Code, body)
 	}
 }
+
+// TestGetDinnerSourceHidesTokenURL: a recipe grounded in a pasted link that
+// carries a token in its path or query shows its site name, but never the
+// link or any part of it.
+func TestGetDinnerSourceHidesTokenURL(t *testing.T) {
+	t.Parallel()
+	e := newDinnersEnv(t)
+	const (
+		pathToken  = "path-token-abc123"
+		queryToken = "query-token-def456"
+		pathPage   = "https://secret-path.example/u/" + pathToken + "/chicken-curry"
+		queryPage  = "https://secret-query.example/curry?access_token=" + queryToken
+	)
+	e.pages.Bodies[pathPage] = recipestest.RecipePage("Chicken Curry", "Curry House", "https://a.example/curry.jpg", 40)
+	e.pages.Bodies[queryPage] = recipestest.RecipePage("Weeknight Curry", "B Kitchen", "https://b.example/curry.jpg", 25)
+	e.images.JPEGs["https://a.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 1)
+	e.images.JPEGs["https://b.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 2)
+	for _, tc := range []struct{ dish, page, site string }{
+		{"Chicken curry", pathPage, "Curry House"},
+		{"Weeknight curry", queryPage, "B Kitchen"},
+	} {
+		id := createDinner(t, e.h, `{"name":"`+tc.dish+`"}`) // runner not started: stays pending
+		e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25))
+		if rec, _ := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"recipe_url":"`+tc.page+`"}`); rec.Code != http.StatusOK {
+			t.Fatalf("POST ground %s = %d: %s", tc.page, rec.Code, rec.Body)
+		}
+		var stored string
+		if err := e.st.Reader.QueryRow(`SELECT source_url FROM recipes WHERE id = (SELECT recipe_id FROM dinners WHERE id = ?)`, id).Scan(&stored); err != nil {
+			t.Fatal(err)
+		}
+		if stored != tc.page {
+			t.Fatalf("stored source_url = %q, want %q", stored, tc.page)
+		}
+
+		rec, _ := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id, "")
+		if rec.Code != http.StatusOK {
+			t.Fatalf("GET dinner = %d: %s", rec.Code, rec.Body)
+		}
+		for _, secret := range []string{tc.page, "secret-path.example", "secret-query.example", pathToken, queryToken, "access_token", `"url"`} {
+			if strings.Contains(rec.Body.String(), secret) {
+				t.Errorf("detail of %s contains %q: %s", tc.dish, secret, rec.Body)
+			}
+		}
+		raw, _ := getDetail(t, e.h, id)
+		if src, _ := raw["source"].(map[string]any); !reflect.DeepEqual(src, map[string]any{"site_name": tc.site}) {
+			t.Errorf("source of %s = %v, want only site_name %q", tc.dish, raw["source"], tc.site)
+		}
+	}
+}
+
+// TestGetDinnerWhyPickedIsCreatingSuggestion: why_picked is the reason of
+// the suggestion that created the dinner, never that of an earlier
+// suggestion row for it that was declined.
+func TestGetDinnerWhyPickedIsCreatingSuggestion(t *testing.T) {
+	t.Parallel()
+	e := newDinnersEnv(t)
+	id := createDinner(t, e.h, `{"name":"Chicken curry"}`)
+	if _, err := e.st.Writer.Exec(`UPDATE dinners SET status = 'quest_log', origin = 'side_quest' WHERE id = ?`, id); err != nil {
+		t.Fatal(err)
+	}
+	q := db.New(e.st.Writer)
+	now := store.Now().String()
+	batch, err := q.CreateSuggestionBatch(t.Context(), db.CreateSuggestionBatchParams{ID: store.NewID(), Trigger: "manual", CreatedAt: now, UpdatedAt: now})
+	if err != nil {
+		t.Fatal(err)
+	}
+	const reason = "Like your Tikka masala, but quicker."
+	// The declined row is created first, so it is the earliest by both
+	// created_at and id.
+	for _, s := range []struct{ reason, status string }{
+		{"Passed on last week.", "declined"},
+		{reason, "accepted"},
+	} {
+		sid := store.NewID()
+		if err := q.CreateSuggestion(t.Context(), db.CreateSuggestionParams{
+			ID: sid, DinnerID: id, BatchID: batch.ID, Name: "Chicken curry",
+			Reason: s.reason, CitedRegularIds: "[]", CreatedAt: now, UpdatedAt: now,
+		}); err != nil {
+			t.Fatal(err)
+		}
+		if n, err := q.DecideSuggestion(t.Context(), db.DecideSuggestionParams{
+			Status: s.status, Now: sql.NullString{String: now, Valid: true}, ID: sid,
+		}); err != nil || n != 1 {
+			t.Fatalf("decide %s = %d, %v", s.status, n, err)
+		}
+	}
+
+	if _, got := getDetail(t, e.h, id); got.WhyPicked == nil || *got.WhyPicked != reason {
+		t.Errorf("why_picked = %v, want %q", got.WhyPicked, reason)
+	}
+}
+
+// TestGetDinnerDuplicatePositionFails: a recipe with two ingredients or two
+// steps at one position has no order anyone can trust, so its detail is a
+// 500 rather than the recipe in some order.
+func TestGetDinnerDuplicatePositionFails(t *testing.T) {
+	t.Parallel()
+	e := newDinnersEnv(t)
+	for _, table := range []string{"recipe_ingredients", "recipe_steps"} {
+		id := createDinner(t, e.h, `{"name":"Family chili `+table+`"}`)
+		if rec, _ := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground",
+			`{"manual":{"ingredients":["1 lb beef","1 can beans"],"steps":["Brown.","Simmer."],"minutes":240,"method":"crockpot"}}`); rec.Code != http.StatusOK {
+			t.Fatalf("POST ground = %d: %s", rec.Code, rec.Body)
+		}
+		if _, err := e.st.Writer.Exec(`UPDATE `+table+` SET position = 0 WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, id); err != nil {
+			t.Fatal(err)
+		}
+		rec, _ := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id, "")
+		if rec.Code != http.StatusInternalServerError || strings.Contains(rec.Body.String(), "Simmer.") || strings.Contains(rec.Body.String(), "1 lb beef") {
+			t.Errorf("GET with duplicate %s positions = %d: %s, want 500 and no recipe", table, rec.Code, rec.Body)
+		}
+	}
+}

migrations/queries/suggestions.sql

@@ -117,10 +117,13 @@ UPDATE dinners
 SET status = sqlc.arg(status), updated_by = sqlc.arg(updated_by), updated_at = sqlc.arg(updated_at)
 WHERE id = sqlc.arg(id) AND status = 'suggested';
 
--- name: GetDinnerSuggestionReason :one
--- The reason of the suggestion that created dinner_id, its earliest.
--- Returns sql.ErrNoRows for a dinner no suggestion created.
+-- name: GetDinnerSuggestionReasons :many
+-- The reason of each suggestion of dinner_id that can have created it,
+-- none for a dinner no suggestion created. A suggestion is only ever
+-- created with a new dinner, and declining one archives its dinner, so a
+-- live dinner's creating suggestion is pending (the dinner still
+-- suggested) or accepted; a declined one did not create it. More than one
+-- row means nothing records which did.
 SELECT reason FROM suggestions
-WHERE dinner_id = ?
-ORDER BY created_at, id
-LIMIT 1;
+WHERE dinner_id = ? AND status IN ('pending', 'accepted')
+ORDER BY id;