diff --git a/.changeset/review-eval-corpus-discrimination.md b/.changeset/review-eval-corpus-discrimination.md new file mode 100644 index 00000000..41f0722d --- /dev/null +++ b/.changeset/review-eval-corpus-discrimination.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +Eval-only: improve the discrimination power of the review eval corpus. Fixes the floor-case ground truth found by the 2026-07-20 drift triage (lifecycle-3's void prune was a real defect the reviewer was right to block, dedup-eventual-consistency's expected verdict contradicted its own premise, retention-unbounded-prune was retired as unevidenceable in-tree), repairs order-sensitive spec regexes that under-counted deterministic matches, annotates four real defects mined from the drift run's unmatched-posted noise pool, mints two mid-band retention/lifecycle cases, and raises the weekly drift budget default from 85 to 120 USD so the full corpus clears without budget skips. No change to the shipped review workflow. diff --git a/.github/workflows/review-eval-drift.yml b/.github/workflows/review-eval-drift.yml index 5e380124..e9c4b97c 100644 --- a/.github/workflows/review-eval-drift.yml +++ b/.github/workflows/review-eval-drift.yml @@ -31,9 +31,20 @@ on: required: false default: "3" max_usd: + # Sized from the 2026-07-20 run (29724668102): $84.33 for 103 + # case-arm-runs (~$0.82 each) with the tail of the corpus + # budget-skipped, which also contaminated the noise-floor bands + # (asymmetric case mix). The 23-case corpus (both new mutation cases + # are live-tagged) needs 23 x 3 repeats x 2 arms = 138 case-arm-runs, + # ~$113; 120 clears it while staying a hard stop against runaways, + # though the headroom is thin: the ~$0.82/run mean comes from a run + # whose tail was budget-skipped, so it may understate. Repeats stay + # at 3: they are + # the power axis, and per-case repeat tiering would make every + # noise-floor sample a different case mix. description: "Total hard budget across both arms and all repeats" required: false - default: "85" + default: "120" permissions: # write: the report step commits the aggregate under @@ -60,7 +71,7 @@ jobs: env: ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} REPEATS: ${{ inputs.repeats || '3' }} - MAX_USD: ${{ inputs.max_usd || '85' }} + MAX_USD: ${{ inputs.max_usd || '120' }} run: | if [ -z "$ANTHROPIC_API_KEY" ]; then echo "ANTHROPIC_API_KEY secret not configured; skipping the drift run." >&2 diff --git a/workflows/review/eval/corpus/golden/golden-retention-fix-push/case.json b/workflows/review/eval/corpus/golden/golden-retention-fix-push/case.json index c2c4239e..d45df1a4 100644 --- a/workflows/review/eval/corpus/golden/golden-retention-fix-push/case.json +++ b/workflows/review/eval/corpus/golden/golden-retention-fix-push/case.json @@ -6,7 +6,7 @@ "live" ], "category": "golden", - "description": "The under-tripwire re-review pricing case: an open PR touched five regions of quota.ts and the prior full review blocked one (an off-by-one in the remaining-quota math). The fix push rewrites only that region (unreviewed share 0.2, below the tripwire threshold), so scoped/flip-gated/fast actually execute their reduced paths. The fix resolves the thread but plants a fresh defect in the same hunk: the remaining count is cached under a shared key with no user id, leaking quota across users. scoped and flip-gated stage exactly that hunk and must catch it; fast dispatches no finder and cannot, which is the recall cost the mode sweep prices.", + "description": "The under-tripwire re-review pricing case: an open PR touched five regions of quota.ts and the prior full review blocked one (an off-by-one in the remaining-quota math). The fix push rewrites only that region (unreviewed share 0.2, below the tripwire threshold), so scoped/flip-gated/fast actually execute their reduced paths. The fix resolves the thread but plants a fresh defect in the same hunk: the remaining count is cached under a shared key with no user id, leaking quota across users. scoped and flip-gated stage exactly that hunk and must catch it; fast dispatches no finder and cannot, which is the recall cost the mode sweep prices. The rewritten hunk also leaves quotaExceeded unsatisfiable (remaining is clamped to zero, then compared < 0, so the quota is never enforced); a second real defect annotated 2026-07-20 from the drift noise pool.", "changedFiles": [ { "path": "src/notes/quota.ts", @@ -68,6 +68,17 @@ "quota-remaining", "another user|cross.user|leak" ] + }, + { + "key": "quota-exceeded-never-true", + "path": "src/notes/quota.ts", + "lineStart": 30, + "lineEnd": 36, + "mechanism": [ + "never (returns? )?true|always (returns? )?false|can never (be true|be negative|exceed)|unsatisfiable", + "Math\\.max\\(0.{0,80}< 0|clamp(ed|s)?.{0,80}< 0|< 0 is (always )?false|0 < 0", + "unlimited (notes|saves)|quota (is )?never enforced|save is (always )?allowed|never blocked" + ] } ], "rereview": { diff --git a/workflows/review/eval/corpus/golden/golden-retention-lifecycle-1/case.json b/workflows/review/eval/corpus/golden/golden-retention-lifecycle-1/case.json index 91a75a11..45ddec5e 100644 --- a/workflows/review/eval/corpus/golden/golden-retention-lifecycle-1/case.json +++ b/workflows/review/eval/corpus/golden/golden-retention-lifecycle-1/case.json @@ -6,7 +6,7 @@ "live" ], "category": "golden", - "description": "Push 1 of the retention lifecycle (sanitized port of a seeded live trial): a per-user note cap with pruning, and note deletion wired into account deletion. Four planted defects: deletion removes at most one note (facade pageSize defaults to 1), the prune offset keeps one note fewer than the cap, the prune query loads the entire result set, and the prune test asserts nothing the cap needs.", + "description": "Push 1 of the retention lifecycle (sanitized port of a seeded live trial): a per-user note cap with pruning, and note deletion wired into account deletion. Four planted defects: deletion removes at most one note (facade pageSize defaults to 1), the prune offset keeps one note fewer than the cap, the fire-and-forget prune discards rejections with no handler (an unhandled rejection can crash the process), and the prune test asserts nothing the cap needs. The former retention-unbounded-prune spec (pageSize \"all\" into one deleteMulti) was retired 2026-07-20: the facade documents no batch limit, so the defect was not evidenceable in-tree and read 0% across every drift sample; mutation-retention-batch-delete-limit covers the mechanism with the limit documented.", "changedFiles": [ { "path": "src/notes/purge-user-data.ts", @@ -71,22 +71,24 @@ "source": "correctness", "finding": { "schema_version": 2, - "id": "retention-unbounded-prune", + "id": "retention-prune-unhandled-rejection", "lens": "correctness", "anchor": { "type": "line", "path": "src/notes/retention.ts", - "line": 17, + "line": 9, "side": "RIGHT" }, "severity": "blocking", - "confidence": 0.9, + "confidence": 0.85, "evidence_trace": [ - "src/notes/retention.ts: pageSize \"all\" on the prune query" + "src/notes/retention.ts:9 fires pruneNotes with `void` and no rejection handler", + "db.query/deleteMulti inside pruneNotes can reject on a transient datastore error", + "an unhandled promise rejection terminates the Node process under default settings" ], - "failure_scenario": "A user far over the cap triggers prune; pageSize \"all\" loads every stale row into memory in one call, so a large backlog makes prune (and the save that fired it) balloon instead of deleting in bounded batches.", - "producing_hunt": "correctness:unbounded-read", - "model_authored_prose": "The prune query loads the entire stale result set at once (`pageSize: \"all\"`). Delete in bounded batches so a large backlog cannot balloon a save." + "failure_scenario": "A transient datastore error makes pruneNotes reject after a save; the `void`-discarded promise has no .catch, so the rejection is unhandled and crashes the process under Node's default --unhandled-rejections=throw; the opposite of the comment's 'a failing prune must not fail the save'.", + "producing_hunt": "correctness:unhandled-rejection", + "model_authored_prose": "`void pruneNotes(...)` discards the promise but not its rejection: a failing prune becomes an unhandledRejection and can crash the process. Attach `.catch(() => {})` (or log the error) so the documented fire-and-forget intent actually holds." } }, { @@ -122,7 +124,7 @@ "verification": "confirmed" }, { - "id": "retention-unbounded-prune", + "id": "retention-prune-unhandled-rejection", "verification": "confirmed" }, { @@ -136,7 +138,7 @@ "mustCatch": [ "retention-purge-limit", "retention-cap-off-by-one", - "retention-unbounded-prune", + "retention-prune-unhandled-rejection", "retention-test-asserts-nothing" ] }, @@ -168,12 +170,14 @@ ] }, { - "key": "retention-unbounded-prune", + "key": "retention-prune-unhandled-rejection", "path": "src/notes/retention.ts", + "lineStart": 6, + "lineEnd": 10, "mechanism": [ - "pageSize.*all", - "entire result|unbounded|all (stale )?rows", - "batch" + "unhandled (promise )?rejection|unhandledRejection", + "no \\.?catch|without (a )?\\.?catch|no rejection handler|neither awaited nor caught", + "crash(es|ed)?.{0,30}process|terminat(es|e|ing).{0,30}process|process (crash|exit)" ] }, { diff --git a/workflows/review/eval/corpus/golden/golden-retention-lifecycle-2/case.json b/workflows/review/eval/corpus/golden/golden-retention-lifecycle-2/case.json index 9e6412a0..dcea94c8 100644 --- a/workflows/review/eval/corpus/golden/golden-retention-lifecycle-2/case.json +++ b/workflows/review/eval/corpus/golden/golden-retention-lifecycle-2/case.json @@ -6,7 +6,7 @@ "live" ], "category": "golden", - "description": "Push 2 of the retention lifecycle: a bad partial fix with added bugs, as an open-PR re-review snapshot. The purge and the unbounded prune are properly fixed; the off-by-one is 'fixed' to a different off-by-one (offset MAX+1) so its thread must be KEPT; the test fix is deferred in a reply so its thread must be KEPT; and the new dedup feature plants a fresh blocking defect (a lowercased 8-char prefix key silently drops distinct notes) plus an untested dedup window.", + "description": "Push 2 of the retention lifecycle: a bad partial fix with added bugs, as an open-PR re-review snapshot. The purge and the unbounded prune are properly fixed; the off-by-one is 'fixed' to a different off-by-one (offset MAX+1) so its thread must be KEPT; the test fix is deferred in a reply so its thread must be KEPT; and the new dedup feature plants a fresh blocking defect (a lowercased 8-char prefix key silently drops distinct notes) plus an untested dedup window. The fire-and-forget prune still has no rejection handler (carried from push 1, annotated 2026-07-20 from the drift noise pool). The overcorrected off-by-one and the still-vacuous test are deliberately NOT specs: their prior threads are KEPT, so fresh findings there are re-review duplicates, not recall.", "changedFiles": [ { "path": "src/notes/purge-user-data.ts", @@ -107,9 +107,27 @@ "key": "retention-dedup-window-untested", "path": "src/notes/retention.ts", "mechanism": [ - "window.*(untested|no test)|no test.*window", + "window.{0,60}(untested|no test|not tested|never tested)|no test.{0,60}window", "DEDUP_WINDOW", - "boundary" + "(boundary|edge).{0,60}(window|dedup)|(window|dedup).{0,60}(boundary|edge)", + "dedup.{0,80}(untested|no test|never (tested|exercised|covered|verifie[sd])|not (covered|tested|exercised|verified))|never verifies.{0,60}dedup", + "(test|coverage).{0,80}dedup" + ], + "altLocations": [ + { + "path": "src/notes/retention.test.ts" + } + ] + }, + { + "key": "retention-prune-unhandled-rejection", + "path": "src/notes/retention.ts", + "lineStart": 29, + "lineEnd": 33, + "mechanism": [ + "unhandled (promise )?rejection|unhandledRejection", + "no \\.?catch|without (a )?\\.?catch|no rejection handler|neither awaited nor caught", + "crash(es|ed)?.{0,30}process|terminat(es|e|ing).{0,30}process|process (crash|exit)" ] } ], diff --git a/workflows/review/eval/corpus/golden/golden-retention-lifecycle-3/case.json b/workflows/review/eval/corpus/golden/golden-retention-lifecycle-3/case.json index b0a742c1..d954b500 100644 --- a/workflows/review/eval/corpus/golden/golden-retention-lifecycle-3/case.json +++ b/workflows/review/eval/corpus/golden/golden-retention-lifecycle-3/case.json @@ -6,7 +6,7 @@ "live" ], "category": "golden", - "description": "Push 3 of the retention lifecycle: everything fixed, as an open-PR re-review snapshot. Every prior thread (the two kept blocking threads and the two push-2 findings) is addressed, so the reconciler must resolve all four and the verdict must flip to APPROVE. The intentional fire-and-forget prune error is a trap: it is documented as deliberate and must not be flagged.", + "description": "Push 3 of the retention lifecycle: everything fixed, as an open-PR re-review snapshot. Every prior thread (the two kept blocking threads and the two push-2 findings) is addressed, so the reconciler must resolve all four and the verdict must flip to APPROVE. The deliberately fire-and-forget prune (rejections swallowed via .catch, documented as intentional) is a trap: it must not be flagged. Until 2026-07-20 the prune was a bare `void` call, a real crash-on-rejection defect the reviewer correctly blocked in every drift sample; the tree now implements the documented intent so the expected APPROVE is fair.", "changedFiles": [ { "path": "src/notes/purge-user-data.ts", @@ -27,7 +27,7 @@ "postedCommentCount": 0, "mustNotPost": [] }, - "diff": "diff --git a/src/notes/purge-user-data.ts b/src/notes/purge-user-data.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/purge-user-data.ts\n@@ -0,0 +1,15 @@\n+import type {Db} from \"./db\";\n+\n+/** Part of account deletion: remove every stored note for the user. */\n+export const purgeUserNotes = async (\n+ db: Db,\n+ userId: string,\n+): Promise => {\n+ for (;;) {\n+ const notes = await db.query(\"Note\", {userId, pageSize: 100});\n+ if (notes.length === 0) {\n+ return;\n+ }\n+ await db.deleteMulti(notes.map((note) => note.id));\n+ }\n+};\ndiff --git a/src/notes/retention.test.ts b/src/notes/retention.test.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/retention.test.ts\n@@ -0,0 +1,30 @@\n+import {memDb, note} from \"./testing\";\n+import {MAX_NOTES_PER_USER, pruneNotes, saveNote} from \"./retention\";\n+\n+const count = async (db: ReturnType): Promise =>\n+ (await db.query(\"Note\", {userId: \"u1\", pageSize: \"all\"})).length;\n+\n+describe(\"retention\", () => {\n+ it(\"caps stored notes per user\", async () => {\n+ const db = memDb();\n+ for (let i = 0; i < MAX_NOTES_PER_USER + 5; i++) {\n+ await saveNote(db, note(\"u1\", `note ${i}`));\n+ }\n+ await pruneNotes(db, \"u1\");\n+ expect(await count(db)).toBeLessThanOrEqual(MAX_NOTES_PER_USER);\n+ });\n+\n+ it(\"dedups an identical note inside the window\", async () => {\n+ const db = memDb();\n+ await saveNote(db, note(\"u1\", \"same content\"));\n+ await saveNote(db, note(\"u1\", \"same content\"));\n+ expect(await count(db)).toBe(1);\n+ });\n+\n+ it(\"keeps distinct notes that share a prefix\", async () => {\n+ const db = memDb();\n+ await saveNote(db, note(\"u1\", \"prefix: alpha\"));\n+ await saveNote(db, note(\"u1\", \"prefix: beta\"));\n+ expect(await count(db)).toBe(2);\n+ });\n+});\ndiff --git a/src/notes/retention.ts b/src/notes/retention.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/retention.ts\n@@ -0,0 +1,39 @@\n+import type {Db, Note} from \"./db\";\n+\n+/** Hard cap on stored notes per user; prune enforces it on every save. */\n+export const MAX_NOTES_PER_USER = 200;\n+\n+/** Window inside which a repeated note is treated as a duplicate. */\n+export const DEDUP_WINDOW_MS = 30 * 24 * 60 * 60 * 1000;\n+\n+const PRUNE_BATCH = 50;\n+\n+export const saveNote = async (db: Db, note: Note): Promise => {\n+ const existing = await db.query(\"Note\", {\n+ userId: note.userId,\n+ pageSize: \"all\",\n+ });\n+ const since = note.createdAt - DEDUP_WINDOW_MS;\n+ const duplicate = existing.some(\n+ (stored) =>\n+ stored.createdAt >= since && stored.content === note.content,\n+ );\n+ if (duplicate) {\n+ return;\n+ }\n+ await db.put(note);\n+ // Intentionally fire-and-forget: a failing prune must not fail the save.\n+ void pruneNotes(db, note.userId);\n+};\n+\n+export const pruneNotes = async (db: Db, userId: string): Promise => {\n+ const stale = await db.query(\"Note\", {\n+ userId,\n+ orderDesc: \"createdAt\",\n+ offset: MAX_NOTES_PER_USER,\n+ pageSize: PRUNE_BATCH,\n+ });\n+ if (stale.length > 0) {\n+ await db.deleteMulti(stale.map((note) => note.id));\n+ }\n+};\n", + "diff": "diff --git a/src/notes/purge-user-data.ts b/src/notes/purge-user-data.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/purge-user-data.ts\n@@ -0,0 +1,15 @@\n+import type {Db} from \"./db\";\n+\n+/** Part of account deletion: remove every stored note for the user. */\n+export const purgeUserNotes = async (\n+ db: Db,\n+ userId: string,\n+): Promise => {\n+ for (;;) {\n+ const notes = await db.query(\"Note\", {userId, pageSize: 100});\n+ if (notes.length === 0) {\n+ return;\n+ }\n+ await db.deleteMulti(notes.map((note) => note.id));\n+ }\n+};\ndiff --git a/src/notes/retention.test.ts b/src/notes/retention.test.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/retention.test.ts\n@@ -0,0 +1,30 @@\n+import {memDb, note} from \"./testing\";\n+import {MAX_NOTES_PER_USER, pruneNotes, saveNote} from \"./retention\";\n+\n+const count = async (db: ReturnType): Promise =>\n+ (await db.query(\"Note\", {userId: \"u1\", pageSize: \"all\"})).length;\n+\n+describe(\"retention\", () => {\n+ it(\"caps stored notes per user\", async () => {\n+ const db = memDb();\n+ for (let i = 0; i < MAX_NOTES_PER_USER + 5; i++) {\n+ await saveNote(db, note(\"u1\", `note ${i}`));\n+ }\n+ await pruneNotes(db, \"u1\");\n+ expect(await count(db)).toBeLessThanOrEqual(MAX_NOTES_PER_USER);\n+ });\n+\n+ it(\"dedups an identical note inside the window\", async () => {\n+ const db = memDb();\n+ await saveNote(db, note(\"u1\", \"same content\"));\n+ await saveNote(db, note(\"u1\", \"same content\"));\n+ expect(await count(db)).toBe(1);\n+ });\n+\n+ it(\"keeps distinct notes that share a prefix\", async () => {\n+ const db = memDb();\n+ await saveNote(db, note(\"u1\", \"prefix: alpha\"));\n+ await saveNote(db, note(\"u1\", \"prefix: beta\"));\n+ expect(await count(db)).toBe(2);\n+ });\n+});\ndiff --git a/src/notes/retention.ts b/src/notes/retention.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/retention.ts\n@@ -0,0 +1,39 @@\n+import type {Db, Note} from \"./db\";\n+\n+/** Hard cap on stored notes per user; prune enforces it on every save. */\n+export const MAX_NOTES_PER_USER = 200;\n+\n+/** Window inside which a repeated note is treated as a duplicate. */\n+export const DEDUP_WINDOW_MS = 30 * 24 * 60 * 60 * 1000;\n+\n+const PRUNE_BATCH = 50;\n+\n+export const saveNote = async (db: Db, note: Note): Promise => {\n+ const existing = await db.query(\"Note\", {\n+ userId: note.userId,\n+ pageSize: \"all\",\n+ });\n+ const since = note.createdAt - DEDUP_WINDOW_MS;\n+ const duplicate = existing.some(\n+ (stored) =>\n+ stored.createdAt >= since && stored.content === note.content,\n+ );\n+ if (duplicate) {\n+ return;\n+ }\n+ await db.put(note);\n+ // Intentionally fire-and-forget: a failing prune must not fail the save.\n+ pruneNotes(db, note.userId).catch(() => {});\n+};\n+\n+export const pruneNotes = async (db: Db, userId: string): Promise => {\n+ const stale = await db.query(\"Note\", {\n+ userId,\n+ orderDesc: \"createdAt\",\n+ offset: MAX_NOTES_PER_USER,\n+ pageSize: PRUNE_BATCH,\n+ });\n+ if (stale.length > 0) {\n+ await db.deleteMulti(stale.map((note) => note.id));\n+ }\n+};\n", "live": { "prContext": { "title": "notes: cap stored notes per user and delete them with user data", @@ -39,9 +39,10 @@ { "key": "retention-void-prune-trap", "path": "src/notes/retention.ts", + "blockingOnly": true, "mechanism": [ - "void prune|fire.and.forget", - "discard(ed|s)?.*(error|rejection)|unhandled" + "void prune|fire.and.forget|\\.catch\\(\\(\\) =>", + "discard(ed|s)?.{0,40}(error|rejection)|unhandled|swallow(s|ed)?|silently (ignor|drop|discard)" ] } ], diff --git a/workflows/review/eval/corpus/golden/golden-retention-lifecycle-3/tree/src/notes/retention.ts b/workflows/review/eval/corpus/golden/golden-retention-lifecycle-3/tree/src/notes/retention.ts index dfa31a21..25542c34 100644 --- a/workflows/review/eval/corpus/golden/golden-retention-lifecycle-3/tree/src/notes/retention.ts +++ b/workflows/review/eval/corpus/golden/golden-retention-lifecycle-3/tree/src/notes/retention.ts @@ -23,7 +23,7 @@ export const saveNote = async (db: Db, note: Note): Promise => { } await db.put(note); // Intentionally fire-and-forget: a failing prune must not fail the save. - void pruneNotes(db, note.userId); + pruneNotes(db, note.userId).catch(() => {}); }; export const pruneNotes = async (db: Db, userId: string): Promise => { 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 index e8452eae..55d7d7e7 100644 --- a/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/case.json +++ b/workflows/review/eval/corpus/incidents/trial-dedup-composite-key/case.json @@ -73,7 +73,10 @@ "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)" + "(dedup|duplicate check).{0,40}(drops?|skips?|suppress).{0,40}(summary|kind)", + "seen\\[note\\.Body\\]|dedup key is (the )?Body|key is (the )?Body( alone| only)?|keyed (only )?(by|on) (note\\.)?Body|Body[- ]only|(dedup|duplicate check) (is |uses )?(on |by )?Body alone", + "(same|identical|matching) Body.{0,100}(different|distinct|another|other) [Kk]ind|[Kk]ind.{0,100}(same|identical|matching) Body", + "(summary|generated summary).{0,100}(silently )?(dropped|skipped|suppressed|never (stored|persisted|saved))|(silently )?(dropped|skipped|suppressed).{0,60}(summary|distinct note)" ], "lens": "correctness", "lineStart": 27, 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 index aa26b5e6..d1f7eed2 100644 --- a/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/case.json +++ b/workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/case.json @@ -6,7 +6,7 @@ "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.", + "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. Revised 2026-07-20: the expected verdict is REQUEST_CHANGES, not APPROVE. The PR's stated purpose is stopping double-submit duplicates, and the eventually consistent read fails for precisely that traffic, so the change does not deliver what it claims; the reviewer blocked it in every drift sample and that judgment is correct.", "changedFiles": [ { "path": "services/notes/save.go", @@ -31,8 +31,8 @@ "line": 23, "side": "RIGHT" }, - "severity": "advisory", - "confidence": 0.65, + "severity": "blocking", + "confidence": 0.7, "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", @@ -45,7 +45,7 @@ } ], "expected": { - "verdict": "APPROVE", + "verdict": "REQUEST_CHANGES", "mustCatch": [ "save-dedup-stale-read" ], diff --git a/workflows/review/eval/corpus/live.ts b/workflows/review/eval/corpus/live.ts index f7d29fbd..9f34ff0f 100644 --- a/workflows/review/eval/corpus/live.ts +++ b/workflows/review/eval/corpus/live.ts @@ -82,6 +82,18 @@ export type LiveDefectSpec = { * mechanism alternates still have to agree wherever the finding anchors. */ altLocations?: LiveSpecLocation[]; + /** + * When true, only a candidate with a blocking label can satisfy the + * spec. Meant for must-not-flag traps guarding a documented deliberate + * pattern: the trap should claim a finding that condemns the pattern as + * a defect, not a non-blocking advisory that merely names it while + * diagnosing something else (observed in the 2026-07-20 A/B: companion + * "log instead of an empty catch" suggestions on the batch-limit defect + * matched the swallow trap's mechanism and scored as false flags). A + * non-blocking mention stays noise, which the noise metric already + * counts. + */ + blockingOnly?: boolean; }; /** @@ -246,6 +258,11 @@ const parseDefectSpecs = ( errors.push(`${at}.lens: must be a non-empty string when present`); return; } + const blockingOnly = entry["blockingOnly"]; + if (blockingOnly !== undefined && typeof blockingOnly !== "boolean") { + errors.push(`${at}.blockingOnly: must be a boolean when present`); + return; + } const rawAlt = entry["altLocations"]; let altLocations: LiveSpecLocation[] | undefined; if (rawAlt !== undefined) { @@ -329,6 +346,9 @@ const parseDefectSpecs = ( if (altLocations !== undefined) { spec.altLocations = altLocations; } + if (blockingOnly !== undefined) { + spec.blockingOnly = blockingOnly; + } specs.push(spec); }); return specs; diff --git a/workflows/review/eval/corpus/loader.test.ts b/workflows/review/eval/corpus/loader.test.ts index 66f2f412..92dbe205 100644 --- a/workflows/review/eval/corpus/loader.test.ts +++ b/workflows/review/eval/corpus/loader.test.ts @@ -120,6 +120,7 @@ describe("parseCase: the live block", () => { key: "trap-1", path: "src/a.ts", mechanism: ["wrapper chunks internally"], + blockingOnly: true, }, ], }, @@ -129,6 +130,8 @@ describe("parseCase: the live block", () => { expect(parsed.live?.mustCatchSpecs?.[0]?.key).toBe("bug-1"); expect(parsed.live?.mustCatchSpecs?.[0]?.lineEnd).toBe(2); expect(parsed.live?.mustNotFlagSpecs?.[0]?.lineStart).toBeUndefined(); + expect(parsed.live?.mustNotFlagSpecs?.[0]?.blockingOnly).toBe(true); + expect(parsed.live?.mustCatchSpecs?.[0]?.blockingOnly).toBeUndefined(); }); it("parses and validates altLocations like the primary location", () => { diff --git a/workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json b/workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json index cc333498..3a03a868 100644 --- a/workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json +++ b/workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json @@ -7,7 +7,7 @@ "live" ], "category": "incident-repro", - "description": "Repro of a production incident: a migration adds a column filtered by a hot query but no index, causing a table scan under load. The data-migrations lens must catch it and block. The defect spans two files, so the spec accepts an anchor at the migration OR at the hot query in order.ts (the 07-09 wave read 8/16 on this case, and every 'miss' was the same blocking finding anchored at the query site; a single-location spec measured anchor-site preference, not recall).", + "description": "Repro of a production incident: a migration adds a column filtered by a hot query but no index, causing a table scan under load. The data-migrations lens must catch it and block. The defect spans two files, so the spec accepts an anchor at the migration OR at the hot query in order.ts (the 07-09 wave read 8/16 on this case, and every 'miss' was the same blocking finding anchored at the query site; a single-location spec measured anchor-site preference, not recall). A second real defect was annotated 2026-07-20 from the drift noise pool: the migration's DEFAULT 'pending' backfills every pre-existing (long fulfilled) order into the picker's pending work queue.", "changedFiles": [ { "path": "db/migrations/20260601_add_status.sql", @@ -45,7 +45,7 @@ ], "failure_scenario": "Once the table grows, the `status` filter in order.ts runs as a full table scan and the hot query times out under load.", "producing_hunt": "data-migrations:index-coverage", - "model_authored_prose": "This migration adds `status` but no index, yet `order.ts` filters on it \u2014 add an index for `status` or the hot query will table-scan under load.", + "model_authored_prose": "This migration adds `status` but no index, yet `order.ts` filters on it — add an index for `status` or the hot query will table-scan under load.", "suggested_patch": "CREATE INDEX CONCURRENTLY idx_orders_status ON orders (status);" } } @@ -84,6 +84,20 @@ "lineEnd": 18 } ] + }, + { + "key": "dm-default-backfill", + "path": "db/migrations/20260601_add_status.sql", + "mechanism": [ + "backfill(s|ed)?", + "(pre.existing|existing|historical|fulfilled|old|every) (order|row)s?.{0,100}('|\")?pending|('|\")?pending('|\")?.{0,100}(pre.existing|existing|historical|fulfilled|old|every) (order|row)s?", + "DEFAULT ('|\")pending.{0,120}(work queue|picker|seed|flood)" + ], + "altLocations": [ + { + "path": "src/models/order.ts" + } + ] } ] }, diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/case.json b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/case.json new file mode 100644 index 00000000..0e6b726a --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/case.json @@ -0,0 +1,107 @@ +{ + "id": "mutation-retention-batch-delete-limit", + "tags": [ + "synthetic-mutation", + "retention-lifecycle", + "live" + ], + "category": "synthetic-mutation", + "description": "Easier-graded variant of the retired retention-unbounded-prune spec (0% across every 2026-07-20 drift sample: the batch limit was not evidenceable in-tree). Here the evidence exists: the unchanged purge-user-data.ts documents that the datastore rejects deleteMulti batches over 100 keys and chunks accordingly, while the new prune passes every stale id (a launch backlog can be hundreds) to one deleteMulti call. The rejection is swallowed by the documented fire-and-forget catch, so the cap silently never engages for exactly the over-cap users it targets, and the backlog test passes only because the in-memory fake enforces no batch limit. Multi-file retention/lifecycle reasoning, the reviewer's demonstrated weak class; authored for the 20-80 percent band but calibrated saturated (identical-arm run 29765059892 2/2, then 6/6 across both arms of the 2026-07-20 pooled A/B, runs 29783522498-29783549087), consistent with the eval README's synthetic-saturation finding; it stays as a family tripwire. The mustCatch window deliberately covers the fire-and-forget .catch line (it is part of the defect's causal chain, and mustCatch claims run before the swallow trap), so a correct diagnosis anchored there scores as a catch, not a false flag; the trap only claims findings that condemn the documented fire-and-forget itself without the batch-limit mechanism.", + "changedFiles": [ + { + "path": "src/notes/retention.ts", + "status": "added" + }, + { + "path": "src/notes/retention.test.ts", + "status": "added" + } + ], + "dimensions": { + "correctness": "assessed", + "skillSeverity": "assessed", + "patternTriage": "assessed" + }, + "findings": [ + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "retention-batch-delete-limit", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "src/notes/retention.ts", + "line": 21, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.85, + "evidence_trace": [ + "src/notes/purge-user-data.ts documents that the datastore rejects deleteMulti batches over 100 keys and chunks every bulk delete", + "src/notes/retention.ts:21 passes the entire stale set (pageSize \"all\") to a single deleteMulti call", + "the fire-and-forget .catch swallows the rejection, so the failure is silent" + ], + "failure_scenario": "A user with a 500-note backlog saves once after the cap ships; prune queries 300 stale ids and passes all of them to one deleteMulti call, which the datastore rejects (over the documented 100-key batch limit); the .catch swallows the error, so the cap silently never engages for exactly the over-cap users it targets.", + "producing_hunt": "correctness:batch-limit", + "model_authored_prose": "pruneNotes deletes the whole stale set in one deleteMulti call, but the datastore rejects batches over 100 keys (see the chunked delete in purge-user-data.ts). Chunk the ids like purgeUserNotes does; the backlog test only passes because the in-memory fake enforces no limit." + } + } + ], + "validation": [ + { + "id": "retention-batch-delete-limit", + "verification": "confirmed" + } + ], + "policyConflicts": [], + "expected": { + "verdict": "REQUEST_CHANGES", + "mustCatch": [ + "retention-batch-delete-limit" + ], + "postedCommentCount": 1 + }, + "diff": "diff --git a/src/notes/retention.test.ts b/src/notes/retention.test.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/retention.test.ts\n@@ -0,0 +1,17 @@\n+import {memDb, note} from \"./testing\";\n+import {MAX_NOTES_PER_USER, pruneNotes, saveNote} from \"./retention\";\n+\n+const count = async (db: ReturnType): Promise =>\n+ (await db.query(\"Note\", {userId: \"u1\", pageSize: \"all\"})).length;\n+\n+describe(\"retention\", () => {\n+ it(\"caps stored notes per user, clearing a large backlog\", async () => {\n+ const db = memDb();\n+ for (let i = 0; i < MAX_NOTES_PER_USER + 150; i++) {\n+ await db.put(note(\"u1\", `imported ${i}`));\n+ }\n+ await saveNote(db, note(\"u1\", \"fresh\"));\n+ await pruneNotes(db, \"u1\");\n+ expect(await count(db)).toBe(MAX_NOTES_PER_USER);\n+ });\n+});\ndiff --git a/src/notes/retention.ts b/src/notes/retention.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/retention.ts\n@@ -0,0 +1,23 @@\n+import type {Db, Note} from \"./db\";\n+\n+/** Hard cap on stored notes per user; prune enforces it on every save. */\n+export const MAX_NOTES_PER_USER = 200;\n+\n+export const saveNote = async (db: Db, note: Note): Promise => {\n+ await db.put(note);\n+ // Intentionally fire-and-forget: a failing prune must not fail the save.\n+ pruneNotes(db, note.userId).catch(() => {});\n+};\n+\n+/** Deletes every stored note past the cap, oldest first. */\n+export const pruneNotes = async (db: Db, userId: string): Promise => {\n+ const stale = await db.query(\"Note\", {\n+ userId,\n+ orderDesc: \"createdAt\",\n+ offset: MAX_NOTES_PER_USER,\n+ pageSize: \"all\",\n+ });\n+ if (stale.length > 0) {\n+ await db.deleteMulti(stale.map((note) => note.id));\n+ }\n+};\n", + "live": { + "prContext": { + "title": "notes: cap stored notes per user", + "description": "Adds a per-user retention cap enforced on every save. The first save after deploy also clears any pre-existing backlog past the cap. Covered by a backlog test in the retention suite.", + "author": "dev-notes", + "baseBranch": "main" + }, + "tree": "tree", + "mustCatchSpecs": [ + { + "key": "retention-batch-delete-limit", + "path": "src/notes/retention.ts", + "lineStart": 6, + "lineEnd": 22, + "mechanism": [ + "100|DELETE_CHUNK|chunk(ed|ing|s)?", + "deleteMulti.{0,80}(reject|fail|limit|too many|more than|exceed|batch)|(reject|limit|batch).{0,60}deleteMulti", + "(backlog|large|hundreds|300|500).{0,80}(one|single) (call|batch)|(one|single) (call|batch).{0,80}(backlog|entire|every|all)", + "(fake|memDb|in.memory).{0,80}(enforce|reject|limit)" + ], + "altLocations": [ + { + "path": "src/notes/retention.test.ts" + } + ] + } + ], + "mustNotFlagSpecs": [ + { + "key": "retention-catch-swallow-trap", + "path": "src/notes/retention.ts", + "lineStart": 6, + "lineEnd": 10, + "blockingOnly": true, + "mechanism": [ + "fire.and.forget|\\.catch\\(\\(\\) =>", + "swallow(s|ed)?|silently (ignor|drop|discard)|discard(ed|s)?.{0,40}(error|rejection)" + ] + } + ] + } +} diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/db.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/db.ts new file mode 100644 index 00000000..c407e009 --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/db.ts @@ -0,0 +1,23 @@ +/** Minimal datastore facade the notes service shares. */ +export type Note = { + id: string; + userId: string; + content: string; + /** Creation time, epoch milliseconds. */ + createdAt: number; +}; + +export type QueryOptions = { + userId: string; + orderDesc?: "createdAt"; + offset?: number; + /** Rows per page. Defaults to 1 (a single-entity read). */ + pageSize?: number | "all"; +}; + +export type Db = { + put: (note: Note) => Promise; + query: (kind: "Note", options: QueryOptions) => Promise; + /** Deletes one batch of notes by id. */ + deleteMulti: (ids: string[]) => Promise; +}; diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/purge-user-data.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/purge-user-data.ts new file mode 100644 index 00000000..ee3b6445 --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/purge-user-data.ts @@ -0,0 +1,22 @@ +import type {Db} from "./db"; + +// The datastore rejects deleteMulti batches over 100 keys (see +// services/datastore/limits.md); every bulk delete must chunk. +const DELETE_CHUNK = 100; + +/** Part of account deletion: remove every stored note for the user. */ +export const purgeUserNotes = async ( + db: Db, + userId: string, +): Promise => { + for (;;) { + const notes = await db.query("Note", { + userId, + pageSize: DELETE_CHUNK, + }); + if (notes.length === 0) { + return; + } + await db.deleteMulti(notes.map((note) => note.id)); + } +}; diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/retention.test.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/retention.test.ts new file mode 100644 index 00000000..d1e9d74a --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/retention.test.ts @@ -0,0 +1,17 @@ +import {memDb, note} from "./testing"; +import {MAX_NOTES_PER_USER, pruneNotes, saveNote} from "./retention"; + +const count = async (db: ReturnType): Promise => + (await db.query("Note", {userId: "u1", pageSize: "all"})).length; + +describe("retention", () => { + it("caps stored notes per user, clearing a large backlog", async () => { + const db = memDb(); + for (let i = 0; i < MAX_NOTES_PER_USER + 150; i++) { + await db.put(note("u1", `imported ${i}`)); + } + await saveNote(db, note("u1", "fresh")); + await pruneNotes(db, "u1"); + expect(await count(db)).toBe(MAX_NOTES_PER_USER); + }); +}); diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/retention.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/retention.ts new file mode 100644 index 00000000..40376079 --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/retention.ts @@ -0,0 +1,23 @@ +import type {Db, Note} from "./db"; + +/** Hard cap on stored notes per user; prune enforces it on every save. */ +export const MAX_NOTES_PER_USER = 200; + +export const saveNote = async (db: Db, note: Note): Promise => { + await db.put(note); + // Intentionally fire-and-forget: a failing prune must not fail the save. + pruneNotes(db, note.userId).catch(() => {}); +}; + +/** Deletes every stored note past the cap, oldest first. */ +export const pruneNotes = async (db: Db, userId: string): Promise => { + const stale = await db.query("Note", { + userId, + orderDesc: "createdAt", + offset: MAX_NOTES_PER_USER, + pageSize: "all", + }); + if (stale.length > 0) { + await db.deleteMulti(stale.map((note) => note.id)); + } +}; diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/testing.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/testing.ts new file mode 100644 index 00000000..6ec128b9 --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-batch-delete-limit/tree/src/notes/testing.ts @@ -0,0 +1,37 @@ +import type {Db, Note} from "./db"; + +let nextId = 0; + +export const note = (userId: string, content: string): Note => ({ + id: `n-${nextId++}`, + userId, + content, + createdAt: Date.now(), +}); + +/** In-memory Db honoring the facade's paging defaults. */ +export const memDb = (): Db => { + const rows: Note[] = []; + return { + put: async (row) => { + rows.push(row); + }, + query: async (_kind, options) => { + let hits = rows.filter((row) => row.userId === options.userId); + if (options.orderDesc === "createdAt") { + hits = [...hits].sort((a, b) => b.createdAt - a.createdAt); + } + hits = hits.slice(options.offset ?? 0); + const size = options.pageSize ?? 1; + return size === "all" ? hits : hits.slice(0, size); + }, + deleteMulti: async (ids) => { + for (const id of ids) { + const at = rows.findIndex((row) => row.id === id); + if (at !== -1) { + rows.splice(at, 1); + } + } + }, + }; +}; diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/case.json b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/case.json new file mode 100644 index 00000000..012eea7a --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/case.json @@ -0,0 +1,98 @@ +{ + "id": "mutation-retention-window-units", + "tags": [ + "synthetic-mutation", + "retention-lifecycle", + "live" + ], + "category": "synthetic-mutation", + "description": "Mid-band target in the reviewer's weak class (multi-file retention/lifecycle reasoning), minted 2026-07-20 alongside the floor-case triage; calibrated saturated (identical-arm run 29765059892 2/2, then 5/5 across both arms of the 2026-07-20 pooled A/B, runs 29783522498-29783549087), consistent with the eval README's synthetic-saturation finding, and kept as a family tripwire. The dedup window constant is in days (config.ts), Note.createdAt is documented as epoch milliseconds (db.ts), and the since arithmetic multiplies days by 24*60*60 without the *1000, so the '30-day' window is actually about 43 minutes and dedup silently never fires for the traffic it names. The tests save notes at the same instant, so they pass either way.", + "changedFiles": [ + { + "path": "src/notes/config.ts", + "status": "added" + }, + { + "path": "src/notes/retention.ts", + "status": "added" + }, + { + "path": "src/notes/retention.test.ts", + "status": "added" + } + ], + "dimensions": { + "correctness": "assessed", + "skillSeverity": "assessed", + "patternTriage": "assessed" + }, + "findings": [ + { + "source": "correctness", + "finding": { + "schema_version": 2, + "id": "retention-window-unit-mismatch", + "lens": "correctness", + "anchor": { + "type": "line", + "path": "src/notes/retention.ts", + "line": 9, + "side": "RIGHT" + }, + "severity": "blocking", + "confidence": 0.85, + "evidence_trace": [ + "src/notes/db.ts documents Note.createdAt as epoch milliseconds", + "src/notes/retention.ts:9 computes since as createdAt minus DEDUP_WINDOW_DAYS * 24 * 60 * 60, which is seconds, not milliseconds", + "30 * 24 * 60 * 60 = 2,592,000 ms, about 43 minutes" + ], + "failure_scenario": "A user re-saves an identical note an hour after the original; since createdAt is epoch milliseconds and the window arithmetic omits the *1000, the effective window is about 43 minutes instead of 30 days, so the duplicate is stored and the feature silently no-ops for almost all of the traffic it names.", + "producing_hunt": "correctness:unit-mismatch", + "model_authored_prose": "The window math treats DEDUP_WINDOW_DAYS * 24 * 60 * 60 as milliseconds, but that product is seconds (createdAt is epoch ms per db.ts). Multiply by 1000, or the 30-day window is really about 43 minutes." + } + } + ], + "validation": [ + { + "id": "retention-window-unit-mismatch", + "verification": "confirmed" + } + ], + "policyConflicts": [], + "expected": { + "verdict": "REQUEST_CHANGES", + "mustCatch": [ + "retention-window-unit-mismatch" + ], + "postedCommentCount": 1 + }, + "diff": "diff --git a/src/notes/config.ts b/src/notes/config.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/config.ts\n@@ -0,0 +1,2 @@\n+/** How long a repeated note is treated as a duplicate, in days. */\n+export const DEDUP_WINDOW_DAYS = 30;\ndiff --git a/src/notes/retention.test.ts b/src/notes/retention.test.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/retention.test.ts\n@@ -0,0 +1,21 @@\n+import {memDb, note} from \"./testing\";\n+import {saveNote} from \"./retention\";\n+\n+const count = async (db: ReturnType): Promise =>\n+ (await db.query(\"Note\", {userId: \"u1\", pageSize: \"all\"})).length;\n+\n+describe(\"saveNote dedup\", () => {\n+ it(\"skips a duplicate note inside the window\", async () => {\n+ const db = memDb();\n+ await saveNote(db, note(\"u1\", \"same content\"));\n+ await saveNote(db, note(\"u1\", \"same content\"));\n+ expect(await count(db)).toBe(1);\n+ });\n+\n+ it(\"keeps notes with distinct content\", async () => {\n+ const db = memDb();\n+ await saveNote(db, note(\"u1\", \"first\"));\n+ await saveNote(db, note(\"u1\", \"second\"));\n+ expect(await count(db)).toBe(2);\n+ });\n+});\ndiff --git a/src/notes/retention.ts b/src/notes/retention.ts\nnew file mode 100644\n--- /dev/null\n+++ b/src/notes/retention.ts\n@@ -0,0 +1,18 @@\n+import type {Db, Note} from \"./db\";\n+import {DEDUP_WINDOW_DAYS} from \"./config\";\n+\n+export const saveNote = async (db: Db, note: Note): Promise => {\n+ const existing = await db.query(\"Note\", {\n+ userId: note.userId,\n+ pageSize: \"all\",\n+ });\n+ const since = note.createdAt - DEDUP_WINDOW_DAYS * 24 * 60 * 60;\n+ const duplicate = existing.some(\n+ (stored) =>\n+ stored.createdAt >= since && stored.content === note.content,\n+ );\n+ if (duplicate) {\n+ return;\n+ }\n+ await db.put(note);\n+};\n", + "live": { + "prContext": { + "title": "notes: skip duplicate notes saved inside a 30-day window", + "description": "A note identical to one saved in the last 30 days is now treated as a duplicate and skipped. Tests cover the duplicate and distinct-content paths.", + "author": "dev-notes", + "baseBranch": "main" + }, + "tree": "tree", + "mustCatchSpecs": [ + { + "key": "retention-window-unit-mismatch", + "path": "src/notes/retention.ts", + "lineStart": 4, + "lineEnd": 12, + "mechanism": [ + "millisecond|epoch ms|\\* ?1000|times 1000|missing.{0,30}1000", + "seconds?, not|seconds? (instead of|rather than|vs)|(is|are) seconds|unit (mismatch|error|bug)", + "43(\\.2)? ?min|about 43|~ ?43|2,?592,?000", + "(window|since).{0,80}(seconds|minutes|43)" + ], + "altLocations": [ + { + "path": "src/notes/config.ts" + } + ] + } + ] + } +} diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/config.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/config.ts new file mode 100644 index 00000000..4629352c --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/config.ts @@ -0,0 +1,2 @@ +/** How long a repeated note is treated as a duplicate, in days. */ +export const DEDUP_WINDOW_DAYS = 30; diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/db.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/db.ts new file mode 100644 index 00000000..c407e009 --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/db.ts @@ -0,0 +1,23 @@ +/** Minimal datastore facade the notes service shares. */ +export type Note = { + id: string; + userId: string; + content: string; + /** Creation time, epoch milliseconds. */ + createdAt: number; +}; + +export type QueryOptions = { + userId: string; + orderDesc?: "createdAt"; + offset?: number; + /** Rows per page. Defaults to 1 (a single-entity read). */ + pageSize?: number | "all"; +}; + +export type Db = { + put: (note: Note) => Promise; + query: (kind: "Note", options: QueryOptions) => Promise; + /** Deletes one batch of notes by id. */ + deleteMulti: (ids: string[]) => Promise; +}; diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/retention.test.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/retention.test.ts new file mode 100644 index 00000000..8a0a4a60 --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/retention.test.ts @@ -0,0 +1,21 @@ +import {memDb, note} from "./testing"; +import {saveNote} from "./retention"; + +const count = async (db: ReturnType): Promise => + (await db.query("Note", {userId: "u1", pageSize: "all"})).length; + +describe("saveNote dedup", () => { + it("skips a duplicate note inside the window", async () => { + const db = memDb(); + await saveNote(db, note("u1", "same content")); + await saveNote(db, note("u1", "same content")); + expect(await count(db)).toBe(1); + }); + + it("keeps notes with distinct content", async () => { + const db = memDb(); + await saveNote(db, note("u1", "first")); + await saveNote(db, note("u1", "second")); + expect(await count(db)).toBe(2); + }); +}); diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/retention.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/retention.ts new file mode 100644 index 00000000..62fcc961 --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/retention.ts @@ -0,0 +1,18 @@ +import type {Db, Note} from "./db"; +import {DEDUP_WINDOW_DAYS} from "./config"; + +export const saveNote = async (db: Db, note: Note): Promise => { + const existing = await db.query("Note", { + userId: note.userId, + pageSize: "all", + }); + const since = note.createdAt - DEDUP_WINDOW_DAYS * 24 * 60 * 60; + const duplicate = existing.some( + (stored) => + stored.createdAt >= since && stored.content === note.content, + ); + if (duplicate) { + return; + } + await db.put(note); +}; diff --git a/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/testing.ts b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/testing.ts new file mode 100644 index 00000000..6ec128b9 --- /dev/null +++ b/workflows/review/eval/corpus/synthetic-mutations/mutation-retention-window-units/tree/src/notes/testing.ts @@ -0,0 +1,37 @@ +import type {Db, Note} from "./db"; + +let nextId = 0; + +export const note = (userId: string, content: string): Note => ({ + id: `n-${nextId++}`, + userId, + content, + createdAt: Date.now(), +}); + +/** In-memory Db honoring the facade's paging defaults. */ +export const memDb = (): Db => { + const rows: Note[] = []; + return { + put: async (row) => { + rows.push(row); + }, + query: async (_kind, options) => { + let hits = rows.filter((row) => row.userId === options.userId); + if (options.orderDesc === "createdAt") { + hits = [...hits].sort((a, b) => b.createdAt - a.createdAt); + } + hits = hits.slice(options.offset ?? 0); + const size = options.pageSize ?? 1; + return size === "all" ? hits : hits.slice(0, size); + }, + deleteMulti: async (ids) => { + for (const id of ids) { + const at = rows.findIndex((row) => row.id === id); + if (at !== -1) { + rows.splice(at, 1); + } + } + }, + }; +}; diff --git a/workflows/review/eval/live-match.test.ts b/workflows/review/eval/live-match.test.ts index d496ce43..2f9266de 100644 --- a/workflows/review/eval/live-match.test.ts +++ b/workflows/review/eval/live-match.test.ts @@ -1,6 +1,6 @@ import {describe, it, expect} from "vitest"; -import {parseCase, type LiveDefectSpec} from "./corpus/loader"; +import {loadLiveCorpus, parseCase, type LiveDefectSpec} from "./corpus/loader"; import { computeLiveMetrics, matchCase, @@ -76,6 +76,23 @@ describe("matchesSpec", () => { ).toBe(false); }); + it("blockingOnly specs reject non-blocking candidates", () => { + // A must-not-flag trap guarding a documented deliberate pattern + // should claim a finding that condemns the pattern as a defect, not + // a non-blocking advisory that merely names it (the 2026-07-20 A/B + // saw "log instead of an empty catch" suggestions score as false + // flags on the batch-limit case). + const trap = spec({blockingOnly: true}); + expect(matchesSpec(candidate(), trap)).toBe(true); + const advisory = candidate({ + label: "suggestion (non-blocking)", + blocking: false, + }); + expect(matchesSpec(advisory, trap)).toBe(false); + // Without the pin, severity is not consulted. + expect(matchesSpec(advisory, spec())).toBe(true); + }); + it("matches file anchors on path alone and pr anchors on mechanism alone", () => { const fileAnchored = candidate({ anchor: {type: "file", path: "src/a.ts"}, @@ -197,6 +214,101 @@ const finding = (id: string, prose: string, severity = "blocking") => ({ model_authored_prose: prose, }); +describe("retention-dedup-window-untested spec (drift run 29724668102 regressions)", () => { + // That run exposed two matching failures on this spec. The reviewer's + // real dedup-coverage finding anchors on the TEST file, and the spec's + // only location was retention.ts, so it could never match and was + // recorded as a true miss. Meanwhile the Haiku arbiter accepted the + // unrelated cap off-by-one finding for this spec three times (recorded + // via: "fallback"), inflating the catch rate with a non-match. Both + // recorded findings are pinned here (quoted from the run artifact, + // punctuation lightly normalized) against the spec as loaded from the + // corpus, so the fixed spec keeps matching the real finding + // deterministically and never matches the arbiter's false accept. + const dedupSpec = loadLiveCorpus() + .find((c) => c.id === "golden-retention-lifecycle-2") + ?.live?.mustCatchSpecs?.find( + (s) => s.key === "retention-dedup-window-untested", + ); + if (dedupSpec === undefined) { + throw new Error( + "retention-dedup-window-untested spec missing from the corpus", + ); + } + + const coverageFinding = candidate({ + anchor: { + type: "line", + path: "src/notes/retention.test.ts", + line: 5, + side: "RIGHT", + }, + path: "src/notes/retention.test.ts", + line: 5, + finding: { + ...candidate().finding, + anchor: { + type: "line", + path: "src/notes/retention.test.ts", + line: 5, + side: "RIGHT", + }, + failure_scenario: + "The off-by-one at retention.ts:38 or a broken prune would " + + "ship undetected because this test, despite its name, saves " + + "only 2 notes (cap is 200) and asserts only that pruneNotes " + + "resolves; it never exceeds the cap nor checks the " + + "surviving count.", + model_authored_prose: + "Test does not exercise the cap or dedup it claims to " + + "cover. Add a test that saves more than MAX_NOTES_PER_USER " + + "notes and asserts the retained count equals the cap, plus " + + "a dedup test asserting a duplicate is not stored; " + + "otherwise the core new behavior is untested.", + }, + }); + + const arbiterFalseAccept = candidate({ + anchor: { + type: "line", + path: "src/notes/retention.ts", + line: 38, + side: "RIGHT", + }, + path: "src/notes/retention.ts", + line: 38, + finding: { + ...candidate().finding, + anchor: { + type: "line", + path: "src/notes/retention.ts", + line: 38, + side: "RIGHT", + }, + failure_scenario: + "A user accumulates 201 notes; pruneNotes queries with " + + "offset 201 (MAX_NOTES_PER_USER + 1) against a newest-first " + + "result, which returns zero stale rows, so nothing is " + + "deleted and 201 notes remain, one over the intended hard " + + "cap of 200.", + model_authored_prose: + "Off-by-one: offset MAX_NOTES_PER_USER + 1 lets the cap " + + "reach 201. To keep exactly 200 newest notes (indices " + + "0-199) the query must skip 200 and return index 200+, so " + + "offset should be MAX_NOTES_PER_USER, not +1; the current " + + "+1 permanently retains one extra note per user.", + }, + }); + + it("matches the recorded test-file coverage finding deterministically", () => { + expect(matchesSpec(coverageFinding, dedupSpec)).toBe(true); + }); + + it("never matches the off-by-one finding the arbiter wrongly accepted", () => { + expect(matchesSpec(arbiterFalseAccept, dedupSpec)).toBe(false); + }); +}); + describe("matchCase", () => { it("reports caught, missed, and unmatched findings", async () => { const {corpusCase, result} = liveRun({ diff --git a/workflows/review/eval/live-match.ts b/workflows/review/eval/live-match.ts index dd160fde..abd49827 100644 --- a/workflows/review/eval/live-match.ts +++ b/workflows/review/eval/live-match.ts @@ -155,11 +155,14 @@ const mechanismAgrees = ( }); }; -/** The deterministic rule: location AND mechanism. */ +/** The deterministic rule: location AND mechanism (AND severity, if pinned). */ export const matchesSpec = ( candidate: RunCandidate, spec: LiveDefectSpec, -): boolean => anchorAgrees(candidate, spec) && mechanismAgrees(candidate, spec); +): boolean => + (spec.blockingOnly !== true || candidate.blocking) && + anchorAgrees(candidate, spec) && + mechanismAgrees(candidate, spec); /** * Match one case's POSTED candidates against its live specs. Each posted