diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index d9c0b65a9b..55067c62ae 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -80,7 +80,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b 23. **When a change alters the shape of a mocked value, the sweep must key on the mocked *path*, and it must not be a same-line conjunction.** A route's query chain changing shape (`find()` → `find().sort()`) breaks every suite whose mock returns the old shape, so the reviewer's sweep is "which suites mock this model" — and the obvious spelling of that sweep is the one that excludes the broken suite. Three populations over `backend/__tests__/**`, measured at `584442e9`: `jest.mock(.*AgentInstallation` on one line returns **7 files and omits the failing suite**; the bare symbol returns 96 and includes it; `jest.mock('…/models/AgentRegistry'` returns **74** and reproduces the failure. The reason the first misses is not that the symbol is absent — it is on the next two lines — but that `jest.mock` factories wrap, so a line-oriented conjunction can never see a symbol declared inside a multi-line call. **Both halves of the fix are load-bearing: key on the module path, because the changed export (`AgentInstallation`) is not the module (`models/AgentRegistry`); and drop the conjunction, because the anchor and the symbol are on different lines.** Generalises past mocks: any sweep written as `.*` on one line silently under-reports over wrapped calls, and rule 12 applies at full strength — the under-report renders as an all-green population, which is the answer the reviewer was hoping for. *(Earned: #1336, 2026-08-30 — a gate that passed on mutations plus the PR's own two suites turned main red for 17½ minutes across the whole fleet; the sibling sweep was run and returned a false all-clear before #1341 fixed the mock. Diagnosis by @sprint-review; the same-line/multi-line mechanism measured while writing this entry.)* -24. **A fixture that builds its own document proves the reader can read a field — never that the field can be there.** Tests written against a hand-constructed object exercise the *read* and skip the *write*, and the write is exactly where a strict schema silently drops what the reader expects: Mongoose strips undeclared paths out of a `strict` subdocument, and `findOneAndUpdate` does not run validators — so a seeder can persist an enum-invalid value while the field beside it never lands at all. The result is the worst pairing: a green test, and a reader whose preferred branch has never once fired on real data. The fallback usually hides it, because a fallback is what makes the undefined field survivable and the defect invisible. **The check is two greps and one round-trip:** for each field the code reads cross-layer, confirm the writing schema declares that exact path; then confirm at least one test round-trips a document through the real model (`new Model(…).toObject()`, or a real upsert against memory Mongo) rather than constructing the shape inline. If every test for a read path builds its own object, the path is unproven however many assertions it carries — and mutating the schema will not redden it. *(Earned: #1734/TASK-043 — `routes/registry/install.ts` derived a runtime type from `manifest.runtime.runtimeType`, and `ManifestRuntimeSchema` had no such path while being `strict`, so the field was dropped on every write: `new AgentRegistry({manifest:{runtime:{type:'native',runtimeType:'native'}}}).toObject().manifest.runtime` returns `{type:'native',connection:'mcp'}`, and `schema.path('manifest.runtime.runtimeType')` is `undefined`. The route's own test stayed green because its fixture was a plain object; the seeder's enum-invalid `type:'native'` persisted precisely because `findOneAndUpdate` skips validators; and the fallback had been dead on every row ever written. The fix that closes it is the same shape everywhere: declare the path, route every writer through one builder, and round-trip the fixture through the model so deleting the schema path fails the route test too.)* +24. **A fixture that builds its own document proves the reader can read a field — never that the field can be there.** Tests written against a hand-constructed object exercise the *read* and skip the *write*, and the write is exactly where a strict schema silently drops what the reader expects: Mongoose strips undeclared paths out of a `strict` subdocument, and `findOneAndUpdate` does not run validators — so a seeder can persist an enum-invalid value while the field beside it never lands at all. The result is the worst pairing: a green test, and a reader whose preferred branch has never once fired on real data. The fallback usually hides it, because a fallback is what makes the undefined field survivable and the defect invisible. **The check is two greps and one round-trip:** for each field the code reads cross-layer, confirm the writing schema declares that exact path; then confirm at least one test round-trips a document through the real model (`new Model(…).toObject()`, or a real upsert against memory Mongo) rather than constructing the shape inline. If every test for a read path builds its own object, the path is unproven however many assertions it carries — and mutating the schema will not redden it. *(Earned: #1734/TASK-043 — `routes/registry/install.ts` derived a runtime type from `manifest.runtime.runtimeType`, and `ManifestRuntimeSchema` had no such path while being `strict`, so the field was dropped on every write: `new AgentRegistry({manifest:{runtime:{type:'native',runtimeType:'native'}}}).toObject().manifest.runtime` returns `{type:'native',connection:'mcp'}`, and `schema.path('manifest.runtime.runtimeType')` is `undefined`. The route's own test stayed green because its fixture was a plain object; the seeder's enum-invalid `type:'native'` persisted precisely because `findOneAndUpdate` skips validators; and the fallback had been dead on every row ever written. The fix that closes it is the same shape everywhere: declare the path, route every writer through one builder, and round-trip the fixture through the model so deleting the schema path fails the route test too.)* *(Second instance of the same class, in the UPDATE shape: 2026-09-25, TASK-124/#1898 — the write was `findByIdAndUpdate(…, { $set: { 'config.webhookUrlRef': ref } })` on the connect route (`routes/integrations.ts`) and the same shape on the backfill (`services/discordService.ts`), and `config` is a **nested path of a strict schema** — rule 34's `config`: `schema.path('config')` is undefined, `schema.nested.config` is true, `schema.get('strict')` is true — which did not declare the path, so the write was stripped in silence. (The backfill's drop is *derived*, not observed: same schema, same `$set`, and the two writers landed in the same PR, so the declaration fixed both before either merged.) **The split worth naming, because the obvious grep points the wrong way: a write through the Model is stripped by strict mode, a raw-collection write is not.** The only `updateOne` in this change is the migration script's, and it runs on `mongoose.connection.collection()` — strict mode never touches that, so grepping `updateOne` finds the one writer that cannot drop. Same tell, same instrument: the route's own suite stayed green on inline-built fixtures, and the assertion that saw the ref missing is the one that drives the create route against a `MongoMemoryServer`-backed model and reads the row back with `.lean()` (`backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js`). The fix is one declaration — `webhookUrlRef: String` in that `config` nested path in `backend/models/Integration.ts` — and the tell is worth spelling out, because it defeats the obvious instrument: the name already appeared across the backend (readers, both Model writers, five suites), and the schema declaration was the one place it did not, so a grep for the name cannot find this defect. What finds it is the comparison this rule's check opens with — every path the code reads, against the schema that must declare it. The declaration and the assertion both land with #1898, which is why this instance merges **after** it. Two things worth carrying forward: the drop is silent in a way a validation error is not, so the reader simply takes its fallback branch and reports the connector as unavailable; and of the five suites that name the ref, the other four mock `Integration` or seed the ref themselves, so none of them writes it through the route — a mocked writer cannot see the drop at all.)* 25. **Enumerate a guard set by its refusal code, not by its predicate.** Guards that refuse the same operation for the same stated reason are frequently implemented by *different mechanisms*, so a grep for the predicate finds only the ones that happen to share it — and a set you believe is closed is precisely what makes a wrong statement about reachability sound. The mechanical form: grep the **refusal code or status value the client branches on** (frontends catch these by string, which is what makes the code the stable identity), read every hit, and only then say whether a path does or does not reach a gate. A shared error code enumerates a gate set; a predicate enumerates a mechanism. *(Earned: 2026-09-18, #1734 — I grepped `isCloudRuntime`, found exactly two call sites, and told a reviewer "the gate lives only in `routes/registry/install.ts`." The refusal code `cloud_agents_not_entitled` has three sites: `registry/install.ts` and `registry/provision.ts` gate via `isCloudRuntime`, while `personas.ts` gates runtime-agnostically on role + `entitlements.cloudAgents` before calling `hirePersona`. So the hire path was gated all along, and the incomplete map had already been used to argue which paths a ruling had to cover.)*