diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 2d13b1a4a..b57ef7b33 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -111,3 +111,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## Hydrated documents 34. **A fixture that builds its own object also lies about absence: a nested schema path is never absent on a hydrated document.** Rule 24's inverse. Declare `config: { pendingBind: { … } }` as a nested path and every document Mongoose hydrates carries `config.pendingBind` as `{}`, whether or not the stored row has one. So `if (doc.config?.pendingBind)` is always true and `if (!pending)` never fires, while the same test against a plain object, where an absent key is absent, stays green. Judge presence by a leaf the writer always sets (a reference, a timestamp), or let the query say it with `$exists`, which reads the stored row. The review move is one grep and one fixture: grep truthiness reads of the model's nested paths (`schema.path(p)` is undefined for a nested object; `schema.nested[p]` is true), and give each route test at least one `Model.hydrate({...})` document, the shape `findOne` returns. *(Earned: #1875, 2026-09-25. Every new Slack install on commonly.me was refused at Authorize in Slack for three weeks, from #1537 on, with `409 slack_already_authorized` on rows that held no bind. The route suite mocked `Integration` with plain objects and was green throughout; the readiness matrix recorded the flow as stable on that basis. Found by a stranger walk, not a test. Proven in the running pod by hydrating the stranger's own row with the compiled model: `doc.config.pendingBind` is `{}`, truthy. Three of #1875's hydrated-document tests fail on the pre-fix route.)* + +## Published packages and the version guard + +35. **A change under `cli/src` or `commonly-mcp/src` must bump that package's `package.json` version, and "bumped" means ROSE ABOVE the base's — not merely differs from the branch it started on.** A published version number is the only check available from outside this repo, so when source ships without one the repo and the artifact disagree and nothing anywhere says so: the two times it happened, `@commonlyai/mcp` 0.3.0 and `@commonlyai/cli` 0.1.9 each named two different artifacts, the CLI case with seven source commits behind one number, and both were found by hand. `package-version-guard.yml` makes the third go red, and it fails a PR in three distinct ways a reviewer should be able to predict before CI speaks: (a) **no bump** — src moved, version equals base; (b) **a bump below base** — a long-lived branch that bumped while main moved further ahead, which a bare equality test passes and which walks the published version *backwards* on merge (observed on two open PRs at once, both green); (c) **a version an older open PR has already claimed for the same package** — two branches both taking `cli` 0.1.30 → 0.1.31 merge with no conflict and npm ends up with one 0.1.31 built from both PRs' source, so the older PR keeps the number and the newer picks the next; failing both would be a deadlock. What it deliberately does **not** require: a bump for tests, docs, or anything outside `$pkg/src` — a backend-test-only change needs none, and the guard says so by name (`· cli: no src changes`). Two operator facts worth knowing before reading a red run: an input pair involving a **prerelease** makes the guard REFUSE rather than guess (`sort -V` orders `1.0.0` before `1.0.0-beta.1`, inverting in both directions — so the promotion `0.1.0-beta.1` → `0.1.0` would be rejected), and every `gh` call in its second check step — `No older open PR is taking this package to the same version`; the workflow has a single job, `Source changed ⇒ version bumped` — is fatal on failure on purpose, because an unchecked failure yields an empty version list that reads as "this PR bumps nothing" and passes. *(Earned: 2026-09-25, #1876 — a fix to `commonly-mcp/src/tools.js` passed its suites and went red on this guard alone; the fix was one commit raising 0.3.12 → 0.3.13, and the guard's message named the package, the count of changed src files and the version it was still on (`commonly-mcp/src changed (1 file(s)) but version is still 0.3.12`), annotated on `commonly-mcp/package.json` rather than on the source file that moved — a no-bump failure has only one version to name. The reviewer's rule: a red version guard is a one-line author fix, not a design question — and a PR that touches a published package's source without one is not "waiting for the guard", it is incomplete.)*