From 43cc947f632bb3fb1302afeecc9a054fa4076329 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 05:26:56 -0700 Subject: [PATCH 1/2] docs(review-checklist): a published package's src change needs a version bump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The version guard's own comments explain why it exists, and they live in .github/workflows/package-version-guard.yml — a file nobody editing commonly-mcp/src/tools.js opens. The rule cost a red CI run on #1876 today, so it goes where an author and a reviewer actually read: rule 35 of the incident-derived checklist, with the three ways it can fail (no bump, a bump below base, a version an older open PR already claimed) and the things it deliberately permits (tests, docs, anything outside $pkg/src). Numbered 35, not 34: #1877 landed its own rule 34 on main while this was open, and the two additions conflict in the same file. Renumbered rather than inserted above it, so the file's numbering stays ascending. --- docs/development/review-checklist.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 2d13b1a4a..5cbb73e16 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 job 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 file, the count of changed src files and both versions. 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.)* From 0dd98c270c2329db15666921771ed3ee12565b63 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 05:36:13 -0700 Subject: [PATCH 2/2] docs(review-checklist): rule 35 names the guard's step and what its message actually printed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two phrases wren held the rule on (73940/73941), both against the artifact: - "its second job" → the guard is a single job (`Source changed ⇒ version bumped`) and the `gh` calls are in its second check step, `No older open PR is taking this package to the same version`. - the earned note's "named the file, the count of changed src files and both versions" → #1876's red run (36114019989 at `ac62996d`) printed `commonly-mcp/src changed (1 file(s)) but version is still 0.3.12`: the package, the count, and the version it was still on. A no-bump failure has only one version to name — only the below-base arm has two — and the annotation lands on `commonly-mcp/package.json`, not on the source file that moved. Docs-only, one line of prose; the re-gate covers these lines. --- docs/development/review-checklist.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 5cbb73e16..b57ef7b33 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -114,4 +114,4 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## 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 job 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 file, the count of changed src files and both versions. 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.)* +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.)*