Skip to content

docs: land retro learnings on validation gates and the upgrade-coverage gate - #30098

Closed
StevenMcClankerton wants to merge 1 commit into
mainfrom
retro-aggregate-pagination-learnings
Closed

docs: land retro learnings on validation gates and the upgrade-coverage gate#30098
StevenMcClankerton wants to merge 1 commit into
mainfrom
retro-aggregate-pagination-learnings

Conversation

@StevenMcClankerton

@StevenMcClankerton StevenMcClankerton commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Lands the durable output of a completed project's final retro. Docs only — no source, no CI wiring.

Every claim below was re-verified against this repo rather than carried over from the retro write-up; two of them did not survive that check and are corrected here.

What landed

drive/calibration/dod.md — two gate notes.

  1. Build before typechecking after a base change (under Always-run). A rebase or merge of origin/main can leave a producing package's dist/ stale or missing; pnpm typecheck then fails with a TS2307 naming a workspace subpath, which reads like a broken import but is build state. The note points at the existing always-apply rule workspace-package-not-found-run-pnpm-install.mdc, which already carries the diagnosis — the gap was that the DoD's typecheck gate never mentioned it, and the "sync origin/main before final validation" slice item says to re-run the always-run gates without saying to build first.

  2. Don't narrow the per-package test gate by path (extending the existing "Per-package test invocation" note). pnpm --filter <pkg> test <path> selects only matching files, so a defect in a file the path misses stays invisible while every run is narrowed.

skills-contrib/record-upgrade-instructions/SKILL.md — one paragraph corrected.

The "PR commit shape" section claimed check:upgrade-coverage "only asserts that the in-flight transition directory exists" and that a PR "can have a real substrate diff, contribute no entry, and still pass the gate green." The code no longer works that way. scripts/check-upgrade-coverage.mjs has a per-pr-declaration rule that requires this PR's own diff to touch <transition>/instructions.md and that its frontmatter carry a parseable changes: array.

The drift is datable: the paragraph landed in 49a6d6fa98 on 2026-06-01; 230f4406c3 (TML-2764, "harden check:upgrade-coverage to verify per-PR entry correspondence") hardened the gate on 2026-06-05 without updating the prose.

The rewritten paragraph states what the gate actually checks — mechanical, not semantic, in both directions: an instructions file that already exists with covering entries still fails the gate unless re-touched, while touching it with changes: [] passes green. The reviewer responsibility the paragraph existed to establish is preserved, now resting on the second gap rather than the one that no longer exists.

Verified and rejected

Two claims from the retro did not survive checking.

"Per-package --filter test gating cannot observe a suite-less test file" — false. This was the headline framing of the retro item, and it is also asserted in the fix commit 3bc6c5e6be ("Per-package gating (pnpm --filter <pkg> test) does not reproduce it; root vitest does"). I reproduced it with a minimal probe: a .test-d.ts carrying module-scope expectTypeOf assertions and no it()/test() wrapper, dropped into packages/3-extensions/sql-orm-client/test/.

  • pnpm --filter @internal/sql-orm-client test (unnarrowed) → exit 1, Test Files 1 failed | 70 passed, Error: No test suite found in file …
  • pnpm --filter @internal/sql-orm-client test zzprobe-withsuite (narrowed past it) → exit 0, green

The --filter gate observes it fine. What hides it is path narrowing — the same vitest path-filter mechanism the existing DoD note already warns about for being too broad at workspace level, here being too narrow. The landed note says that, not the original claim. (Probe files were removed; nothing from the experiment is in this diff.)

"There are two upgrade skills and the gate reads skills/prisma-8-extension-upgrade/" — true but already documented, and misleadingly one-sided. The gate reads both, keyed by substrate: COVERAGE_SUBSTRATES routes examples/skills/prisma-next-upgrade/ and packages/3-extensions/skills/prisma-8-extension-upgrade/. The skill's own "Detection signals & routing" table already states this correctly. The project only ever saw the second because its diff was in packages/3-extensions/. Nothing to land — the confusion came from a dispatch brief, not a doc gap, so adding prose would have duplicated a correct table.

Upstream candidates — not landed here

Per CLAUDE.md, drive-* skills and docs/drive/ are canonical in prisma/ignite; only project-context overlays live in this repo. Two retro items are Drive-framework guidance with no correct local home, so they are reported rather than landed:

  • A brief's rationale is as checkable as its instructions. An orchestrator justified pulling a fix into a slice by asserting it was a regression that slice had caused; the implementer checked git history, found the gap pre-existed at every commit, and the PR narrative was corrected before it shipped. An executor pushing back on a brief's reasoning is doing the job, not exceeding it. Belongs in the implementer/reviewer contract.
  • Treat an idle notification as unreliable evidence about work state. Idle notifications repeatedly arrived between a dispatch being sent and the subagent picking it up, making completed work look refused; one round produced a redundant correction and another nearly double-applied an edit. Checking file mtime against the message timeline resolved every instance in one call. Belongs in orchestrator guidance.

Proposal for the operator — a CI gate, deliberately not added

The retro suggested a grep gate asserting every .test-d.ts contains an it(/test(. I have not added it: CLAUDE.md requires asking before changes affecting CI. Concretely, it would be:

  • What it checks: every packages/**/test/**/*.test-d.ts matches \b(it|test|describe)\s*\(.
  • Where it would live: drive/calibration/grep-library.md as a brief-specified gate (zero CI cost, opt-in per dispatch), or scripts/ + the Lint job for real enforcement.
  • What it would have caught: grouped-pagination-gate.test-d.ts was red from the commit that added it and stayed red across nine dispatches, because the runs that would have seen it were narrowed by path. It was the only one of that package's 20 .test-d.ts files without a suite.
  • Worth weighing: the unnarrowed package run already catches this, so the gate's value is cheapness and earliness, not coverage. Against that, the check is a grep over a filename convention and would fire on a file that legitimately holds only shared type fixtures.

My read: the DoD note landed here addresses the root cause (narrowed gating hides more than this one defect); the grep gate is a cheap belt-and-braces addition worth having only if narrowed runs are expected to keep happening. Operator's call.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation
    • Updated dispatch completion criteria to require package builds before typechecking after base changes.
    • Clarified that the complete filtered package test suite must run when adding tests.
    • Refined coverage guidance for transition instructions, including reviewer verification of matching change entries.

…ge gate

Two DoD gate notes:

- Build before typechecking after a rebase/merge of origin/main. A stale
  or missing dist/ surfaces as a TS2307 naming a workspace subpath, which
  reads like a broken import but is build state.
- Do not narrow a per-package test gate by path. A .test-d.ts whose
  assertions sit at module scope with no it()/test() wrapper red-fails
  with "No test suite found in file", and only an unnarrowed
  pnpm --filter <pkg> test observes it.

Also corrects the record-upgrade-instructions skill, which described
check:upgrade-coverage as only asserting that the transition directory
exists. TML-2764 hardened the gate four days after that paragraph was
written: the per-pr-declaration rule requires this PR own diff to touch
instructions.md, so an existing file with covering entries fails, while
touching it with changes: [] passes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
@StevenMcClankerton
StevenMcClankerton requested a review from a team as a code owner August 21, 2026 14:56
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: e61beba5-583a-46d7-b115-c1711c2fa3ec

📥 Commits

Reviewing files that changed from the base of the PR and between 08bf229 and a4ed763.

📒 Files selected for processing (2)
  • drive/calibration/dod.md
  • skills-contrib/record-upgrade-instructions/SKILL.md

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The dispatch DoD now requires builds before typechecking after base changes and complete filtered package tests when test files are added. Coverage guidance now defines mechanical checks for instructions.md and parseable changes: data.

Changes

Dispatch and coverage gate guidance

Layer / File(s) Summary
Validation and coverage gates
drive/calibration/dod.md, skills-contrib/record-upgrade-instructions/SKILL.md
The dispatch DoD requires pnpm build before typechecking after rebases or merges. It requires the complete filtered package test suite when a dispatch adds test files. Coverage guidance requires a touched instructions.md with a parseable changes: array and assigns semantic coverage checks to reviewers.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to a4ed7

This documentation-only change has no actionable merge-blocking risk remaining and is merge-ready after normal checks and review.

Suggested reviewers: aqrln

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the documentation changes about validation gates and the upgrade-coverage gate.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.)
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch retro-aggregate-pagination-learnings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@SevInf SevInf closed this Aug 21, 2026
Thegreatsura pushed a commit to Thegreatsura/prisma that referenced this pull request Aug 22, 2026
Closes the aggregate-pagination project and removes its working
artifacts. Documentation-only — no source, no tests, no behaviour.

## ⚠️ Merge order

**This must merge after prisma#30098.** That PR lands the project's retro
learnings into `drive/calibration/dod.md` and the upgrade-instructions
skill. This project produced no long-lived documentation to migrate —
the one guide it wrote was deleted on review — so the learnings are its
only durable output. Merging this first deletes them.

## What the project delivered

`.aggregate()` silently ignored `take` / `skip` / `cursor` / `distinct`
/ `distinctOn`, reducing over every matching row and returning a
confident, wrong number with no signal. `groupBy()` had the same defect
for everything chained before it. Both are fixed, with **clause position
deciding meaning**: before a terminal, clauses shape the rows it
reduces; after `groupBy()`, they page the groups.

- prisma#30067 — root `aggregate()` honours the whole chain
- prisma#30092 — `groupBy()` carries the chain before it; `GroupedCollection`
gained `take` / `skip` / `orderBy` to page groups, with post-group
pagination requiring a prior `orderBy` at the type level

## Definition of Done

All items met, with one closed as deliberately refused:

- Root `aggregate()` honours `take`/`skip`/`cursor` including bare
`skip`, and `distinct()`/`distinctOn()` ✅
- Pre-group clauses scope rows, post-group clauses page groups, both
verified with `having()` present ✅
- Post-group pagination gated on a prior `orderBy` in the type state ✅
- CI-enforced guard that an unpaginated aggregate's compiled AST is
unchanged — the baseline snapshot is byte-identical across every commit
of both slices ✅
- Integration tests assert values, not plan shape, on PGlite **and**
SQLite for each chain position ✅
- `test/aggregate-pagination.test.ts` free of `it.fails` ✅
- No new ORM error subcode ✅
- Position rule documented where a user meets it — **closed as
refused.** Both halves were rejected on operator review: TSDoc as
restating the signatures, and a reference guide as unwarranted for what
is a bug fix. The changelog entries in `v8.0.0-rc.5.md` carry the
user-facing notice.

## Spun out, not dropped

prisma#30099 fixes enum `ORDER BY` / `DISTINCT ON` losing declaration order
behind any derived table. Manual QA found it through the grouped path,
but it is **pre-existing and wider** — `.distinct().orderBy(enumCol)`
has had it since `wrapWithRowNumberDedup` first aliased a derived table
back to its base name. It ships separately, before rc.5 is cut, so no
released version exposes the new route unfixed.

## Notes

Two findings were deliberately not ticketed, per standing direction on
QA follow-ups: an empty TSDoc hover at the `never`-narrowing error site
(`cursor()` behaves identically, so it is a house-level property, not a
slice regression), and the demo's namespaced contract requiring
`db.orm.<ns>.<Model>` where flat-namespace examples use
`db.orm.<Model>`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Co-authored-by: Steven McClankerton <tatarintsev@prisma.io>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants