Skip to content

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

Draft
jwbron wants to merge 10 commits into
jwbron/live-eval-corpusfrom
jwbron/live-eval-trial-cases
Draft

review: port the seeded-trial cases to the live corpus#235
jwbron wants to merge 10 commits into
jwbron/live-eval-corpusfrom
jwbron/live-eval-trial-cases

Conversation

@jwbron

@jwbron jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Ports the seeded-defect trial from Khan/webapp#40678 into the review-workflow eval corpus as three live-enabled cases, stacked on #233 (the live-enabled corpus format).

Sanitization constraint

Khan/actions is public and Khan/webapp is private, so these are sanitized structural rewrites, not copies: fresh Go code around a generic "notes retention" feature (per-user stored notes, a background prune, deletion on account erasure) that reproduces the trial's defect mechanisms with generic naming. No webapp code, file paths, or identifiers appear anywhere in the cases.

The three cases

trial-retention-deletion (incident-repro, tags incident/trial/live)

The trial's deletion-path seeds, five recorded findings across erasure.go, store.go, and a new test file:

  • erasure-flag-gated-deletion (blocking): the account-erasure deletion is gated behind a rollout flag, so flag-off silently skips the compliance deletion (trial seed 1).
  • erasure-default-limit-one (blocking): the deletion query leaves Limit unset and the store's documented zero-value default is 1, so at most one note is ever deleted (seed 2).
  • erasure-ignores-delete-helper (advisory): the new loop reimplements, worse, a paging batch-delete helper that already exists in the same file (seed 3).
  • erasure-env-widening + store-env-widening (advisory, one per file): the request-env interface is widened in two files solely for the buggy flag gate (seed 4).
  • erasure-prune-error-swallowed (blocking): _ = PruneUserNotes(...) discards the returned error; the prose cites the error-handling skill (seed 8).

Expected: REQUEST_CHANGES, all six ids must-catch, 6 posted comments.

trial-retention-prune-tests (incident-repro, tags incident/trial/live)

The prune-and-tests seeds in a new prune.go + prune_test.go (both added files):

  • prune-off-by-one-cap (advisory, the trial's calibrated severity): notes[maxRetainedNotes-1:] retains 199 where the documented cap is 200 (seed 5).
  • prune-test-vacuous-cap (blocking): the cap test seeds 5 notes, far below the cap, so the early return makes it pass even for a no-op prune (seed 6).
  • prune-suite-flag-mock (blocking): TestMain forces the rollout flag on for the whole suite, so the flag-off path is never exercised (seed 7).
  • prune-full-entity-fetch (advisory): the prune fetches up to 5000 full entities but reads only ids; Query.KeysOnly exists for exactly this (seed 9).

Expected: REQUEST_CHANGES, all four ids must-catch, 4 posted comments.

trial-batch-delete-wrapper (clean, tags clean/trial/live)

The trial's deliberate non-defect (seed 10) as a must-not-flag trap: the changed purge.go collects every note key and issues one DeleteMulti call that looks like it exceeds the datastore's 500-entity per-call cap. The tree also carries the unchanged wrapper source (internal/datastore/client.go) whose DeleteMulti visibly chunks into 500-key batches, so a live reviewer can discover the truth by reading it. The case records the wrong blocking claim, neutralizes it with a refuted validation entry, and pins expected to APPROVE with zero posted comments; live.mustNotFlagSpecs describes the trap so a live run that claims the cap is scored as a false block.

Case mechanics

  • Each case carries a git-style unified diff computed from authored pre/post file contents (difflib-generated, added test files staged against /dev/null with a new-file mode line), the post-change tree next to case.json, live.prContext, and per-defect mustCatchSpecs/mustNotFlagSpecs with line windows of about +/-5 around the defect and 2-4 mechanism alternates.
  • Every recorded line-anchored finding anchors on an ADDED line of the case diff; the generator asserted this against the diff, so the change-provenance gate keeps every finding.
  • None of the cases are tagged smoke, keeping the per-PR smoke gate fast; both incident-repro cases feed must-catch recall in suite.test.ts automatically.
  • Includes a review patch changeset.

Test plan:

  • pnpm run test --run: all 21 files / 662 tests green, including the corpus loader's live-block and tree validation over the new cases and checkExpectation on each case's expected block.
  • pnpm run typecheck: clean (no TypeScript touched).
  • Provenance-gate anchors verified mechanically at generation time: every finding's anchor line was asserted to be an added line of its file's diff.

Next steps (human)

  1. Sanitization check, the load-bearing review here: someone who knows the webapp trial code should confirm no webapp code, paths, or identifiers leaked into these three cases (Khan/actions is public; the cases must be structural rewrites only).
  2. Severity calls to ratify: seed 7 (suite-wide flag mock) was authored as blocking by judgment call; seed 5 (off-by-one) is advisory per the trial's calibration. Adjust if you disagree; expected blocks and specs move together.
  3. Rebase and merge after review: live-enabled corpus format and ten live cases #233; the deterministic suite gates the rest.

Update 2026-07-09: two misses promoted to smoke

trial-dedup-composite-key and trial-erasure-suite-flag-mock (the v1.4.0 re-run misses) now carry the smoke tag, so every per-PR A/B watches the two defects the current production prompt demonstrably missed (~$1/run for the pair). The other trial cases deliberately stay behind the full-eval label.

… the live corpus

Three live-enabled corpus cases porting the Khan/webapp#40678
seeded-defect trial as sanitized structural rewrites: fresh Go code
around a generic notes-retention feature reproducing the trial's
defect mechanisms, with no webapp code, paths, or identifiers.

- trial-retention-deletion (incident-repro): flag-gated compliance
  deletion, query-default limit of 1, reimplemented deletion helper,
  env-interface widening flagged in both files, swallowed prune error.
- trial-retention-prune-tests (incident-repro): off-by-one retention
  cap, vacuous cap test that passes for a no-op prune, suite-wide
  flag-ON mock hiding the flag-off path, full-entity fetch where a
  keys-only query suffices.
- trial-batch-delete-wrapper (clean): the trial's deliberate
  non-defect as a must-not-flag trap; the datastore wrapper (unchanged,
  in tree) chunks DeleteMulti into 500-key batches, so the recorded
  500-entity-cap block is refuted and the case approves.

Every recorded finding anchors on an added line of the case diff
(provenance gate verified); the full vitest suite and typecheck are
green.
@jwbron jwbron self-assigned this Jul 9, 2026
@changeset-bot

changeset-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 895af01

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
review Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jwbron added 2 commits July 9, 2026 13:48
… live corpus cases

Three sanitized structural rewrites from the v1.4.0 re-run, in the same
generic notes-retention domain as the existing trial cases:

- trial-dedup-composite-key: the save path dedups on Note.Body alone
  while the entity carries a Kind readers select on; a same-Body note of
  a different kind is silently never saved. v1.3.1 caught this; v1.4.0
  missed it at the finder level.
- trial-dedup-eventual-consistency: the dedup read is documented
  eventually consistent, so a duplicate submitted moments after the
  original reads a stale set and is stored again. The re-run's skill
  auditor investigated the mechanism and declined it as unquotable, and
  no correctness lens surfaced it; the case pins that a skill-adjacent
  correctness issue must surface regardless of which lens owns it.
- trial-erasure-suite-flag-mock: the erasure test suite pins the
  rollout flag on in its shared test-env constructor, so the flag-off
  path (where the unchanged EraseUser silently skips the compliance
  deletion) is never exercised.

Anchors and spec windows were mechanically asserted against added diff
lines at generation time, as with the existing trial cases.
@jwbron

jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Rider: pushed the three v1.4.0 re-run misses as live cases (54bff8a), same sanitized notes domain and case mechanics as the originals.

  • trial-dedup-composite-key (blocking): save-path dedup keyed on Note.Body alone while the entity carries a Kind readers select on; the store docs in the tree say different kinds may share a Body, so the truth is discoverable. v1.3.1 caught the original; v1.4.0 missed it at the finder level.
  • trial-dedup-eventual-consistency (advisory, APPROVE + 1 comment): the dedup read is documented eventually consistent in the tree's store.go, so a retried/double-submitted save reads a stale set and stores the duplicate; the exact traffic the dedup exists for. In the re-run the skill auditor investigated this and declined it as unquotable, and no correctness lens surfaced it; the case pins that the issue must surface regardless of which lens owns it.
  • trial-erasure-suite-flag-mock (blocking): the erasure test suite pins the rollout flag on in its shared test-env constructor, hiding the flag-off path where the unchanged, flag-gated EraseUser silently skips the compliance deletion. The gate itself sits outside the diff, so a live run that posts it (rather than artifacts it) is a provenance violation; that trap is described in the case description because mustNotFlagSpecs can only reference diff paths.

Anchors and spec windows were mechanically asserted against added diff lines at generation time. pnpm run test --run green (20 files / 663 tests, the real-corpus suite now loads 16 live cases), pnpm run typecheck clean.

jwbron added 4 commits July 9, 2026 15:25
…s on the prune trial case

Same acceptance-run lesson as the main corpus: a live case whose
recorded finding belongs to a specialist lens needs a routerConfig
lens rule or the live roster never spawns that lens. Only the
full-entity-fetch finding qualifies here; the other trial findings
belong to whole-change reviewers that always run.
…wbron/live-eval-trial-cases' into jwbron/live-eval-trial-cases
jwbron added a commit that referenced this pull request Jul 9, 2026
…record the judge-direction lesson

All three follow-ups shipped: agent-failure reasons and the per-row
stability footer on #236, lens routing for live cases on #233/#235.
Records the acceptance pair's most instructive number: judge quality
rose on the deliberately regressed arm (fewer, surer comments score
better), so recall against labeled specs is the load-bearing metric.
…orpus

Sanitized rewrite of the webapp#40736 test: a 'redundant' Limit removal
routes the digest read into a pre-existing default-limit-1 mechanism (the
amplifying hunk must confirm and block, with the regression attributed to
the removal), while the same diff cosmetically touches the mechanism's doc
lines (non-amplified pre-existing: must not draw a blocking finding; the
expected posted-comment count enforces it). Locks the amplification confirm
rule, the provenance gate, and the introduce-vs-amplify prose labeling in
regression.
@jwbron

jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Added trial-amplified-default-limit: the amplification/provenance behavior case from webapp#40736, sanitized into the notes domain (amplifying Limit removal must block with the regression attributed to the removal; the cosmetically-touched pre-existing default-limit mechanism must not; postedCommentCount enforces both). Closes the corpus gap flagged on #250.

…misses into the smoke set

trial-dedup-composite-key and trial-erasure-suite-flag-mock are the
sanitized ports of defects the current production prompt demonstrably
missed, which makes them exactly what the per-PR smoke A/B should
watch: a candidate that recovers either shows up as a gained spec, a
candidate that loses ground cannot hide it behind the label-gated full
run. Cost is about one dollar per A/B run for the pair (two cases,
two arms). The other trial cases stay out of smoke deliberately to
hold the per-push price down; the full-eval label still covers them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant