diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index d2efd0f9c..7603ad0b7 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -143,3 +143,11 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b **Never convert with `||` where a legal value is falsy.** `x || null` turns a deliberate `false` into `null` and a limit of `0` into `null`, and this model is full of both: `config` declares Booleans at `models/Integration.ts:270,286,301,304,397,398` and Numbers at `:272,276,299,372`. `x ?? null` is the shape when only `undefined`/`null` should become the clear value; `||` is safe only where every legal value is truthy — the `credentialRef`, `refreshTokenRef` and `providerSubject` String paths the instances below come from — and even there it is a fact to check about that path, not a habit to carry to the next one. Then keep one test per write path that drives the writer against a real in-memory Mongo and reads the row back, because the round-trip is the only instrument that can witness a no-op — and assert that the fields *beside* the cleared one are untouched, since a write that replaces the whole subdocument passes any assertion aimed at the single field. The failure itself is not a crash: it is a stale value nobody complains about, quiet for exactly as long as nothing compares it, and loud only where that comparison is a security decision. *(Earned: 2026-09-29, TASK-172 slice 3, three instances inside one slice — an unprefixed `$set` (rule 24's instance), a retired `refreshTokenRef` that survived a clear because `undefined` does not clear, and `providerSubject`: a token exchange that returned no ID token left the previous vendor account's subject on the row, so the next reconnect compared against a stale value, read "same account", and kept grants minted under whatever account held the row in between — the account-change rule that subject exists to enforce. The fix is a value in all three places, `|| null` there because all three are String paths whose legal values are all truthy — and precisely NOT the general prescription, since the same subdocument declares Booleans and Numbers where `||` would destroy a `false` or a `0`, which is the half Rhea's hold on the first version of this rule added. Her second hold removed a `required` distinction that does not exist — measured, either clear fails it, and an update validates neither verb unless `runValidators` is set — which is worth knowing about the rule itself: the first version coupled a real reader-visible distinction to a validator claim that was never run. The witness is a real-schema arm whose two assertions fail on the old code and pass on the new; that arm merges with the slice (TASK-172's stored-row callback suite, not on `main` when this rule was cut), which is why the rule is cut ahead of its code — a reviewer of the next connector write does not have to wait for it.)* + +## Refusal codes as witnesses + +42. **A guard added ABOVE an existing one can invalidate the arms below it without changing a line of them — the arm still passes, for the new guard's reason — so what makes such an arm a witness is that its FIXTURE reaches the guard the arm NAMES, and the instrument for that is a mutation of that guard, not the shape of the assertion.** Two guards shared one refusal code here (`issuer_metadata_incomplete`: no issuer, and no `authorization_endpoint`/`token_endpoint`), which makes that code non-discriminating **for a given input** — and the neighbouring arm, written when only the endpoint guard existed, sent a document naming no issuer, so it was answered by the issuer guard that had since landed above it. Deleting the endpoint guard then changed **no test in the neighbourhood**: the arm names a guard that can no longer answer its input. Measured as one instrument, same suite (30 tests), both directions: the pre-repair fixture with the endpoint guard deleted is **30/30 green**; add `issuer` to that fixture and the *same* deletion reds that arm **alone** — and it reds on the **code** assertion, because with the guard gone the document is accepted, nothing is thrown, and `thrown.code` is a `TypeError` rather than a wrong code. So **re-aiming the fixture is the fix, and it is local and cheap; asserting the message is an option that additionally pins WHICH refusal, never a requirement.** The review move splits the same way: counting the emitters of a code says only where attribution is *at risk* (rule 25's enumeration, read from the client's side), while the question per arm is which guards can fire on **its** input — so when you add a guard, list the existing arms whose input now reaches it first and re-aim each, then witness it by mutating the guard that arm **names**, which is exactly the guard a shared code hides. + + **Rider — a shared code is client-visible, so "same code" is a contract decision and not a test detail.** Where a route answers `{ error: code }` with no message, the operator cannot tell the two defects apart: one reading `issuer_metadata_incomplete` cannot distinguish a document that names no issuer from one that names no endpoint, and the code is all the response carries. Splitting the vocabulary is a separate, client-visible change; making that choice deliberately — or surfacing the message — is the honest form, and the worst outcome is neither: a test that cannot tell the two apart beside a response that cannot either. + + *(Earned: 2026-09-29, #2007/TASK-172 — the RFC 8414 §3.3 issuer check versus the endpoint guard in `backend/services/hostedMcpIntakeService.ts`. Found by the gate, not the author: deleting `if (!body.authorization_endpoint || !body.token_endpoint)` left the neighbourhood green, and one line adding an `issuer` to that arm's fixture restored the witness. **The first draft of this rule then drew the wrong conclusion from its own incident** — that a shared code obliges every arm asserting it to assert the message too — and the docs gate refused it with the counter-example that the measured table above now carries: at the repaired head the code assertion alone discriminates. Both halves are the rule, and the second is a caution about this file: a checklist line that over-prescribes the *assertion shape* sends every future reader to add a second assertion instead of asking which guard answers their input.)*