diff --git a/.changeset/review-trial-corpus-cases.md b/.changeset/review-trial-corpus-cases.md new file mode 100644 index 00000000..f28f8cb0 --- /dev/null +++ b/.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. diff --git a/workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/case.json b/workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/case.json new file mode 100644 index 00000000..1d6dd032 --- /dev/null +++ b/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 + } + ] + } +} diff --git a/workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/tree/internal/datastore/client.go b/workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/tree/internal/datastore/client.go new file mode 100644 index 00000000..90d1698f --- /dev/null +++ b/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 +} diff --git a/workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/tree/services/notes/purge.go b/workflows/review/eval/corpus/clean/trial-batch-delete-wrapper/tree/services/notes/purge.go new file mode 100644 index 00000000..422a2a15 --- /dev/null +++ b/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) +} diff --git a/workflows/review/eval/corpus/incidents/trial-amplified-default-limit/case.json b/workflows/review/eval/corpus/incidents/trial-amplified-default-limit/case.json new file mode 100644 index 00000000..2b8e18cc --- /dev/null +++ b/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 + } + ] + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-amplified-default-limit/tree/services/notes/digest.go b/workflows/review/eval/corpus/incidents/trial-amplified-default-limit/tree/services/notes/digest.go new file mode 100644 index 00000000..4ffbbcdf --- /dev/null +++ b/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 +} diff --git a/workflows/review/eval/corpus/incidents/trial-amplified-default-limit/tree/services/notes/store.go b/workflows/review/eval/corpus/incidents/trial-amplified-default-limit/tree/services/notes/store.go new file mode 100644 index 00000000..ef64ca28 --- /dev/null +++ b/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 +} diff --git a/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/case.json b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/case.json new file mode 100644 index 00000000..e8452eae --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/case.json @@ -0,0 +1,84 @@ +{ + "id": "trial-dedup-composite-key", + "tags": [ + "incident", + "trial", + "live", + "smoke" + ], + "category": "incident-repro", + "description": "Sanitized structural rewrite of a v1.4.0 re-run miss (dedup push): the new save path keys its duplicate check on Note.Body alone while the Note entity carries a Kind that readers select on (the store docs say notes of different kinds may share a Body), so a same-Body note of a different kind is silently never saved. The prior reviewer version caught this; v1.4.0 missed it at the finder level.", + "changedFiles": [ + { + "path": "services/notes/save.go", + "status": "added" + }, + { + "path": "services/notes/save_test.go", + "status": "added" + } + ], + "dimensions": { + "correctness": "assessed", + "skillSeverity": "assessed", + "patternTriage": "assessed" + }, + "findings": [ + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "save-dedup-composite-key", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "services/notes/save.go", + "line": 32, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.8, + "evidence_trace": [ + "services/notes/save.go:32 keys the dedup set on note.Body alone", + "services/notes/store.go documents Kind as a field readers select on: notes of different kinds may share a Body", + "a fresh note whose Body matches an existing note of a different Kind lands in seen and is silently skipped" + ], + "failure_scenario": "A user stores a Kind=\"note\" entry; later the summary pipeline saves a Kind=\"summary\" note with the same Body. SaveNotes keys dedup on Body alone, so the summary is silently dropped (no error, no write) and readers querying Kind=\"summary\" find nothing, though the caller was told the save succeeded.", + "producing_hunt": "correctness:dedup-key-completeness", + "model_authored_prose": "Dedup keys on `note.Body` alone, but `Note.Kind` is part of note identity: readers select on it, and the store docs say notes of different kinds may share a Body. Key the seen set on the (Kind, Body) pair, or dedup per kind." + } + } + ], + "expected": { + "verdict": "REQUEST_CHANGES", + "mustCatch": [ + "save-dedup-composite-key" + ], + "postedCommentCount": 1 + }, + "diff": "diff --git a/services/notes/save.go b/services/notes/save.go\nnew file mode 100644\n--- /dev/null\n+++ b/services/notes/save.go\n@@ -0,0 +1,49 @@\n+package notes\n+\n+import (\n+\t\"context\"\n+\t\"fmt\"\n+)\n+\n+// dedupCheckLimit bounds the dedup read to the retention cap; a user\n+// never retains more notes than that, so reading this many covers\n+// everything a new note could duplicate.\n+const dedupCheckLimit = 200\n+\n+// saveEnv is the slice of the request environment saving needs,\n+// kept local so this file names only what it uses.\n+type saveEnv interface {\n+\tStore() Store\n+}\n+\n+// SaveNotes stores the given notes for userID. A note the user\n+// already has is skipped, so repeated submissions of the same\n+// content do not pile up duplicate entries.\n+func SaveNotes(ctx context.Context, env saveEnv, userID string, notes []Note) error {\n+\texisting, err := env.Store().Run(ctx, Query{\n+\t\tUserID: userID,\n+\t\tLimit: dedupCheckLimit,\n+\t})\n+\tif err != nil {\n+\t\treturn fmt.Errorf(\"list notes for dedup of %s: %w\", userID, err)\n+\t}\n+\tseen := make(map[string]bool, len(existing))\n+\tfor _, note := range existing {\n+\t\tseen[note.Body] = true\n+\t}\n+\tfresh := make([]Note, 0, len(notes))\n+\tfor _, note := range notes {\n+\t\tif seen[note.Body] {\n+\t\t\tcontinue\n+\t\t}\n+\t\tseen[note.Body] = true\n+\t\tfresh = append(fresh, note)\n+\t}\n+\tif len(fresh) == 0 {\n+\t\treturn nil\n+\t}\n+\tif err := env.Store().Put(ctx, fresh); err != nil {\n+\t\treturn fmt.Errorf(\"save notes for %s: %w\", userID, err)\n+\t}\n+\treturn nil\n+}\ndiff --git a/services/notes/save_test.go b/services/notes/save_test.go\nnew file mode 100644\n--- /dev/null\n+++ b/services/notes/save_test.go\n@@ -0,0 +1,97 @@\n+package notes\n+\n+import (\n+\t\"context\"\n+\t\"fmt\"\n+\t\"testing\"\n+)\n+\n+// saveStore is an in-memory Store for the save tests.\n+type saveStore struct {\n+\tnotes []Note\n+\tnextID int\n+}\n+\n+func (s *saveStore) Run(_ context.Context, q Query) ([]Note, error) {\n+\tlimit := q.Limit\n+\tif limit == 0 {\n+\t\tlimit = 1\n+\t}\n+\tvar out []Note\n+\tfor _, note := range s.notes {\n+\t\tif note.UserID != q.UserID {\n+\t\t\tcontinue\n+\t\t}\n+\t\tif q.Kind != \"\" && note.Kind != q.Kind {\n+\t\t\tcontinue\n+\t\t}\n+\t\tout = append(out, note)\n+\t\tif len(out) == limit {\n+\t\t\tbreak\n+\t\t}\n+\t}\n+\treturn out, nil\n+}\n+\n+func (s *saveStore) Put(_ context.Context, notes []Note) error {\n+\tfor _, note := range notes {\n+\t\ts.nextID++\n+\t\tnote.ID = fmt.Sprintf(\"note-%d\", s.nextID)\n+\t\ts.notes = append(s.notes, note)\n+\t}\n+\treturn nil\n+}\n+\n+func (s *saveStore) Delete(_ context.Context, ids []string) error {\n+\tdrop := make(map[string]bool, len(ids))\n+\tfor _, id := range ids {\n+\t\tdrop[id] = true\n+\t}\n+\tkept := s.notes[:0]\n+\tfor _, note := range s.notes {\n+\t\tif !drop[note.ID] {\n+\t\t\tkept = append(kept, note)\n+\t\t}\n+\t}\n+\ts.notes = kept\n+\treturn nil\n+}\n+\n+// saveTestEnv bundles the fakes behind the saveEnv interface.\n+type saveTestEnv struct {\n+\tstore *saveStore\n+}\n+\n+func (e *saveTestEnv) Store() Store { return e.store }\n+\n+func TestSaveSkipsDuplicates(t *testing.T) {\n+\tenv := &saveTestEnv{store: &saveStore{}}\n+\tnote := Note{\n+\t\tUserID: \"user-1\",\n+\t\tKind: \"note\",\n+\t\tBody: \"reread chapter three before the quiz\",\n+\t}\n+\tif err := SaveNotes(context.Background(), env, \"user-1\", []Note{note}); err != nil {\n+\t\tt.Fatalf(\"SaveNotes: %v\", err)\n+\t}\n+\tif err := SaveNotes(context.Background(), env, \"user-1\", []Note{note}); err != nil {\n+\t\tt.Fatalf(\"SaveNotes: %v\", err)\n+\t}\n+\tif got := len(env.store.notes); got != 1 {\n+\t\tt.Fatalf(\"duplicate save stored %d notes, want 1\", got)\n+\t}\n+}\n+\n+func TestSaveStoresDistinctNotes(t *testing.T) {\n+\tenv := &saveTestEnv{store: &saveStore{}}\n+\tnotes := []Note{\n+\t\t{UserID: \"user-1\", Kind: \"note\", Body: \"reread chapter three before the quiz\"},\n+\t\t{UserID: \"user-1\", Kind: \"note\", Body: \"ask about the second practice set\"},\n+\t}\n+\tif err := SaveNotes(context.Background(), env, \"user-1\", notes); err != nil {\n+\t\tt.Fatalf(\"SaveNotes: %v\", err)\n+\t}\n+\tif got := len(env.store.notes); got != 2 {\n+\t\tt.Fatalf(\"stored %d notes, want 2\", got)\n+\t}\n+}\n", + "live": { + "prContext": { + "title": "notes: skip duplicate notes on save", + "description": "Saving the same note across sessions piles up identical entries. SaveNotes now skips notes the user already has among their recent notes before writing. Includes tests against an in-memory store.", + "author": "dev-notes", + "baseBranch": "main" + }, + "mustCatchSpecs": [ + { + "key": "save-dedup-composite-key", + "path": "services/notes/save.go", + "mechanism": [ + "key(s|ed)? (the )?(dedup|seen|duplicate).{0,20} on (note\\.)?Body (alone|only)", + "ignor(es?|ing) (note\\.)?Kind", + "(same|identical|shared?) Body.{0,40}(different|distinct|another) [Kk]ind", + "composite.{0,20}(key|identity)", + "(dedup|duplicate check).{0,40}(drops?|skips?|suppress).{0,40}(summary|kind)" + ], + "lens": "correctness", + "lineStart": 27, + "lineEnd": 37 + } + ] + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/save.go b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/save.go new file mode 100644 index 00000000..aca0408c --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/save.go @@ -0,0 +1,49 @@ +package notes + +import ( + "context" + "fmt" +) + +// dedupCheckLimit bounds the dedup read to the retention cap; a user +// never retains more notes than that, so reading this many covers +// everything a new note could duplicate. +const dedupCheckLimit = 200 + +// saveEnv is the slice of the request environment saving needs, +// kept local so this file names only what it uses. +type saveEnv interface { + Store() Store +} + +// SaveNotes stores the given notes for userID. A note the user +// already has is skipped, so repeated submissions of the same +// content do not pile up duplicate entries. +func SaveNotes(ctx context.Context, env saveEnv, userID string, notes []Note) error { + existing, err := env.Store().Run(ctx, Query{ + UserID: userID, + Limit: dedupCheckLimit, + }) + if err != nil { + return fmt.Errorf("list notes for dedup of %s: %w", userID, err) + } + seen := make(map[string]bool, len(existing)) + for _, note := range existing { + seen[note.Body] = true + } + fresh := make([]Note, 0, len(notes)) + for _, note := range notes { + if seen[note.Body] { + continue + } + seen[note.Body] = true + fresh = append(fresh, note) + } + if len(fresh) == 0 { + return nil + } + if err := env.Store().Put(ctx, fresh); err != nil { + return fmt.Errorf("save notes for %s: %w", userID, err) + } + return nil +} diff --git a/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/save_test.go b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/save_test.go new file mode 100644 index 00000000..6dace948 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/save_test.go @@ -0,0 +1,97 @@ +package notes + +import ( + "context" + "fmt" + "testing" +) + +// saveStore is an in-memory Store for the save tests. +type saveStore struct { + notes []Note + nextID int +} + +func (s *saveStore) Run(_ context.Context, q Query) ([]Note, error) { + limit := q.Limit + if limit == 0 { + limit = 1 + } + var out []Note + for _, note := range s.notes { + if note.UserID != q.UserID { + continue + } + if q.Kind != "" && note.Kind != q.Kind { + continue + } + out = append(out, note) + if len(out) == limit { + break + } + } + return out, nil +} + +func (s *saveStore) Put(_ context.Context, notes []Note) error { + for _, note := range notes { + s.nextID++ + note.ID = fmt.Sprintf("note-%d", s.nextID) + s.notes = append(s.notes, note) + } + return nil +} + +func (s *saveStore) Delete(_ context.Context, ids []string) error { + drop := make(map[string]bool, len(ids)) + for _, id := range ids { + drop[id] = true + } + kept := s.notes[:0] + for _, note := range s.notes { + if !drop[note.ID] { + kept = append(kept, note) + } + } + s.notes = kept + return nil +} + +// saveTestEnv bundles the fakes behind the saveEnv interface. +type saveTestEnv struct { + store *saveStore +} + +func (e *saveTestEnv) Store() Store { return e.store } + +func TestSaveSkipsDuplicates(t *testing.T) { + env := &saveTestEnv{store: &saveStore{}} + note := Note{ + UserID: "user-1", + Kind: "note", + Body: "reread chapter three before the quiz", + } + if err := SaveNotes(context.Background(), env, "user-1", []Note{note}); err != nil { + t.Fatalf("SaveNotes: %v", err) + } + if err := SaveNotes(context.Background(), env, "user-1", []Note{note}); err != nil { + t.Fatalf("SaveNotes: %v", err) + } + if got := len(env.store.notes); got != 1 { + t.Fatalf("duplicate save stored %d notes, want 1", got) + } +} + +func TestSaveStoresDistinctNotes(t *testing.T) { + env := &saveTestEnv{store: &saveStore{}} + notes := []Note{ + {UserID: "user-1", Kind: "note", Body: "reread chapter three before the quiz"}, + {UserID: "user-1", Kind: "note", Body: "ask about the second practice set"}, + } + if err := SaveNotes(context.Background(), env, "user-1", notes); err != nil { + t.Fatalf("SaveNotes: %v", err) + } + if got := len(env.store.notes); got != 2 { + t.Fatalf("stored %d notes, want 2", got) + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/store.go b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/store.go new file mode 100644 index 00000000..6f47ef18 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/store.go @@ -0,0 +1,44 @@ +// 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), + // NOT unlimited; callers that want more must set it. + 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 +} diff --git a/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/case.json b/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/case.json new file mode 100644 index 00000000..aa26b5e6 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/case.json @@ -0,0 +1,79 @@ +{ + "id": "trial-dedup-eventual-consistency", + "tags": [ + "incident", + "trial", + "live" + ], + "category": "incident-repro", + "description": "Sanitized structural rewrite of a v1.4.0 re-run miss (dedup push): the save path dedups against a read of the user's recent notes, but the store documents Run as eventually consistent, so a duplicate submitted moments after the original (a retry or double-click, the exact traffic dedup exists for) reads a stale set and is stored again. In the re-run the skill auditor investigated this mechanism and declined to report it for want of a quotable skill rule, and no correctness lens ever surfaced it; ground truth here is that a skill-adjacent correctness issue must surface regardless of which lens owns it.", + "changedFiles": [ + { + "path": "services/notes/save.go", + "status": "added" + } + ], + "dimensions": { + "correctness": "assessed", + "skillSeverity": "assessed", + "patternTriage": "assessed" + }, + "findings": [ + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "save-dedup-stale-read", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "services/notes/save.go", + "line": 23, + "side": "RIGHT" + }, + "severity": "advisory", + "confidence": 0.65, + "evidence_trace": [ + "services/notes/save.go:23 reads the user's notes to build the dedup set immediately before writing", + "services/notes/store.go documents Run as eventually consistent: a note stored by a recent Put can take a short time to become visible", + "the SaveNotes doc comment names retried and double-submitted saves as the dedup's target, which is exactly the window where the read is stale" + ], + "failure_scenario": "A client double-submits the same note a second apart; the second SaveNotes call's dedup Run does not yet see the first call's Put, so seen misses the note's Body and the duplicate is written. The dedup silently fails for precisely the rapid-resubmission traffic it was added to stop.", + "producing_hunt": "correctness:read-after-write-consistency", + "model_authored_prose": "This dedup is read-check-write over an eventually consistent Run (per the Store docs), so it cannot see writes from the last few moments, and rapid resubmission is the main duplicate source. Consider a strongly consistent read if the store offers one, an idempotency key on the write path, or documenting the dedup as best-effort." + } + } + ], + "expected": { + "verdict": "APPROVE", + "mustCatch": [ + "save-dedup-stale-read" + ], + "postedCommentCount": 1 + }, + "diff": "diff --git a/services/notes/save.go b/services/notes/save.go\nnew file mode 100644\n--- /dev/null\n+++ b/services/notes/save.go\n@@ -0,0 +1,49 @@\n+package notes\n+\n+import (\n+\t\"context\"\n+\t\"fmt\"\n+)\n+\n+// dedupCheckLimit bounds the dedup read to the retention cap; a user\n+// never retains more notes than that, so reading this many covers\n+// everything a new note could duplicate.\n+const dedupCheckLimit = 200\n+\n+// saveEnv is the slice of the request environment saving needs,\n+// kept local so this file names only what it uses.\n+type saveEnv interface {\n+\tStore() Store\n+}\n+\n+// SaveNotes stores the given notes for userID. A note the user\n+// already has is skipped, so a save submitted twice (a retried\n+// request, a double-click) does not pile up duplicate entries.\n+func SaveNotes(ctx context.Context, env saveEnv, userID string, notes []Note) error {\n+\texisting, err := env.Store().Run(ctx, Query{\n+\t\tUserID: userID,\n+\t\tLimit: dedupCheckLimit,\n+\t})\n+\tif err != nil {\n+\t\treturn fmt.Errorf(\"list notes for dedup of %s: %w\", userID, err)\n+\t}\n+\tseen := make(map[string]bool, len(existing))\n+\tfor _, note := range existing {\n+\t\tseen[note.Body] = true\n+\t}\n+\tfresh := make([]Note, 0, len(notes))\n+\tfor _, note := range notes {\n+\t\tif seen[note.Body] {\n+\t\t\tcontinue\n+\t\t}\n+\t\tseen[note.Body] = true\n+\t\tfresh = append(fresh, note)\n+\t}\n+\tif len(fresh) == 0 {\n+\t\treturn nil\n+\t}\n+\tif err := env.Store().Put(ctx, fresh); err != nil {\n+\t\treturn fmt.Errorf(\"save notes for %s: %w\", userID, err)\n+\t}\n+\treturn nil\n+}\n", + "live": { + "prContext": { + "title": "notes: skip duplicate notes on save", + "description": "A retried or double-submitted save currently stores the same note twice. SaveNotes now reads the user's recent notes and skips any it already has before writing. Tests land with the retention suite.", + "author": "dev-notes", + "baseBranch": "main" + }, + "mustCatchSpecs": [ + { + "key": "save-dedup-stale-read", + "path": "services/notes/save.go", + "mechanism": [ + "eventual(ly)? consisten(t|cy)", + "stale (read|dedup|seen|set)", + "read[- ]after[- ]write", + "recent (Put|write).{0,40}not (yet )?(visible|seen)", + "(double[- ]?(click|submit)|retr(y|ied)|resubmi).{0,60}duplicate" + ], + "lens": "correctness", + "lineStart": 18, + "lineEnd": 28 + } + ] + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/tree/services/notes/save.go b/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/tree/services/notes/save.go new file mode 100644 index 00000000..2f04122f --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/tree/services/notes/save.go @@ -0,0 +1,49 @@ +package notes + +import ( + "context" + "fmt" +) + +// dedupCheckLimit bounds the dedup read to the retention cap; a user +// never retains more notes than that, so reading this many covers +// everything a new note could duplicate. +const dedupCheckLimit = 200 + +// saveEnv is the slice of the request environment saving needs, +// kept local so this file names only what it uses. +type saveEnv interface { + Store() Store +} + +// SaveNotes stores the given notes for userID. A note the user +// already has is skipped, so a save submitted twice (a retried +// request, a double-click) does not pile up duplicate entries. +func SaveNotes(ctx context.Context, env saveEnv, userID string, notes []Note) error { + existing, err := env.Store().Run(ctx, Query{ + UserID: userID, + Limit: dedupCheckLimit, + }) + if err != nil { + return fmt.Errorf("list notes for dedup of %s: %w", userID, err) + } + seen := make(map[string]bool, len(existing)) + for _, note := range existing { + seen[note.Body] = true + } + fresh := make([]Note, 0, len(notes)) + for _, note := range notes { + if seen[note.Body] { + continue + } + seen[note.Body] = true + fresh = append(fresh, note) + } + if len(fresh) == 0 { + return nil + } + if err := env.Store().Put(ctx, fresh); err != nil { + return fmt.Errorf("save notes for %s: %w", userID, err) + } + return nil +} diff --git a/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/tree/services/notes/store.go b/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/tree/services/notes/store.go new file mode 100644 index 00000000..aa04ee0e --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/tree/services/notes/store.go @@ -0,0 +1,41 @@ +// 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 + Body string + CreatedAt time.Time +} + +// Query selects notes for one user, newest first. +type Query struct { + UserID string + // Limit caps the number of rows returned. Zero means 1 (the + // store's default, tuned for the common latest-note lookup), + // NOT unlimited; callers that want more must set it. + 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. Reads + // are eventually consistent: a note stored by a recent Put can + // take a short time to become visible to Run. + Run(ctx context.Context, q Query) ([]Note, error) + // Put stores the given notes, assigning IDs to new ones. Writes + // are durable once Put returns; see Run for read visibility. + Put(ctx context.Context, notes []Note) error + // Delete removes the notes with the given IDs. + Delete(ctx context.Context, ids []string) error +} diff --git a/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/case.json b/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/case.json new file mode 100644 index 00000000..2e8c2859 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/case.json @@ -0,0 +1,79 @@ +{ + "id": "trial-erasure-suite-flag-mock", + "tags": [ + "incident", + "trial", + "live", + "smoke" + ], + "category": "incident-repro", + "description": "Sanitized structural rewrite of a v1.4.0 re-run miss (first push): the new erasure test suite pins the retention rollout flag on inside the shared test-env constructor, so no test exercises the flag-off path, where EraseUser (unchanged in this PR) silently skips the compliance deletion; flag off is the production default while the rollout is in progress. The pre-existing flag gate itself sits outside the diff, so under the change-provenance discipline it belongs in the artifact, not the posted review; the postable defect is the test gap.", + "changedFiles": [ + { + "path": "services/notes/erasure_test.go", + "status": "added" + } + ], + "dimensions": { + "correctness": "assessed", + "skillSeverity": "assessed", + "patternTriage": "assessed" + }, + "findings": [ + { + "source": "test-adequacy", + "finding": { + "schema_version": 2, + "id": "erasure-suite-flag-mock", + "lens": "test-adequacy", + "anchor": { + "type": "line", + "path": "services/notes/erasure_test.go", + "line": 75, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.8, + "evidence_trace": [ + "services/notes/erasure_test.go:75 forces notes-retention-enabled on inside newEraseTestEnv, which every test in the suite uses", + "the flag-off early return in EraseUser is therefore never executed by any test", + "flag off is the production default while the rollout is in progress, and on this path EraseUser silently skips account-erasure deletion" + ], + "failure_scenario": "Every erasure test runs with the flag forced on, so no test observes that flag-off makes EraseUser return nil without deleting anything; the production-default path of a compliance deletion ships unexercised, and a regression that widens the gate stays invisible to the suite.", + "producing_hunt": "test-adequacy:flag-coverage", + "model_authored_prose": "newEraseTestEnv pins notes-retention-enabled on for every erasure test, hiding the flag-off path, which is the production default during rollout and where EraseUser silently skips deletion. At least one test should run with the flag off and assert what is (and is not) supposed to happen." + } + } + ], + "expected": { + "verdict": "REQUEST_CHANGES", + "mustCatch": [ + "erasure-suite-flag-mock" + ], + "postedCommentCount": 1 + }, + "diff": "diff --git a/services/notes/erasure_test.go b/services/notes/erasure_test.go\nnew file mode 100644\n--- /dev/null\n+++ b/services/notes/erasure_test.go\n@@ -0,0 +1,105 @@\n+package notes\n+\n+import (\n+\t\"context\"\n+\t\"fmt\"\n+\t\"testing\"\n+)\n+\n+// eraseFlags is the FlagSet the erasure tests run under.\n+type eraseFlags struct {\n+\tforced map[string]bool\n+}\n+\n+func (f *eraseFlags) Enabled(_ context.Context, name string) bool {\n+\treturn f.forced[name]\n+}\n+\n+// eraseStore is an in-memory Store for the erasure tests.\n+type eraseStore struct {\n+\tnotes []Note\n+}\n+\n+func (s *eraseStore) Run(_ context.Context, q Query) ([]Note, error) {\n+\tlimit := q.Limit\n+\tif limit == 0 {\n+\t\tlimit = 1\n+\t}\n+\tvar out []Note\n+\tfor _, note := range s.notes {\n+\t\tif note.UserID != q.UserID {\n+\t\t\tcontinue\n+\t\t}\n+\t\tif q.KeysOnly {\n+\t\t\tnote = Note{ID: note.ID}\n+\t\t}\n+\t\tout = append(out, note)\n+\t\tif len(out) == limit {\n+\t\t\tbreak\n+\t\t}\n+\t}\n+\treturn out, nil\n+}\n+\n+func (s *eraseStore) Delete(_ context.Context, ids []string) error {\n+\tdrop := make(map[string]bool, len(ids))\n+\tfor _, id := range ids {\n+\t\tdrop[id] = true\n+\t}\n+\tkept := s.notes[:0]\n+\tfor _, note := range s.notes {\n+\t\tif !drop[note.ID] {\n+\t\t\tkept = append(kept, note)\n+\t\t}\n+\t}\n+\ts.notes = kept\n+\treturn nil\n+}\n+\n+// eraseTestEnv bundles the fakes behind the eraseEnv interface.\n+type eraseTestEnv struct {\n+\tstore *eraseStore\n+\tflags *eraseFlags\n+}\n+\n+func (e *eraseTestEnv) Store() Store { return e.store }\n+\n+func (e *eraseTestEnv) Flags() FlagSet { return e.flags }\n+\n+// newEraseTestEnv returns the env every erasure test runs under.\n+// Retention is rolling out everywhere; run the suite with the flag\n+// on, as production will be.\n+func newEraseTestEnv() *eraseTestEnv {\n+\treturn &eraseTestEnv{\n+\t\tstore: &eraseStore{},\n+\t\tflags: &eraseFlags{forced: map[string]bool{retentionFlag: true}},\n+\t}\n+}\n+\n+func seedNotes(env *eraseTestEnv, n int) {\n+\tfor i := 0; i < n; i++ {\n+\t\tenv.store.notes = append(env.store.notes, Note{\n+\t\t\tID: fmt.Sprintf(\"note-%d\", i),\n+\t\t\tUserID: \"user-1\",\n+\t\t})\n+\t}\n+}\n+\n+func TestEraseUserRemovesAllNotes(t *testing.T) {\n+\tenv := newEraseTestEnv()\n+\t// Enough notes to span two deletion pages.\n+\tseedNotes(env, erasePageSize+250)\n+\tif err := EraseUser(context.Background(), env, \"user-1\"); err != nil {\n+\t\tt.Fatalf(\"EraseUser: %v\", err)\n+\t}\n+\tif got := len(env.store.notes); got != 0 {\n+\t\tt.Fatalf(\"erasure left %d notes, want 0\", got)\n+\t}\n+}\n+\n+func TestEraseUserNoNotes(t *testing.T) {\n+\tenv := newEraseTestEnv()\n+\tif err := EraseUser(context.Background(), env, \"user-1\"); err != nil {\n+\t\tt.Fatalf(\"EraseUser: %v\", err)\n+\t}\n+}\n", + "live": { + "prContext": { + "title": "notes: cover account erasure with tests", + "description": "Adds tests for EraseUser: full multi-page deletion for a user whose notes span more than one deletion page, and the no-notes case. Uses in-memory store and flag fakes.", + "author": "dev-notes", + "baseBranch": "main" + }, + "mustCatchSpecs": [ + { + "key": "erasure-suite-flag-mock", + "path": "services/notes/erasure_test.go", + "mechanism": [ + "(constructor|helper|newEraseTestEnv|every test|suite[- ]wide|whole suite).{0,40}(pins?|forces?|mocks?|hard[- ]?codes?).{0,20}flag", + "flag[- ]off (path|branch|behavior).{0,40}(never|not) (run|exercised|tested|executed|covered)", + "hid(es|ing|den).{0,20}flag.?off", + "silently skips?.{0,40}(erasure|deletion)" + ], + "lens": "test-adequacy", + "lineStart": 70, + "lineEnd": 80 + } + ] + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/erasure.go b/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/erasure.go new file mode 100644 index 00000000..6e53bf11 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/erasure.go @@ -0,0 +1,56 @@ +package notes + +import ( + "context" + "fmt" +) + +// erasePageSize is how many notes each deletion page fetches. +const erasePageSize = 500 + +// retentionFlag gates the notes-retention feature while it rolls out. +const retentionFlag = "notes-retention-enabled" + +// eraseEnv is the slice of the request environment the erasure path +// needs, kept local so this file names only what it uses. +type eraseEnv interface { + Store() Store + // Flags exposes feature-flag lookups for the rollout gate. + Flags() FlagSet +} + +// deleteAllForUser removes every stored note for userID, paging +// through the store until no rows remain. +func deleteAllForUser(ctx context.Context, store Store, userID string) error { + for { + notes, err := store.Run(ctx, Query{ + UserID: userID, + Limit: erasePageSize, + KeysOnly: true, + }) + if err != nil { + return fmt.Errorf("list notes for %s: %w", userID, err) + } + if len(notes) == 0 { + return nil + } + ids := make([]string, 0, len(notes)) + for _, note := range notes { + ids = append(ids, note.ID) + } + if err := store.Delete(ctx, ids); err != nil { + return fmt.Errorf("delete notes for %s: %w", userID, err) + } + } +} + +// EraseUser removes every note a user has stored. The account-erasure +// pipeline calls this after the user record is tombstoned; nothing the +// user wrote may survive it. +func EraseUser(ctx context.Context, env eraseEnv, userID string) error { + if !env.Flags().Enabled(ctx, retentionFlag) { + // Retention is still rolling out; skip until the flag is on. + return nil + } + return deleteAllForUser(ctx, env.Store(), userID) +} diff --git a/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/erasure_test.go b/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/erasure_test.go new file mode 100644 index 00000000..e9fb2132 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/erasure_test.go @@ -0,0 +1,105 @@ +package notes + +import ( + "context" + "fmt" + "testing" +) + +// eraseFlags is the FlagSet the erasure tests run under. +type eraseFlags struct { + forced map[string]bool +} + +func (f *eraseFlags) Enabled(_ context.Context, name string) bool { + return f.forced[name] +} + +// eraseStore is an in-memory Store for the erasure tests. +type eraseStore struct { + notes []Note +} + +func (s *eraseStore) Run(_ context.Context, q Query) ([]Note, error) { + limit := q.Limit + if limit == 0 { + limit = 1 + } + var out []Note + for _, note := range s.notes { + if note.UserID != q.UserID { + continue + } + if q.KeysOnly { + note = Note{ID: note.ID} + } + out = append(out, note) + if len(out) == limit { + break + } + } + return out, nil +} + +func (s *eraseStore) Delete(_ context.Context, ids []string) error { + drop := make(map[string]bool, len(ids)) + for _, id := range ids { + drop[id] = true + } + kept := s.notes[:0] + for _, note := range s.notes { + if !drop[note.ID] { + kept = append(kept, note) + } + } + s.notes = kept + return nil +} + +// eraseTestEnv bundles the fakes behind the eraseEnv interface. +type eraseTestEnv struct { + store *eraseStore + flags *eraseFlags +} + +func (e *eraseTestEnv) Store() Store { return e.store } + +func (e *eraseTestEnv) Flags() FlagSet { return e.flags } + +// newEraseTestEnv returns the env every erasure test runs under. +// Retention is rolling out everywhere; run the suite with the flag +// on, as production will be. +func newEraseTestEnv() *eraseTestEnv { + return &eraseTestEnv{ + store: &eraseStore{}, + flags: &eraseFlags{forced: map[string]bool{retentionFlag: true}}, + } +} + +func seedNotes(env *eraseTestEnv, n int) { + for i := 0; i < n; i++ { + env.store.notes = append(env.store.notes, Note{ + ID: fmt.Sprintf("note-%d", i), + UserID: "user-1", + }) + } +} + +func TestEraseUserRemovesAllNotes(t *testing.T) { + env := newEraseTestEnv() + // Enough notes to span two deletion pages. + seedNotes(env, erasePageSize+250) + if err := EraseUser(context.Background(), env, "user-1"); err != nil { + t.Fatalf("EraseUser: %v", err) + } + if got := len(env.store.notes); got != 0 { + t.Fatalf("erasure left %d notes, want 0", got) + } +} + +func TestEraseUserNoNotes(t *testing.T) { + env := newEraseTestEnv() + if err := EraseUser(context.Background(), env, "user-1"); err != nil { + t.Fatalf("EraseUser: %v", err) + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/store.go b/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/store.go new file mode 100644 index 00000000..36c7fbbb --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/store.go @@ -0,0 +1,51 @@ +// 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 + Body string + CreatedAt time.Time +} + +// Query selects notes for one user, newest first. +type Query struct { + UserID string + // Limit caps the number of rows returned. Zero means 1 (the + // store's default, tuned for the common latest-note lookup), + // NOT unlimited; callers that want more must set it. + 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) + // Delete removes the notes with the given IDs. + Delete(ctx context.Context, ids []string) error +} + +// FlagSet reports feature-flag state for a request. +type FlagSet interface { + // Enabled reports whether the named flag is on for this request. + Enabled(ctx context.Context, name string) bool +} + +// Env is the request environment the notes package reads. +type Env interface { + // Store returns the notes persistence surface for this request. + Store() Store + // Flags exposes feature-flag lookups. Added for the erasure + // path's rollout gate. + Flags() FlagSet +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-deletion/case.json b/workflows/review/eval/corpus/incidents/trial-retention-deletion/case.json new file mode 100644 index 00000000..a2973077 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-deletion/case.json @@ -0,0 +1,274 @@ +{ + "id": "trial-retention-deletion", + "tags": [ + "incident", + "trial", + "live" + ], + "category": "incident-repro", + "description": "Sanitized structural rewrite of a seeded-defect trial PR, deletion path: account erasure of stored notes is gated behind a rollout flag, fetches with the store's default limit of 1, reimplements an existing deletion helper per-note, widens the request-env interface in two files for the buggy gate alone, and swallows the tail-prune error.", + "changedFiles": [ + { + "path": "services/notes/erasure.go", + "status": "modified" + }, + { + "path": "services/notes/erasure_test.go", + "status": "added" + }, + { + "path": "services/notes/store.go", + "status": "modified" + } + ], + "dimensions": { + "correctness": "assessed", + "skillSeverity": "assessed", + "patternTriage": "assessed" + }, + "findings": [ + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "erasure-flag-gated-deletion", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "services/notes/erasure.go", + "line": 52, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.9, + "evidence_trace": [ + "services/notes/erasure.go:52 returns nil from EraseUser when notes-retention-enabled is off", + "retentionFlag is a rollout flag and defaults off until the launch completes", + "EraseUser is the account-erasure entry point; no other path deletes the user's notes" + ], + "failure_scenario": "With notes-retention-enabled off (the rollout default), EraseUser returns nil before deleting anything: the account-erasure pipeline records success while every stored note survives.", + "producing_hunt": "correctness:flag-gating", + "model_authored_prose": "The account-erasure deletion is gated behind the rollout flag: with notes-retention-enabled off, EraseUser silently skips and the user's notes survive erasure. Compliance deletion has to run regardless of rollout state; only the new retention behavior should be flag-gated." + } + }, + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "erasure-default-limit-one", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "services/notes/erasure.go", + "line": 56, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.88, + "evidence_trace": [ + "services/notes/erasure.go:56 runs Query{UserID: userID} with Limit unset", + "services/notes/store.go documents Query.Limit: zero means 1 (the store default), not unlimited", + "the erasure loop deletes only what this single query returned" + ], + "failure_scenario": "A user with 50 stored notes requests account erasure: the query returns exactly one note (the store's zero-value Limit default), so one note is deleted and 49 persist, run after run.", + "producing_hunt": "correctness:query-limits", + "model_authored_prose": "Query.Limit is unset here and the store's documented default is 1, so this erasure pass deletes at most one note per call. Page through with an explicit limit (or use the existing helper) so the whole set is removed." + } + }, + { + "source": "conventions", + "finding": { + "schema_version": 2, + "id": "erasure-ignores-delete-helper", + "lens": "conventions", + "anchor": { + "type": "line", + "path": "services/notes/erasure.go", + "line": 61, + "side": "RIGHT" + }, + "severity": "advisory", + "confidence": 0.7, + "evidence_trace": [ + "services/notes/erasure.go:61 deletes one note per Delete call inside a new loop", + "deleteAllForUser in the same file already pages through the store and batch-deletes ids", + "the new loop neither pages nor batches, so it does strictly less than the helper it shadows" + ], + "failure_scenario": "The two deletion paths drift: a fix to deleteAllForUser (paging, batching, retries) never reaches the erasure loop, and the loop issues one Delete call per note instead of one per page.", + "producing_hunt": "conventions:reuse-existing-helper", + "model_authored_prose": "deleteAllForUser a few lines up already does this deletion correctly: it pages through the store and deletes ids in batches. Reusing it instead of this per-note loop keeps one deletion path and drops a round-trip per note." + } + }, + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "erasure-env-widening", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "services/notes/erasure.go", + "line": 19, + "side": "RIGHT" + }, + "severity": "advisory", + "confidence": 0.65, + "evidence_trace": [ + "services/notes/erasure.go:19 widens eraseEnv with Flags()", + "the only caller of Flags() in this file is the rollout gate in EraseUser", + "the same widening is applied to Env in services/notes/store.go" + ], + "failure_scenario": "Every eraseEnv implementation (and every test fake) must now provide Flags() solely to serve the flag gate on the compliance deletion; when that gate is removed, the widened interface remains with no caller.", + "producing_hunt": "correctness:interface-scope", + "model_authored_prose": "This widens eraseEnv just for the flag gate in EraseUser, which itself should not exist for a compliance deletion. If the gate goes away, the widening (here and on Env in store.go) can go with it." + } + }, + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "store-env-widening", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "services/notes/store.go", + "line": 50, + "side": "RIGHT" + }, + "severity": "advisory", + "confidence": 0.65, + "evidence_trace": [ + "services/notes/store.go:50 widens Env with Flags()", + "no store-side code reads Flags(); the only consumer is the erasure rollout gate", + "the parallel widening of eraseEnv in services/notes/erasure.go serves the same single call" + ], + "failure_scenario": "Every Env implementation across the service now carries a Flags() method that only the erasure path's flag gate uses; the package-level contract grows for one questionable call site.", + "producing_hunt": "correctness:interface-scope", + "model_authored_prose": "Env gains Flags() here although nothing in this file uses it; the sole consumer is the erasure gate. Keeping the request-env contract minimal argues for scoping the flag lookup to the one caller, or dropping it together with the gate." + } + }, + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "erasure-prune-error-swallowed", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "services/notes/erasure.go", + "line": 66, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.86, + "evidence_trace": [ + "services/notes/erasure.go:66 assigns the PruneUserNotes error to the blank identifier", + "PruneUserNotes returns an error precisely when its store calls fail", + "EraseUser then returns nil, so the caller cannot distinguish a clean erasure from a failed tail prune" + ], + "failure_scenario": "PruneUserNotes fails mid-delete (a store timeout) and EraseUser still returns nil: the erasure pipeline marks the account done while notes written during the erasure remain stored.", + "producing_hunt": "correctness:error-handling", + "model_authored_prose": "Per the error-handling skill, a returned error is handled or propagated, never discarded: `_ = PruneUserNotes(...)` swallows a real failure and EraseUser reports success anyway. Propagate it, or at minimum log it and surface a retry." + } + } + ], + "expected": { + "verdict": "REQUEST_CHANGES", + "mustCatch": [ + "erasure-flag-gated-deletion", + "erasure-default-limit-one", + "erasure-ignores-delete-helper", + "erasure-env-widening", + "store-env-widening", + "erasure-prune-error-swallowed" + ], + "postedCommentCount": 6 + }, + "diff": "diff --git a/services/notes/erasure.go b/services/notes/erasure.go\n--- a/services/notes/erasure.go\n+++ b/services/notes/erasure.go\n@@ -8,10 +8,15 @@\n // erasePageSize is how many notes each deletion page fetches.\n const erasePageSize = 500\n \n+// retentionFlag gates the notes-retention feature while it rolls out.\n+const retentionFlag = \"notes-retention-enabled\"\n+\n // eraseEnv is the slice of the request environment the erasure path\n // needs, kept local so this file names only what it uses.\n type eraseEnv interface {\n \tStore() Store\n+\t// Flags exposes feature-flag lookups for the rollout gate.\n+\tFlags() FlagSet\n }\n \n // deleteAllForUser removes every stored note for userID, paging\n@@ -39,3 +44,25 @@\n \t\t}\n \t}\n }\n+\n+// EraseUser removes every note a user has stored. The account-erasure\n+// pipeline calls this after the user record is tombstoned; nothing the\n+// user wrote may survive it.\n+func EraseUser(ctx context.Context, env eraseEnv, userID string) error {\n+\tif !env.Flags().Enabled(ctx, retentionFlag) {\n+\t\t// Retention is still rolling out; skip until the flag is on.\n+\t\treturn nil\n+\t}\n+\tnotes, err := env.Store().Run(ctx, Query{UserID: userID})\n+\tif err != nil {\n+\t\treturn fmt.Errorf(\"list notes for erasure of %s: %w\", userID, err)\n+\t}\n+\tfor _, note := range notes {\n+\t\tif err := env.Store().Delete(ctx, []string{note.ID}); err != nil {\n+\t\t\treturn fmt.Errorf(\"erase note %s: %w\", note.ID, err)\n+\t\t}\n+\t}\n+\t// Best-effort tail prune in case new notes landed mid-erasure.\n+\t_ = PruneUserNotes(ctx, env, userID)\n+\treturn nil\n+}\ndiff --git a/services/notes/erasure_test.go b/services/notes/erasure_test.go\nnew file mode 100644\n--- /dev/null\n+++ b/services/notes/erasure_test.go\n@@ -0,0 +1,90 @@\n+package notes\n+\n+import (\n+\t\"context\"\n+\t\"fmt\"\n+\t\"testing\"\n+)\n+\n+// fakeStore is an in-memory Store for the erasure tests.\n+type fakeStore struct {\n+\tnotes []Note\n+}\n+\n+func (s *fakeStore) Run(_ context.Context, q Query) ([]Note, error) {\n+\tlimit := q.Limit\n+\tif limit == 0 {\n+\t\tlimit = 1\n+\t}\n+\tvar out []Note\n+\tfor _, note := range s.notes {\n+\t\tif note.UserID != q.UserID {\n+\t\t\tcontinue\n+\t\t}\n+\t\tout = append(out, note)\n+\t\tif len(out) == limit {\n+\t\t\tbreak\n+\t\t}\n+\t}\n+\treturn out, nil\n+}\n+\n+func (s *fakeStore) Delete(_ context.Context, ids []string) error {\n+\tdrop := make(map[string]bool, len(ids))\n+\tfor _, id := range ids {\n+\t\tdrop[id] = true\n+\t}\n+\tkept := s.notes[:0]\n+\tfor _, note := range s.notes {\n+\t\tif !drop[note.ID] {\n+\t\t\tkept = append(kept, note)\n+\t\t}\n+\t}\n+\ts.notes = kept\n+\treturn nil\n+}\n+\n+// fakeFlags is a FlagSet with every flag forced on.\n+type fakeFlags struct{}\n+\n+func (fakeFlags) Enabled(context.Context, string) bool { return true }\n+\n+// fakeEnv bundles the fakes behind the eraseEnv interface.\n+type fakeEnv struct {\n+\tstore *fakeStore\n+}\n+\n+func (e *fakeEnv) Store() Store { return e.store }\n+\n+func (e *fakeEnv) Flags() FlagSet { return fakeFlags{} }\n+\n+func seedNotes(n int) *fakeStore {\n+\ts := &fakeStore{}\n+\tfor i := 0; i < n; i++ {\n+\t\ts.notes = append(s.notes, Note{\n+\t\t\tID: fmt.Sprintf(\"note-%d\", i),\n+\t\t\tUserID: \"user-1\",\n+\t\t})\n+\t}\n+\treturn s\n+}\n+\n+func TestEraseUserRemovesStoredNote(t *testing.T) {\n+\tenv := &fakeEnv{store: seedNotes(1)}\n+\tif err := EraseUser(context.Background(), env, \"user-1\"); err != nil {\n+\t\tt.Fatalf(\"EraseUser: %v\", err)\n+\t}\n+\tif got := len(env.store.notes); got != 0 {\n+\t\tt.Fatalf(\"EraseUser left %d notes, want 0\", got)\n+\t}\n+}\n+\n+func TestDeleteAllForUserPagesToEmpty(t *testing.T) {\n+\tstore := seedNotes(7)\n+\tif err := deleteAllForUser(context.Background(), store, \"user-1\"); err != nil {\n+\t\tt.Fatalf(\"deleteAllForUser: %v\", err)\n+\t}\n+\tif got := len(store.notes); got != 0 {\n+\t\tt.Fatalf(\"deleteAllForUser left %d notes, want 0\", got)\n+\t}\n+}\ndiff --git a/services/notes/store.go b/services/notes/store.go\n--- a/services/notes/store.go\n+++ b/services/notes/store.go\n@@ -35,8 +35,17 @@\n \tDelete(ctx context.Context, ids []string) error\n }\n \n+// FlagSet reports feature-flag state for a request.\n+type FlagSet interface {\n+\t// Enabled reports whether the named flag is on for this request.\n+\tEnabled(ctx context.Context, name string) bool\n+}\n+\n // Env is the request environment the notes package reads.\n type Env interface {\n \t// Store returns the notes persistence surface for this request.\n \tStore() Store\n+\t// Flags exposes feature-flag lookups. Added for the erasure\n+\t// path's rollout gate.\n+\tFlags() FlagSet\n }\n", + "live": { + "prContext": { + "title": "notes: delete stored notes on account erasure", + "description": "Wires the notes service into the account-erasure pipeline: EraseUser removes a user's stored notes after the user record is tombstoned, behind the notes-retention-enabled rollout flag. Adds a best-effort tail prune so notes written mid-erasure are cleaned up.", + "author": "dev-notes", + "baseBranch": "main" + }, + "mustCatchSpecs": [ + { + "key": "erasure-flag-gated-deletion", + "path": "services/notes/erasure.go", + "mechanism": [ + "feature.?flag.*(gate|gating|gated|skips?).*(erasure|deletion|delete)", + "flag.*(off|disabled).*(survive|remain|skip)", + "compliance deletion.*(flag|rollout)", + "EraseUser returns nil.*flag" + ], + "lens": "correctness", + "lineStart": 47, + "lineEnd": 57 + }, + { + "key": "erasure-default-limit-one", + "path": "services/notes/erasure.go", + "mechanism": [ + "limit.*(defaults? to|zero (value|means)).*1", + "at most one (note|row|item).*(deleted|removed|returned)", + "Limit (unset|not set|omitted|missing)", + "deletes? only (one|a single) (note|row|item)" + ], + "lens": "correctness", + "lineStart": 51, + "lineEnd": 61 + }, + { + "key": "erasure-ignores-delete-helper", + "path": "services/notes/erasure.go", + "mechanism": [ + "deleteAllForUser", + "existing (deletion )?helper", + "re-?implement", + "duplicat(es|ed|ing).*(deletion|helper)" + ], + "lens": "conventions", + "lineStart": 56, + "lineEnd": 66 + }, + { + "key": "erasure-env-widening", + "path": "services/notes/erasure.go", + "mechanism": [ + "widen(s|ed|ing)?.*(eraseEnv|interface)", + "Flags\\(\\).*(only|single|one) (call|caller|consumer)", + "interface.*(grows|widened).*flag" + ], + "lens": "correctness", + "lineStart": 14, + "lineEnd": 24 + }, + { + "key": "store-env-widening", + "path": "services/notes/store.go", + "mechanism": [ + "widen(s|ed|ing)?.*(Env|interface)", + "Flags\\(\\).*(unused|nothing in (this|the) file|no (store|other) (code|caller))", + "Env.*(gains|widened).*Flags" + ], + "lens": "correctness", + "lineStart": 45, + "lineEnd": 55 + }, + { + "key": "erasure-prune-error-swallowed", + "path": "services/notes/erasure.go", + "mechanism": [ + "_ = PruneUserNotes", + "swallow(s|ed|ing).*error", + "(ignores?|discard(s|ed)?).*(returned )?error", + "blank identifier.*error" + ], + "lens": "correctness", + "lineStart": 61, + "lineEnd": 71 + } + ] + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/erasure.go b/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/erasure.go new file mode 100644 index 00000000..1d99de2a --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/erasure.go @@ -0,0 +1,68 @@ +package notes + +import ( + "context" + "fmt" +) + +// erasePageSize is how many notes each deletion page fetches. +const erasePageSize = 500 + +// retentionFlag gates the notes-retention feature while it rolls out. +const retentionFlag = "notes-retention-enabled" + +// eraseEnv is the slice of the request environment the erasure path +// needs, kept local so this file names only what it uses. +type eraseEnv interface { + Store() Store + // Flags exposes feature-flag lookups for the rollout gate. + Flags() FlagSet +} + +// deleteAllForUser removes every stored note for userID, paging +// through the store until no rows remain. It is the deletion helper +// erasure-style callers in this package are expected to use. +func deleteAllForUser(ctx context.Context, store Store, userID string) error { + for { + notes, err := store.Run(ctx, Query{ + UserID: userID, + Limit: erasePageSize, + KeysOnly: true, + }) + if err != nil { + return fmt.Errorf("list notes for %s: %w", userID, err) + } + if len(notes) == 0 { + return nil + } + ids := make([]string, 0, len(notes)) + for _, note := range notes { + ids = append(ids, note.ID) + } + if err := store.Delete(ctx, ids); err != nil { + return fmt.Errorf("delete notes for %s: %w", userID, err) + } + } +} + +// EraseUser removes every note a user has stored. The account-erasure +// pipeline calls this after the user record is tombstoned; nothing the +// user wrote may survive it. +func EraseUser(ctx context.Context, env eraseEnv, userID string) error { + if !env.Flags().Enabled(ctx, retentionFlag) { + // Retention is still rolling out; skip until the flag is on. + return nil + } + notes, err := env.Store().Run(ctx, Query{UserID: userID}) + if err != nil { + return fmt.Errorf("list notes for erasure of %s: %w", userID, err) + } + for _, note := range notes { + if err := env.Store().Delete(ctx, []string{note.ID}); err != nil { + return fmt.Errorf("erase note %s: %w", note.ID, err) + } + } + // Best-effort tail prune in case new notes landed mid-erasure. + _ = PruneUserNotes(ctx, env, userID) + return nil +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/erasure_test.go b/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/erasure_test.go new file mode 100644 index 00000000..59be84ab --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/erasure_test.go @@ -0,0 +1,90 @@ +package notes + +import ( + "context" + "fmt" + "testing" +) + +// fakeStore is an in-memory Store for the erasure tests. +type fakeStore struct { + notes []Note +} + +func (s *fakeStore) Run(_ context.Context, q Query) ([]Note, error) { + limit := q.Limit + if limit == 0 { + limit = 1 + } + var out []Note + for _, note := range s.notes { + if note.UserID != q.UserID { + continue + } + out = append(out, note) + if len(out) == limit { + break + } + } + return out, nil +} + +func (s *fakeStore) Delete(_ context.Context, ids []string) error { + drop := make(map[string]bool, len(ids)) + for _, id := range ids { + drop[id] = true + } + kept := s.notes[:0] + for _, note := range s.notes { + if !drop[note.ID] { + kept = append(kept, note) + } + } + s.notes = kept + return nil +} + +// fakeFlags is a FlagSet with every flag forced on. +type fakeFlags struct{} + +func (fakeFlags) Enabled(context.Context, string) bool { return true } + +// fakeEnv bundles the fakes behind the eraseEnv interface. +type fakeEnv struct { + store *fakeStore +} + +func (e *fakeEnv) Store() Store { return e.store } + +func (e *fakeEnv) Flags() FlagSet { return fakeFlags{} } + +func seedNotes(n int) *fakeStore { + s := &fakeStore{} + for i := 0; i < n; i++ { + s.notes = append(s.notes, Note{ + ID: fmt.Sprintf("note-%d", i), + UserID: "user-1", + }) + } + return s +} + +func TestEraseUserRemovesStoredNote(t *testing.T) { + env := &fakeEnv{store: seedNotes(1)} + if err := EraseUser(context.Background(), env, "user-1"); err != nil { + t.Fatalf("EraseUser: %v", err) + } + if got := len(env.store.notes); got != 0 { + t.Fatalf("EraseUser left %d notes, want 0", got) + } +} + +func TestDeleteAllForUserPagesToEmpty(t *testing.T) { + store := seedNotes(7) + if err := deleteAllForUser(context.Background(), store, "user-1"); err != nil { + t.Fatalf("deleteAllForUser: %v", err) + } + if got := len(store.notes); got != 0 { + t.Fatalf("deleteAllForUser left %d notes, want 0", got) + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/prune.go b/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/prune.go new file mode 100644 index 00000000..2d0abb50 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/prune.go @@ -0,0 +1,34 @@ +package notes + +import ( + "context" + "fmt" +) + +// maxRetainedNotes is the retention cap: pruning keeps the newest 200 +// notes per user and deletes everything older. +const maxRetainedNotes = 200 + +// PruneUserNotes enforces the retention cap for one user. The +// background retention job calls it for every active user. +func PruneUserNotes(ctx context.Context, env eraseEnv, userID string) error { + if !env.Flags().Enabled(ctx, retentionFlag) { + return nil + } + notes, err := env.Store().Run(ctx, Query{ + UserID: userID, + Limit: erasePageSize, + KeysOnly: true, + }) + if err != nil { + return fmt.Errorf("list notes to prune for %s: %w", userID, err) + } + if len(notes) <= maxRetainedNotes { + return nil + } + ids := make([]string, 0, len(notes)-maxRetainedNotes) + for _, note := range notes[maxRetainedNotes:] { + ids = append(ids, note.ID) + } + return env.Store().Delete(ctx, ids) +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/store.go b/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/store.go new file mode 100644 index 00000000..36c7fbbb --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/store.go @@ -0,0 +1,51 @@ +// 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 + Body string + CreatedAt time.Time +} + +// Query selects notes for one user, newest first. +type Query struct { + UserID string + // Limit caps the number of rows returned. Zero means 1 (the + // store's default, tuned for the common latest-note lookup), + // NOT unlimited; callers that want more must set it. + 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) + // Delete removes the notes with the given IDs. + Delete(ctx context.Context, ids []string) error +} + +// FlagSet reports feature-flag state for a request. +type FlagSet interface { + // Enabled reports whether the named flag is on for this request. + Enabled(ctx context.Context, name string) bool +} + +// Env is the request environment the notes package reads. +type Env interface { + // Store returns the notes persistence surface for this request. + Store() Store + // Flags exposes feature-flag lookups. Added for the erasure + // path's rollout gate. + Flags() FlagSet +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/case.json b/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/case.json new file mode 100644 index 00000000..53c76c30 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/case.json @@ -0,0 +1,206 @@ +{ + "id": "trial-retention-prune-tests", + "tags": [ + "incident", + "trial", + "live" + ], + "category": "incident-repro", + "description": "Sanitized structural rewrite of a seeded-defect trial PR, prune-and-tests path: the retention prune retains 199 notes where the documented cap is 200, its test seeds below the cap so a no-op prune also passes, TestMain forces the rollout flag on for the whole suite so the flag-off path is never exercised, and the prune fetches full entities where a keys-only query suffices.", + "changedFiles": [ + { + "path": "services/notes/prune.go", + "status": "added" + }, + { + "path": "services/notes/prune_test.go", + "status": "added" + } + ], + "dimensions": { + "correctness": "assessed", + "skillSeverity": "assessed", + "patternTriage": "assessed" + }, + "findings": [ + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "prune-off-by-one-cap", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "services/notes/prune.go", + "line": 44, + "side": "RIGHT" + }, + "severity": "advisory", + "confidence": 0.75, + "evidence_trace": [ + "services/notes/prune.go:44 slices the stale set from index maxRetainedNotes-1", + "notes are newest first, so index 199 is the 200th-newest note and it lands in the stale slice", + "the documented cap says pruning keeps the newest 200 notes" + ], + "failure_scenario": "A user with 300 notes is pruned: the slice starts at index 199, deleting 101 notes and retaining 199 where the documented cap is 200; the 200th-newest note is wrongly deleted on every pass.", + "producing_hunt": "correctness:boundary-conditions", + "model_authored_prose": "Off by one: `notes[maxRetainedNotes-1:]` puts the 200th-newest note into the stale slice, so the cap actually retains 199. `notes[maxRetainedNotes:]` matches the documented keep-the-newest-200 behavior." + } + }, + { + "source": "test-adequacy", + "finding": { + "schema_version": 2, + "id": "prune-test-vacuous-cap", + "lens": "test-adequacy", + "anchor": { + "type": "line", + "path": "services/notes/prune_test.go", + "line": 87, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.85, + "evidence_trace": [ + "services/notes/prune_test.go:87 seeds 5 notes, far below maxRetainedNotes (200)", + "PruneUserNotes returns early when len(notes) <= maxRetainedNotes, so the deletion path never runs", + "the assertion kept == 5 also holds for a PruneUserNotes body of `return nil`" + ], + "failure_scenario": "Replace PruneUserNotes's body with `return nil` and TestPruneKeepsRecentNotes still passes: with 5 seeded notes the early return fires before any retention logic, so the cap (including its boundary behavior) is never exercised by the suite.", + "producing_hunt": "test-adequacy:under-exercised-path", + "model_authored_prose": "This test never crosses the retention cap: 5 seeded notes hit the early return, so it passes even if PruneUserNotes is a no-op. Seed more than maxRetainedNotes and assert both the retained count and which notes survive." + } + }, + { + "source": "test-adequacy", + "finding": { + "schema_version": 2, + "id": "prune-suite-flag-mock", + "lens": "test-adequacy", + "anchor": { + "type": "line", + "path": "services/notes/prune_test.go", + "line": 24, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.8, + "evidence_trace": [ + "services/notes/prune_test.go:24 forces notes-retention-enabled on for the whole suite in TestMain", + "the flag-off early return in PruneUserNotes is therefore never executed by any test", + "flag off is the production default while the rollout is in progress" + ], + "failure_scenario": "Every test runs with the flag forced on, so no test observes that flag-off makes retention (and any deletion behavior gated on the same flag) silently skip; the production-default path ships unexercised.", + "producing_hunt": "test-adequacy:flag-coverage", + "model_authored_prose": "TestMain pins notes-retention-enabled on for the entire suite, hiding the flag-off path, which is the production default during rollout. At least one test should run with the flag off and assert what is (and is not) supposed to happen." + } + }, + { + "source": "caching-resource", + "finding": { + "schema_version": 2, + "id": "prune-full-entity-fetch", + "lens": "caching-resource", + "anchor": { + "type": "line", + "path": "services/notes/prune.go", + "line": 33, + "side": "RIGHT" + }, + "severity": "advisory", + "confidence": 0.7, + "evidence_trace": [ + "services/notes/prune.go:33 fetches up to pruneFetchLimit full notes", + "the function only reads note.ID from the results", + "Query.KeysOnly exists and skips entity bodies for exactly this shape of call" + ], + "failure_scenario": "Each prune pass for an active user loads up to 5000 full note bodies to collect ids for deletion, paying entity deserialization and memory for data it never reads.", + "producing_hunt": "caching-resource:keys-only-query", + "model_authored_prose": "Per the datastore-efficiency skill, use a keys-only query when only identifiers are needed: this pass reads nothing but note.ID, so set KeysOnly and skip fetching up to 5000 note bodies." + } + } + ], + "expected": { + "verdict": "REQUEST_CHANGES", + "mustCatch": [ + "prune-off-by-one-cap", + "prune-test-vacuous-cap", + "prune-suite-flag-mock", + "prune-full-entity-fetch" + ], + "postedCommentCount": 4 + }, + "diff": "diff --git a/services/notes/prune.go b/services/notes/prune.go\nnew file mode 100644\n--- /dev/null\n+++ b/services/notes/prune.go\n@@ -0,0 +1,53 @@\n+package notes\n+\n+import (\n+\t\"context\"\n+\t\"fmt\"\n+)\n+\n+// maxRetainedNotes is the documented retention cap: pruning keeps the\n+// newest 200 notes per user and deletes everything older.\n+const maxRetainedNotes = 200\n+\n+// pruneFetchLimit bounds one prune pass, well above any real user.\n+const pruneFetchLimit = 5000\n+\n+// retentionFlag gates the notes-retention feature while it rolls out.\n+const retentionFlag = \"notes-retention-enabled\"\n+\n+// pruneEnv is the slice of the request environment pruning needs,\n+// kept local so this file names only what it uses.\n+type pruneEnv interface {\n+\tStore() Store\n+\tFlags() FlagSet\n+}\n+\n+// PruneUserNotes enforces the retention cap for one user: it keeps\n+// the newest maxRetainedNotes notes and deletes the rest. The\n+// background retention job calls it for every active user.\n+func PruneUserNotes(ctx context.Context, env pruneEnv, userID string) error {\n+\tif !env.Flags().Enabled(ctx, retentionFlag) {\n+\t\t// Retention is still rolling out; skip until the flag is on.\n+\t\treturn nil\n+\t}\n+\tnotes, err := env.Store().Run(ctx, Query{\n+\t\tUserID: userID,\n+\t\tLimit: pruneFetchLimit,\n+\t})\n+\tif err != nil {\n+\t\treturn fmt.Errorf(\"list notes to prune for %s: %w\", userID, err)\n+\t}\n+\tif len(notes) <= maxRetainedNotes {\n+\t\treturn nil\n+\t}\n+\t// Run returns notes newest first; everything past the cap is stale.\n+\tstale := notes[maxRetainedNotes-1:]\n+\tids := make([]string, 0, len(stale))\n+\tfor _, note := range stale {\n+\t\tids = append(ids, note.ID)\n+\t}\n+\tif err := env.Store().Delete(ctx, ids); err != nil {\n+\t\treturn fmt.Errorf(\"prune notes for %s: %w\", userID, err)\n+\t}\n+\treturn nil\n+}\ndiff --git a/services/notes/prune_test.go b/services/notes/prune_test.go\nnew file mode 100644\n--- /dev/null\n+++ b/services/notes/prune_test.go\n@@ -0,0 +1,94 @@\n+package notes\n+\n+import (\n+\t\"context\"\n+\t\"fmt\"\n+\t\"os\"\n+\t\"testing\"\n+)\n+\n+// suiteFlags is the FlagSet every prune test runs under.\n+type suiteFlags struct {\n+\tforced map[string]bool\n+}\n+\n+func (f *suiteFlags) Enabled(_ context.Context, name string) bool {\n+\treturn f.forced[name]\n+}\n+\n+var testFlags = &suiteFlags{forced: map[string]bool{}}\n+\n+func TestMain(m *testing.M) {\n+\t// Retention is rolling out everywhere; run the whole suite with\n+\t// the flag on, as production will be.\n+\ttestFlags.forced[retentionFlag] = true\n+\tos.Exit(m.Run())\n+}\n+\n+// pruneStore is an in-memory Store for the prune tests.\n+type pruneStore struct {\n+\tnotes []Note\n+}\n+\n+func (s *pruneStore) Run(_ context.Context, q Query) ([]Note, error) {\n+\tlimit := q.Limit\n+\tif limit == 0 {\n+\t\tlimit = 1\n+\t}\n+\tvar out []Note\n+\tfor _, note := range s.notes {\n+\t\tif note.UserID != q.UserID {\n+\t\t\tcontinue\n+\t\t}\n+\t\tout = append(out, note)\n+\t\tif len(out) == limit {\n+\t\t\tbreak\n+\t\t}\n+\t}\n+\treturn out, nil\n+}\n+\n+func (s *pruneStore) Delete(_ context.Context, ids []string) error {\n+\tdrop := make(map[string]bool, len(ids))\n+\tfor _, id := range ids {\n+\t\tdrop[id] = true\n+\t}\n+\tkept := s.notes[:0]\n+\tfor _, note := range s.notes {\n+\t\tif !drop[note.ID] {\n+\t\t\tkept = append(kept, note)\n+\t\t}\n+\t}\n+\ts.notes = kept\n+\treturn nil\n+}\n+\n+// pruneTestEnv bundles the fakes behind the pruneEnv interface.\n+type pruneTestEnv struct {\n+\tstore *pruneStore\n+}\n+\n+func (e *pruneTestEnv) Store() Store { return e.store }\n+\n+func (e *pruneTestEnv) Flags() FlagSet { return testFlags }\n+\n+func seedStore(n int) *pruneStore {\n+\ts := &pruneStore{}\n+\tfor i := 0; i < n; i++ {\n+\t\ts.notes = append(s.notes, Note{\n+\t\t\tID: fmt.Sprintf(\"note-%d\", i),\n+\t\t\tUserID: \"user-1\",\n+\t\t})\n+\t}\n+\treturn s\n+}\n+\n+func TestPruneKeepsRecentNotes(t *testing.T) {\n+\tenv := &pruneTestEnv{store: seedStore(5)}\n+\tif err := PruneUserNotes(context.Background(), env, \"user-1\"); err != nil {\n+\t\tt.Fatalf(\"PruneUserNotes: %v\", err)\n+\t}\n+\tif got := len(env.store.notes); got != 5 {\n+\t\tt.Fatalf(\"prune touched recent notes: kept %d, want 5\", got)\n+\t}\n+}\n", + "live": { + "prContext": { + "title": "notes: prune stored notes to the retention cap", + "description": "Adds the background prune for the notes retention policy: PruneUserNotes keeps the newest 200 notes per user and deletes the rest, behind the notes-retention-enabled rollout flag. Includes tests against an in-memory store.", + "author": "dev-notes", + "baseBranch": "main" + }, + "mustCatchSpecs": [ + { + "key": "prune-off-by-one-cap", + "path": "services/notes/prune.go", + "mechanism": [ + "off.?by.?one", + "maxRetainedNotes-1", + "retains? (only )?199", + "200th[- ]newest note.*deleted" + ], + "lens": "correctness", + "lineStart": 39, + "lineEnd": 49 + }, + { + "key": "prune-test-vacuous-cap", + "path": "services/notes/prune_test.go", + "mechanism": [ + "(vacuous|never (exercises?|crosses|reaches)).*(cap|retention|deletion)", + "passes even (if|when).*(no-?op|return nil)", + "early return.*(before|skips).*(deletion|retention)", + "seeds? (only )?5 notes.*below the cap" + ], + "lens": "test-adequacy", + "lineStart": 82, + "lineEnd": 92 + }, + { + "key": "prune-suite-flag-mock", + "path": "services/notes/prune_test.go", + "mechanism": [ + "TestMain.*(forces?|pins?|mocks?|overrides?).*flag", + "flag[- ]off (path|behavior|branch).*(never|not) (run|exercised|tested|executed)", + "suite[- ]wide.*flag.*on", + "hid(es|ing|den).*flag.?off" + ], + "lens": "test-adequacy", + "lineStart": 19, + "lineEnd": 29 + }, + { + "key": "prune-full-entity-fetch", + "path": "services/notes/prune.go", + "mechanism": [ + "keys.?only", + "full (note |entity )?(bodies|entities|fetch)", + "only (reads?|needs?) (note\\.)?ids?", + "KeysOnly" + ], + "lens": "caching-resource", + "lineStart": 28, + "lineEnd": 38 + } + ] + }, + "routerConfig": { + "lensRules": [ + { + "pattern": "services/notes/prune.go", + "lenses": [ + "caching-resource" + ] + } + ] + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/prune.go b/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/prune.go new file mode 100644 index 00000000..7f9b4f4e --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/prune.go @@ -0,0 +1,53 @@ +package notes + +import ( + "context" + "fmt" +) + +// maxRetainedNotes is the documented retention cap: pruning keeps the +// newest 200 notes per user and deletes everything older. +const maxRetainedNotes = 200 + +// pruneFetchLimit bounds one prune pass, well above any real user. +const pruneFetchLimit = 5000 + +// retentionFlag gates the notes-retention feature while it rolls out. +const retentionFlag = "notes-retention-enabled" + +// pruneEnv is the slice of the request environment pruning needs, +// kept local so this file names only what it uses. +type pruneEnv interface { + Store() Store + Flags() FlagSet +} + +// PruneUserNotes enforces the retention cap for one user: it keeps +// the newest maxRetainedNotes notes and deletes the rest. The +// background retention job calls it for every active user. +func PruneUserNotes(ctx context.Context, env pruneEnv, userID string) error { + if !env.Flags().Enabled(ctx, retentionFlag) { + // Retention is still rolling out; skip until the flag is on. + return nil + } + notes, err := env.Store().Run(ctx, Query{ + UserID: userID, + Limit: pruneFetchLimit, + }) + if err != nil { + return fmt.Errorf("list notes to prune for %s: %w", userID, err) + } + if len(notes) <= maxRetainedNotes { + return nil + } + // Run returns notes newest first; everything past the cap is stale. + stale := notes[maxRetainedNotes-1:] + ids := make([]string, 0, len(stale)) + for _, note := range stale { + ids = append(ids, note.ID) + } + if err := env.Store().Delete(ctx, ids); err != nil { + return fmt.Errorf("prune notes for %s: %w", userID, err) + } + return nil +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/prune_test.go b/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/prune_test.go new file mode 100644 index 00000000..8feb6d95 --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/prune_test.go @@ -0,0 +1,94 @@ +package notes + +import ( + "context" + "fmt" + "os" + "testing" +) + +// suiteFlags is the FlagSet every prune test runs under. +type suiteFlags struct { + forced map[string]bool +} + +func (f *suiteFlags) Enabled(_ context.Context, name string) bool { + return f.forced[name] +} + +var testFlags = &suiteFlags{forced: map[string]bool{}} + +func TestMain(m *testing.M) { + // Retention is rolling out everywhere; run the whole suite with + // the flag on, as production will be. + testFlags.forced[retentionFlag] = true + os.Exit(m.Run()) +} + +// pruneStore is an in-memory Store for the prune tests. +type pruneStore struct { + notes []Note +} + +func (s *pruneStore) Run(_ context.Context, q Query) ([]Note, error) { + limit := q.Limit + if limit == 0 { + limit = 1 + } + var out []Note + for _, note := range s.notes { + if note.UserID != q.UserID { + continue + } + out = append(out, note) + if len(out) == limit { + break + } + } + return out, nil +} + +func (s *pruneStore) Delete(_ context.Context, ids []string) error { + drop := make(map[string]bool, len(ids)) + for _, id := range ids { + drop[id] = true + } + kept := s.notes[:0] + for _, note := range s.notes { + if !drop[note.ID] { + kept = append(kept, note) + } + } + s.notes = kept + return nil +} + +// pruneTestEnv bundles the fakes behind the pruneEnv interface. +type pruneTestEnv struct { + store *pruneStore +} + +func (e *pruneTestEnv) Store() Store { return e.store } + +func (e *pruneTestEnv) Flags() FlagSet { return testFlags } + +func seedStore(n int) *pruneStore { + s := &pruneStore{} + for i := 0; i < n; i++ { + s.notes = append(s.notes, Note{ + ID: fmt.Sprintf("note-%d", i), + UserID: "user-1", + }) + } + return s +} + +func TestPruneKeepsRecentNotes(t *testing.T) { + env := &pruneTestEnv{store: seedStore(5)} + if err := PruneUserNotes(context.Background(), env, "user-1"); err != nil { + t.Fatalf("PruneUserNotes: %v", err) + } + if got := len(env.store.notes); got != 5 { + t.Fatalf("prune touched recent notes: kept %d, want 5", got) + } +} diff --git a/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/store.go b/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/store.go new file mode 100644 index 00000000..36c7fbbb --- /dev/null +++ b/workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/store.go @@ -0,0 +1,51 @@ +// 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 + Body string + CreatedAt time.Time +} + +// Query selects notes for one user, newest first. +type Query struct { + UserID string + // Limit caps the number of rows returned. Zero means 1 (the + // store's default, tuned for the common latest-note lookup), + // NOT unlimited; callers that want more must set it. + 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) + // Delete removes the notes with the given IDs. + Delete(ctx context.Context, ids []string) error +} + +// FlagSet reports feature-flag state for a request. +type FlagSet interface { + // Enabled reports whether the named flag is on for this request. + Enabled(ctx context.Context, name string) bool +} + +// Env is the request environment the notes package reads. +type Env interface { + // Store returns the notes persistence surface for this request. + Store() Store + // Flags exposes feature-flag lookups. Added for the erasure + // path's rollout gate. + Flags() FlagSet +}