Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/development/review-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.)*
Loading