From 568df1c2ec2f8aa51c122413267c48a2086cd7d1 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 29 Sep 2026 08:05:31 -0700 Subject: [PATCH 1/2] =?UTF-8?q?docs(checklist):=20rule=2042=20=E2=80=94=20?= =?UTF-8?q?a=20shared=20refusal=20code=20makes=20an=20arm's=20witness=20am?= =?UTF-8?q?biguous?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two guards that refuse with the same code make one arm ambiguous, and a guard added ABOVE an existing one silently transfers the old guard's only witness to the new one. The arm does not change, the test file shows nothing, the suite is green — which is why the review check is not "is this arm good" but "how many guards emit this code". Earned today on #2007/TASK-172. The new RFC 8414 §3.3 issuer check landed above an established endpoint guard; the neighbouring arm sent a document naming no issuer, kept refusing with the same `issuer_metadata_incomplete`, and so was pinning the new guard rather than the one it was written for. Deleting the endpoint guard changed no test in the neighbourhood. Measured on both sides of the repair, in one instrument: pre-repair arm + guard deleted = 30/30 green (the survivor), post-repair arm + guard deleted = that arm alone goes red, by name. Rider: where a route answers `{ error: code }` with no message, the shared code is client-visible too, so "same code" is a contract decision rather than a test detail. Guard: scripts/verify-numbered-rules.js → 42 rules, 1..42, 20 citations resolve. --- docs/development/review-checklist.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index d2efd0f9c..13d944c4d 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. **Two guards that refuse with the same code make one arm ambiguous, and a guard added ABOVE an existing one silently transfers the old guard's only witness to the new one — so an assertion on a code is a witness only while that code is unique to the guard.** The arm does not change, the diff shows nothing in the test file, and the suite stays green: that is the whole failure mode, and it means the check is not "is this arm good" but "how many guards emit this code". The new RFC 8414 §3.3 issuer check landed above an established endpoint guard, and the neighbouring arm — written when only the endpoint guard could fire, sending a document that named no issuer — kept refusing, with the same `issuer_metadata_incomplete`, for a reason it had never tested. Deleting the endpoint guard outright then changed **no test in the neighbourhood** (green, measured by the reviewer and reproduced independently). The refusal was never in doubt; its ATTRIBUTION was, and the arm was pinning whichever guard happened to run first. The mechanical move, on both sides of the review: grep the refusal code and count its emitters — more than one means every arm asserting it must pin the MESSAGE as well, because the code cannot discriminate (rule 25 enumerates a guard set by its code for reachability; this is that same code read as a test's identity) — and when adding a guard, name which existing arms now sit downstream of it and re-aim each fixture so the intended guard is the one that fires. + + **Rider — the 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, two defects sharing a code are indistinguishable to the operator: one reading `issuer_metadata_incomplete` cannot tell a document that names no issuer from one that names no endpoint, and the code is the only thing 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 §3.3 issuer check versus the endpoint guard in `backend/services/hostedMcpIntakeService.ts`. Found by the gate rather than the author: their measurement was that deleting `if (!body.authorization_endpoint || !body.token_endpoint)` left the neighbourhood green, and their repair was one line — give the arm an `issuer`, which is a fixture change that also has to pin the message, because the two refusals had shared `issuer_metadata_incomplete` since the issuer check landed and the deliberate no-issuer arm beside it asserted that same code. Reproduced with a mutant on both sides: a survivor before, and after the repair the arm alone goes red, by name.)* From 3ec8034b7f4c8a02f9c7092f1240e2f009ec7b47 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 29 Sep 2026 08:09:12 -0700 Subject: [PATCH 2/2] =?UTF-8?q?docs(checklist):=20rule=2042=20=E2=80=94=20?= =?UTF-8?q?witness=20is=20fixture=20reachability,=20not=20the=20assertion?= =?UTF-8?q?=20shape?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Corrected per the docs gate's HOLD at 568df1c2. The first draft claimed a shared refusal code obliges every arm to assert the message; the rule's own incident disproves it, and I measured the counter-example before rewriting: arm fixture endpoint guard deleted result no `issuer` (pre) yes 30/30 green (guard unwitnessed) `issuer` present yes that arm ALONE reds — on the CODE assertion: with the guard gone nothing is thrown, so `thrown.code` is a TypeError So re-aiming the fixture is the fix and it is local; pinning the message is an option that additionally pins WHICH refusal, not a requirement. The review move: counting a code's emitters says where attribution is at risk (rule 25); the per-arm question is which guards can fire on its input, and the witness is a mutation of the guard that arm NAMES. The rider (a shared code is client-visible, so "same code" is a contract decision) is unchanged. Guard: 42 rules, 1..42, 20 citations resolve. --- docs/development/review-checklist.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 13d944c4d..7603ad0b7 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -146,8 +146,8 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## Refusal codes as witnesses -42. **Two guards that refuse with the same code make one arm ambiguous, and a guard added ABOVE an existing one silently transfers the old guard's only witness to the new one — so an assertion on a code is a witness only while that code is unique to the guard.** The arm does not change, the diff shows nothing in the test file, and the suite stays green: that is the whole failure mode, and it means the check is not "is this arm good" but "how many guards emit this code". The new RFC 8414 §3.3 issuer check landed above an established endpoint guard, and the neighbouring arm — written when only the endpoint guard could fire, sending a document that named no issuer — kept refusing, with the same `issuer_metadata_incomplete`, for a reason it had never tested. Deleting the endpoint guard outright then changed **no test in the neighbourhood** (green, measured by the reviewer and reproduced independently). The refusal was never in doubt; its ATTRIBUTION was, and the arm was pinning whichever guard happened to run first. The mechanical move, on both sides of the review: grep the refusal code and count its emitters — more than one means every arm asserting it must pin the MESSAGE as well, because the code cannot discriminate (rule 25 enumerates a guard set by its code for reachability; this is that same code read as a test's identity) — and when adding a guard, name which existing arms now sit downstream of it and re-aim each fixture so the intended guard is the one that fires. +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 — the 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, two defects sharing a code are indistinguishable to the operator: one reading `issuer_metadata_incomplete` cannot tell a document that names no issuer from one that names no endpoint, and the code is the only thing 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. + **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 §3.3 issuer check versus the endpoint guard in `backend/services/hostedMcpIntakeService.ts`. Found by the gate rather than the author: their measurement was that deleting `if (!body.authorization_endpoint || !body.token_endpoint)` left the neighbourhood green, and their repair was one line — give the arm an `issuer`, which is a fixture change that also has to pin the message, because the two refusals had shared `issuer_metadata_incomplete` since the issuer check landed and the deliberate no-issuer arm beside it asserted that same code. Reproduced with a mutant on both sides: a survivor before, and after the repair the arm alone goes red, by name.)* + *(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.)*