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

.changeset/review-live-corpus-format.md

@@ -0,0 +1,5 @@
+---
+"review": minor
+---
+
+Live-enabled corpus cases (live A/B eval plan, phase 1). The eval corpus gains an opt-in `live` block carrying what a REAL model run needs and the deterministic replay does not: the PR context (`prContext`, with the description treated as untrusted author text), a post-change file tree on disk next to the case (`<id>/case.json` + `<id>/tree/`, coexisting with the flat `<id>.json` layout), and labeled defect specs (`mustCatchSpecs` / `mustNotFlagSpecs`: path, line window, mechanism keyword alternates) so a live run's model-chosen finding ids can be matched to ground truth. A live case must carry the `live` tag, a cleanly-parseable diff, and every non-removed changed file in its tree; the loader enforces all of it and `loadLiveCorpus()` returns the subset. Ten cases are converted with hand-authored real diffs and trees (five smoke incidents, both clean cases, one adversarial injection whose payload lives in the diff, one golden holdout, one synthetic mutation); their recorded line anchors now point at the authored defect lines, and the deterministic suite runs them unchanged, provenance gate included.

.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.

.eslintrc.js

@@ -26,7 +26,14 @@ module.exports = {
         sourceType: "module",
         ecmaVersion: 2022,
     },
-    ignorePatterns: ["**/dist/", "actions/**/index.js"],
+    ignorePatterns: [
+        "**/dist/",
+        "actions/**/index.js",
+        // Live eval-corpus trees are byte-exact fixtures paired with each
+        // case's diff: linting (and especially prettier auto-formatting)
+        // would desync them from the diff the provenance gate parses.
+        "workflows/review/eval/corpus/**/tree/",
+    ],
     overrides: [
         {
             files: "**/*.ts",

vitest.config.ts

@@ -1,9 +1,16 @@
-import {defineConfig} from "vitest/config";
+import {configDefaults, defineConfig} from "vitest/config";
 
 export default defineConfig({
     test: {
         watch: false,
         clearMocks: true,
         setupFiles: ["./config/tests/setup.ts"],
+        // Live eval-corpus trees are case fixtures, not suite code: a tree
+        // may legitimately carry a *.test.ts whose tests fail by design
+        // (the test-adequacy cases), so vitest must never execute them.
+        exclude: [
+            ...configDefaults.exclude,
+            "workflows/review/eval/corpus/**/tree/**",
+        ],
     },
 });

workflows/review/eval/corpus/clean/clean-typed-refactor/case.json

@@ -0,0 +1,33 @@
+{
+    "id": "clean-typed-refactor",
+    "tags": [
+        "clean",
+        "live"
+    ],
+    "category": "clean",
+    "description": "Known-clean PR: a mechanical rename of a helper across a few files with no behaviour change. No lens produces a finding; the run must approve without posting.",
+    "changedFiles": [
+        {
+            "path": "src/util/format.ts",
+            "status": "modified"
+        },
+        {
+            "path": "src/util/format.test.ts",
+            "status": "modified"
+        }
+    ],
+    "findings": [],
+    "expected": {
+        "verdict": "APPROVE",
+        "postedCommentCount": 0
+    },
+    "diff": "diff --git a/src/util/format.ts b/src/util/format.ts\n--- a/src/util/format.ts\n+++ b/src/util/format.ts\n@@ -1,7 +1,7 @@\n /** Shared display formatting helpers. */\n \n /** Format a fractional amount as a fixed two-decimal string. */\n-export const fmt = (amount: number): string => amount.toFixed(2);\n+export const formatAmount = (amount: number): string => amount.toFixed(2);\n \n /** Format a byte count using binary units. */\n export const formatBytes = (bytes: number): string => {\ndiff --git a/src/util/format.test.ts b/src/util/format.test.ts\n--- a/src/util/format.test.ts\n+++ b/src/util/format.test.ts\n@@ -1,11 +1,11 @@\n import {describe, expect, it} from \"vitest\";\n \n-import {fmt, formatBytes} from \"./format\";\n+import {formatAmount, formatBytes} from \"./format\";\n \n-describe(\"fmt\", () => {\n+describe(\"formatAmount\", () => {\n     it(\"renders two decimals\", () => {\n-        expect(fmt(3)).toBe(\"3.00\");\n-        expect(fmt(3.456)).toBe(\"3.46\");\n+        expect(formatAmount(3)).toBe(\"3.00\");\n+        expect(formatAmount(3.456)).toBe(\"3.46\");\n     });\n });\n \n",
+    "live": {
+        "prContext": {
+            "title": "util: rename fmt to formatAmount",
+            "description": "Mechanical rename; fmt was too terse to grep for. No behavior change.",
+            "author": "dev-web",
+            "baseBranch": "main"
+        }
+    }
+}

workflows/review/eval/corpus/clean/clean-typed-refactor/tree/src/util/format.test.ts

@@ -0,0 +1,17 @@
+import {describe, expect, it} from "vitest";
+
+import {formatAmount, formatBytes} from "./format";
+
+describe("formatAmount", () => {
+    it("renders two decimals", () => {
+        expect(formatAmount(3)).toBe("3.00");
+        expect(formatAmount(3.456)).toBe("3.46");
+    });
+});
+
+describe("formatBytes", () => {
+    it("picks binary units", () => {
+        expect(formatBytes(512)).toBe("512 B");
+        expect(formatBytes(2048)).toBe("2.0 KiB");
+    });
+});

workflows/review/eval/corpus/clean/clean-typed-refactor/tree/src/util/format.ts

@@ -0,0 +1,13 @@
+/** Shared display formatting helpers. */
+
+/** Format a fractional amount as a fixed two-decimal string. */
+export const formatAmount = (amount: number): string => amount.toFixed(2);
+
+/** Format a byte count using binary units. */
+export const formatBytes = (bytes: number): string => {
+    if (bytes < 1024) {
+        return `${bytes} B`;
+    }
+    const kib = bytes / 1024;
+    return kib < 1024 ? `${kib.toFixed(1)} KiB` : `${(kib / 1024).toFixed(1)} MiB`;
+};

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

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

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

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

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

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