review: port the seeded-trial cases to the live corpus

.changeset/review-trial-corpus-cases.md

@@ -0,0 +1,9 @@
+---
+"review": patch
+---
+
+Add three live-enabled eval corpus cases porting the Khan/webapp#40678 seeded-defect trial into the review-workflow corpus. All three are sanitized structural rewrites: fresh Go code around a generic "notes retention" feature that reproduces the trial's defect mechanisms, carrying no webapp code, paths, or identifiers.
+
+- `trial-retention-deletion` (incident-repro): the deletion-path seeds; a flag-gated compliance deletion, a query-default limit of 1, a reimplemented deletion helper, an env-interface widening flagged in both files, and a swallowed prune error.
+- `trial-retention-prune-tests` (incident-repro): the prune-and-tests seeds; an off-by-one retention cap, a vacuous cap test that passes for a no-op prune, a suite-wide flag-ON mock hiding the flag-off path, and a full-entity fetch where keys-only suffices.
+- `trial-batch-delete-wrapper` (clean): the trial's deliberate non-defect as a must-not-flag trap; a large single DeleteMulti call that looks over the datastore's 500-entity cap but is chunked internally by the (unchanged, in-tree) datastore wrapper. The recorded false block is refuted by validation and the case must approve.

workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/case.json

@@ -0,0 +1,82 @@
+{
+    "id": "trial-batch-delete-wrapper",
+    "tags": [
+        "clean",
+        "trial",
+        "live"
+    ],
+    "category": "clean",
+    "description": "Sanitized structural rewrite of the trial's deliberate non-defect: purge collects every note key and issues one DeleteMulti call that looks like it exceeds the datastore's 500-entity per-call cap, but the repo's wrapper visibly chunks DeleteMulti into 500-key batches (internal/datastore/client.go, present unchanged in the tree). A reviewer must read the wrapper and stay silent; the recorded blocking claim is refuted by validation and the case must approve.",
+    "changedFiles": [
+        {
+            "path": "services/notes/purge.go",
+            "status": "modified"
+        }
+    ],
+    "dimensions": {
+        "correctness": "assessed",
+        "skillSeverity": "assessed",
+        "patternTriage": "assessed"
+    },
+    "findings": [
+        {
+            "source": "correctness",
+            "finding": {
+                "schema_version": 2,
+                "id": "purge-batch-cap-false-block",
+                "lens": "correctness",
+                "anchor": {
+                    "type": "line",
+                    "path": "services/notes/purge.go",
+                    "line": 44,
+                    "side": "RIGHT"
+                },
+                "severity": "blocking",
+                "confidence": 0.8,
+                "evidence_trace": [
+                    "services/notes/purge.go:44 passes every collected key to a single DeleteMulti call",
+                    "the hosted datastore's multi-entity operations are capped at 500 entities per call"
+                ],
+                "failure_scenario": "A user with more than 500 notes reaches DeleteMulti with an over-cap key slice; the call is rejected by the 500-entity per-call limit and account erasure fails for exactly the heaviest users.",
+                "producing_hunt": "correctness:batch-limits",
+                "model_authored_prose": "DeleteMulti receives an unbounded key slice here; above the datastore's 500-entity per-call cap the operation fails, breaking purge for heavy note-takers. Chunk the keys into batches of 500."
+            }
+        }
+    ],
+    "validation": [
+        {
+            "id": "purge-batch-cap-false-block",
+            "verification": "refuted"
+        }
+    ],
+    "expected": {
+        "verdict": "APPROVE",
+        "mustNotPost": [
+            "purge-batch-cap-false-block"
+        ],
+        "postedCommentCount": 0
+    },
+    "diff": "diff --git a/services/notes/purge.go b/services/notes/purge.go\n--- a/services/notes/purge.go\n+++ b/services/notes/purge.go\n@@ -7,27 +7,39 @@\n \t\"example.dev/notesvc/internal/datastore\"\n )\n \n-// purgePageSize is how many keys each purge pass lists and deletes.\n-const purgePageSize = 100\n+// purgePageSize is how many keys each listing page fetches.\n+const purgePageSize = 1000\n \n // PurgeUserNotes hard-deletes every note entity a user has stored.\n // The account-erasure pipeline calls it after the retention window\n // closes; nothing the user wrote may survive it.\n+//\n+// Keys are collected up front and deleted in one call: re-listing\n+// between deletes raced the store's eventually-consistent index and\n+// made the loop spin on already-deleted keys.\n func PurgeUserNotes(ctx context.Context, client *datastore.Client, userID string) error {\n+\tvar keys []datastore.Key\n+\tcursor := \"\"\n \tfor {\n \t\tpage, err := client.ListKeys(ctx, datastore.KeyQuery{\n-\t\t\tKind:  \"Note\",\n-\t\t\tOwner: userID,\n-\t\t\tLimit: purgePageSize,\n+\t\t\tKind:   \"Note\",\n+\t\t\tOwner:  userID,\n+\t\t\tLimit:  purgePageSize,\n+\t\t\tCursor: cursor,\n \t\t})\n \t\tif err != nil {\n \t\t\treturn fmt.Errorf(\"list note keys for %s: %w\", userID, err)\n \t\t}\n-\t\tif len(page.Keys) == 0 {\n-\t\t\treturn nil\n+\t\tkeys = append(keys, page.Keys...)\n+\t\tif page.Cursor == \"\" {\n+\t\t\tbreak\n \t\t}\n-\t\tif err := client.DeleteMulti(ctx, page.Keys); err != nil {\n-\t\t\treturn fmt.Errorf(\"purge notes for %s: %w\", userID, err)\n-\t\t}\n+\t\tcursor = page.Cursor\n \t}\n+\tif len(keys) == 0 {\n+\t\treturn nil\n+\t}\n+\t// A heavy note-taker can hold tens of thousands of notes; delete\n+\t// them all in one DeleteMulti call.\n+\treturn client.DeleteMulti(ctx, keys)\n }\n",
+    "live": {
+        "prContext": {
+            "title": "notes: purge note entities in one DeleteMulti pass",
+            "description": "Collects every note key up front and deletes in a single DeleteMulti call. Re-listing between per-page deletes raced the eventually-consistent key index and made the purge loop spin on already-deleted keys.",
+            "author": "dev-notes",
+            "baseBranch": "main"
+        },
+        "mustNotFlagSpecs": [
+            {
+                "key": "purge-batch-cap-false-block",
+                "path": "services/notes/purge.go",
+                "mechanism": [
+                    "500[- ](entity|key|item)?.?(cap|limit)",
+                    "exceeds? the (per[- ]call|batch|multi[- ]entity) (cap|limit)",
+                    "DeleteMulti.*(unbatched|unchunked|too (many|large)|over the (cap|limit))",
+                    "trap: the datastore wrapper chunks DeleteMulti into 500-key batches internally (internal/datastore/client.go); claiming the cap without reading the wrapper is a false block"
+                ],
+                "lineStart": 39,
+                "lineEnd": 49
+            }
+        ]
+    }
+}

workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/tree/internal/datastore/client.go

@@ -0,0 +1,66 @@
+// Package datastore wraps the hosted datastore API behind a small
+// client that hides the service's per-call operation limits.
+package datastore
+
+import (
+	"context"
+	"fmt"
+)
+
+// opBatchSize is the hosted datastore's per-call entity limit. The
+// client chunks multi-entity calls to stay under it, so callers may
+// pass arbitrarily large slices.
+const opBatchSize = 500
+
+// Key names one stored entity.
+type Key struct {
+	Kind string
+	ID   string
+}
+
+// KeyQuery selects keys of one kind for one owner.
+type KeyQuery struct {
+	Kind   string
+	Owner  string
+	Limit  int
+	Cursor string
+}
+
+// KeyPage is one page of query results.
+type KeyPage struct {
+	Keys []Key
+	// Cursor resumes the query; empty means no more results.
+	Cursor string
+}
+
+// rawAPI is the transport seam (the real service or a test fake).
+type rawAPI interface {
+	ListKeys(ctx context.Context, q KeyQuery) (KeyPage, error)
+	DeleteBatch(ctx context.Context, keys []Key) error
+}
+
+// Client is the app-facing datastore handle.
+type Client struct {
+	api rawAPI
+}
+
+// ListKeys returns one page of keys matching q.
+func (c *Client) ListKeys(ctx context.Context, q KeyQuery) (KeyPage, error) {
+	return c.api.ListKeys(ctx, q)
+}
+
+// DeleteMulti deletes every key in keys. It chunks the work into
+// opBatchSize batches internally, so callers may pass slices of any
+// length without tripping the service's per-call entity limit.
+func (c *Client) DeleteMulti(ctx context.Context, keys []Key) error {
+	for start := 0; start < len(keys); start += opBatchSize {
+		end := start + opBatchSize
+		if end > len(keys) {
+			end = len(keys)
+		}
+		if err := c.api.DeleteBatch(ctx, keys[start:end]); err != nil {
+			return fmt.Errorf("delete batch at %d: %w", start, err)
+		}
+	}
+	return nil
+}

workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/tree/services/notes/purge.go

@@ -0,0 +1,45 @@
+package notes
+
+import (
+	"context"
+	"fmt"
+
+	"example.dev/notesvc/internal/datastore"
+)
+
+// purgePageSize is how many keys each listing page fetches.
+const purgePageSize = 1000
+
+// PurgeUserNotes hard-deletes every note entity a user has stored.
+// The account-erasure pipeline calls it after the retention window
+// closes; nothing the user wrote may survive it.
+//
+// Keys are collected up front and deleted in one call: re-listing
+// between deletes raced the store's eventually-consistent index and
+// made the loop spin on already-deleted keys.
+func PurgeUserNotes(ctx context.Context, client *datastore.Client, userID string) error {
+	var keys []datastore.Key
+	cursor := ""
+	for {
+		page, err := client.ListKeys(ctx, datastore.KeyQuery{
+			Kind:   "Note",
+			Owner:  userID,
+			Limit:  purgePageSize,
+			Cursor: cursor,
+		})
+		if err != nil {
+			return fmt.Errorf("list note keys for %s: %w", userID, err)
+		}
+		keys = append(keys, page.Keys...)
+		if page.Cursor == "" {
+			break
+		}
+		cursor = page.Cursor
+	}
+	if len(keys) == 0 {
+		return nil
+	}
+	// A heavy note-taker can hold tens of thousands of notes; delete
+	// them all in one DeleteMulti call.
+	return client.DeleteMulti(ctx, keys)
+}

workflows/review/eval/corpus/incidents/trial-amplified-default-limit/case.json

@@ -0,0 +1,84 @@
+{
+    "id": "trial-amplified-default-limit",
+    "tags": [
+        "incident",
+        "trial",
+        "live"
+    ],
+    "category": "incident-repro",
+    "description": "Sanitized structural rewrite of the amplification/provenance behavior test (Khan/webapp#40736): the PR drops an 'unnecessary' Limit from the digest read, routing it into a pre-existing store default of one row (store.go documents Query.Limit zero as 1, NOT unlimited), so every digest silently shrinks from five notes to one. The defect mechanism predates the diff; the removal is what amplifies it, and the finding must attribute the regression to the removal (the sibling RecentPins read keeps its explicit Limit, and the PR description mischaracterizes the dropped limit as redundant). The same diff also cosmetically rewords the default-limit doc lines in store.go: a touched, non-amplified pre-existing mechanism that must NOT draw a blocking finding; the expected posted-comment count enforces that. The v1.4.0-era live run passed this shape; the case locks the amplification confirm rule, the provenance gate, and the introduce-vs-amplify prose labeling (Khan/actions#250) in regression.",
+    "changedFiles": [
+        {
+            "path": "services/notes/digest.go",
+            "status": "modified"
+        },
+        {
+            "path": "services/notes/store.go",
+            "status": "modified"
+        }
+    ],
+    "dimensions": {
+        "correctness": "assessed",
+        "skillSeverity": "assessed",
+        "patternTriage": "assessed"
+    },
+    "findings": [
+        {
+            "source": "correctness",
+            "finding": {
+                "schema_version": 2,
+                "id": "digest-amplified-default-limit",
+                "lens": "correctness",
+                "anchor": {
+                    "type": "line",
+                    "path": "services/notes/digest.go",
+                    "line": 23,
+                    "side": "RIGHT"
+                },
+                "severity": "blocking",
+                "confidence": 0.8,
+                "evidence_trace": [
+                    "services/notes/digest.go:21-24 drops Limit from the digest read; Query.Limit is now zero",
+                    "services/notes/store.go documents the pre-existing default: Limit zero means 1 (NOT unlimited)",
+                    "services/notes/digest.go:38-42: the sibling RecentPins read keeps Limit: digestSize, so the removal changes behavior rather than removing redundancy",
+                    "the PR description calls the limit redundant; Kind narrows which notes match, not how many return"
+                ],
+                "failure_scenario": "Any user with more than one summary note opens the home surface after this change: BuildDigest's query falls back to the store's default limit of one, and the digest renders a single note instead of the five the feature promises, with no error anywhere.",
+                "producing_hunt": "correctness:removed-behavior-audit",
+                "model_authored_prose": "Removing `Limit: digestSize` does not remove a redundancy; it amplifies a pre-existing mechanism. `Query.Limit` zero falls back to the store default of one (store.go), a default that predates this change; the removal is what routes the digest read into it, silently shrinking every digest from five notes to one. The sibling `RecentPins` read keeps its explicit limit. Restore `Limit: digestSize` here."
+            }
+        }
+    ],
+    "expected": {
+        "verdict": "REQUEST_CHANGES",
+        "mustCatch": [
+            "digest-amplified-default-limit"
+        ],
+        "postedCommentCount": 1
+    },
+    "diff": "diff --git a/services/notes/digest.go b/services/notes/digest.go\nindex 39c2b1c..4ffbbcd 100644\n--- a/services/notes/digest.go\n+++ b/services/notes/digest.go\n@@ -21,7 +21,6 @@ func BuildDigest(ctx context.Context, env digestEnv, userID string) (string, err\n \tsummaries, err := env.Store().Run(ctx, Query{\n \t\tUserID: userID,\n \t\tKind:   \"summary\",\n-\t\tLimit:  digestSize,\n \t})\n \tif err != nil {\n \t\treturn \"\", fmt.Errorf(\"list summaries for digest of %s: %w\", userID, err)\ndiff --git a/services/notes/store.go b/services/notes/store.go\nindex 6f47ef1..ef64ca2 100644\n--- a/services/notes/store.go\n+++ b/services/notes/store.go\n@@ -25,8 +25,9 @@ type Query struct {\n \t// Kind, when non-empty, selects only notes of that kind.\n \tKind string\n \t// Limit caps the number of rows returned. Zero means 1 (the\n-\t// store's default, tuned for the common latest-note lookup),\n-\t// NOT unlimited; callers that want more must set it.\n+\t// store's default, tuned for the common latest-note lookup and\n+\t// the cheapest read), NOT unlimited; callers that want more\n+\t// must set it explicitly.\n \tLimit int\n \t// KeysOnly returns notes with only ID populated, skipping the\n \t// entity bodies. Much cheaper when the caller needs keys alone.\n",
+    "live": {
+        "prContext": {
+            "title": "notes: tidy digest query defaults; drop a redundant limit",
+            "description": "The digest read already narrows to Kind=\"summary\", so the explicit Limit duplicated what the query narrows anyway; drop it. Also tightens the Query.Limit doc wording while in the area.",
+            "author": "dev-notes",
+            "baseBranch": "main"
+        },
+        "mustCatchSpecs": [
+            {
+                "key": "digest-amplified-default-limit",
+                "path": "services/notes/digest.go",
+                "mechanism": [
+                    "remov(e[sd]?|al|ing).{0,40}[Ll]imit",
+                    "default.{0,40}(limit.{0,12}(of )?(one|1)|(one|1|single) (row|note|result))",
+                    "(digest|summar(y|ies)).{0,60}(one|1|single) (note|row|result)",
+                    "amplif|pre-?existing|predates|already (there|present|defaulted)",
+                    "not (a )?redundan|behavio(r|ur) change|does not preserve"
+                ],
+                "lens": "correctness",
+                "lineStart": 19,
+                "lineEnd": 27
+            }
+        ]
+    }
+}

workflows/review/eval/corpus/incidents/trial-amplified-default-limit/tree/services/notes/digest.go

@@ -0,0 +1,47 @@
+package notes
+
+import (
+	"context"
+	"fmt"
+	"strings"
+)
+
+// digestSize is how many recent notes the digest and sidebar surface.
+const digestSize = 5
+
+// digestEnv is the slice of the request environment digests need,
+// kept local so this file names only what it uses.
+type digestEnv interface {
+	Store() Store
+}
+
+// BuildDigest renders the user's recent summary notes as one block
+// for the home surface, newest first.
+func BuildDigest(ctx context.Context, env digestEnv, userID string) (string, error) {
+	summaries, err := env.Store().Run(ctx, Query{
+		UserID: userID,
+		Kind:   "summary",
+	})
+	if err != nil {
+		return "", fmt.Errorf("list summaries for digest of %s: %w", userID, err)
+	}
+	lines := make([]string, 0, len(summaries))
+	for _, note := range summaries {
+		lines = append(lines, "- "+note.Body)
+	}
+	return strings.Join(lines, "\n"), nil
+}
+
+// RecentPins lists the user's pinned notes for the sidebar, newest
+// first, capped to the sidebar's five slots.
+func RecentPins(ctx context.Context, env digestEnv, userID string) ([]Note, error) {
+	pins, err := env.Store().Run(ctx, Query{
+		UserID: userID,
+		Kind:   "pin",
+		Limit:  digestSize,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("list pins for %s: %w", userID, err)
+	}
+	return pins, nil
+}

workflows/review/eval/corpus/incidents/trial-amplified-default-limit/tree/services/notes/store.go

@@ -0,0 +1,45 @@
+// Package notes stores per-user study notes and enforces the
+// retention policy over them.
+package notes
+
+import (
+	"context"
+	"time"
+)
+
+// Note is one stored per-user note.
+type Note struct {
+	ID     string
+	UserID string
+	// Kind distinguishes what a note is: "note" for user-authored
+	// text, "summary" for generated study summaries. Readers select
+	// on it, so notes of different kinds may share a Body.
+	Kind      string
+	Body      string
+	CreatedAt time.Time
+}
+
+// Query selects notes for one user, newest first.
+type Query struct {
+	UserID string
+	// Kind, when non-empty, selects only notes of that kind.
+	Kind string
+	// Limit caps the number of rows returned. Zero means 1 (the
+	// store's default, tuned for the common latest-note lookup and
+	// the cheapest read), NOT unlimited; callers that want more
+	// must set it explicitly.
+	Limit int
+	// KeysOnly returns notes with only ID populated, skipping the
+	// entity bodies. Much cheaper when the caller needs keys alone.
+	KeysOnly bool
+}
+
+// Store is the persistence surface the notes package reads and writes.
+type Store interface {
+	// Run executes the query and returns the matching notes.
+	Run(ctx context.Context, q Query) ([]Note, error)
+	// Put stores the given notes, assigning IDs to new ones.
+	Put(ctx context.Context, notes []Note) error
+	// Delete removes the notes with the given IDs.
+	Delete(ctx context.Context, ids []string) error
+}