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
8 changes: 8 additions & 0 deletions docs/development/review-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.)*
Loading