fix(app-blocks): round-0 audit fixes — six wrong or stale counts, three false rationales

Net -29 lines. No behaviour change except the one noted below; everything else is prose that did not survive its own first extension. Each item's evidence was re-verified before acting. ARITHMETIC, re-derived rather than adjusted: - bridgeLabels.ts said this sixth outcome grew the label product "BY 20% — (approved apps + 1) x 47 x 2 x 6". Both halves wrong: 20% is the outcome axis alone and the type axis grew too, and the `x 47` omits the `'other'` slot the other two sites include. Re-derived at 50 approved apps: 51 x 48 x 2 x 6 = 29,376 against 51 x 47 x 2 x 5 = 23,970, i.e. +22.6%. Also recorded that the product is a CEILING, not allocated heap — nothing pre-initialises the label space, so the sixth value costs zero series until a rejection occurs. - block-message.ts said "roughly 9x the existing renders_total product". The only renders_total figure in the repo is ~2,040, so it is ~14x. (The base said "7x" against 11.75x, so this one was already wrong before this branch — but it was rewritten rather than re-derived, which is the same defect.) STALE COUNTS the change should have touched and did not: - bridgeLabels.ts "46-key INVENTORY" -> 47-key - bridgeLabels.ts "wrong for THREE of the five outcomes" -> FOUR of the six - bridgeMessageBeacon.ts "three of the five outcomes are reported above the bridge's inbound limiter" -> four of the six; validator_rejected is the fourth, which this branch's own comment already said - usePostMessageOutcomes.browser.test.tsx header "The four DISPATCHER outcomes … The fifth, no_token" -> five of the six are dispatcher-side now FALSE RATIONALES — each would have led a reader to the wrong conclusion: - usePostMessageOutcomes.browser.test.tsx said "`report` -> `recordBridgeMessage` -> `boundBridgeMessageType` does the clamping; this pins that the branch routes through it". That is the PRE-correction design: these tests supply their own `onOutcome`, so `recordBridgeMessage` never runs and the clamp under test is the branch's own. A reader following the old comment would conclude the branch's clamp is dead code, delete it, redden four rows, and read the failure as "the test is wrong". - bridgeTelemetry.test.ts justified the new INVENTORY entry by the dispatcher's `no_handler` bookkeeping — a path this same change makes UNREACHABLE, since the new branch returns above it. The entry's real and only current purpose is hostHandlerParity's one-directional compile-time gate, i.e. it is what lets this repo bump @civitai/app-sdk past the version adding the message. Also recorded the cost of keeping it: boundBridgeMessageType now passes 'BLOCK_MESSAGE_REJECTED' through for a forged POST instead of clamping it. - bridgeLabels.ts called this "the fifth silence". Retracted: the SDK's own handleMessage still drops silently and uncounted on an origin mismatch, on a malformed envelope, and on a well-formed reply whose requestId matches no pending request. This value covers the validator path only. DELETED: - 10 of the 37 comment lines in usePostMessage.ts's new branch. Five of its six paragraphs restated bridgeLabels.ts, and the duplication had ALREADY drifted from the original inside this same branch — which is the defect the six counts above are. It now points at bridgeLabels.ts for the reading rules and keeps only what is specific to the branch. - one of the four clamp test.each rows. `{ type: 42 }` and `{}` reach the same `typeof !== 'string'` arm; each row costs a full renderWithProviders + iframe mount in chromium. `undefined` is kept — it is the only row exercising the `?.`. Re-verified after the changes, since an audit fix resets the gate: pnpm typecheck 0 errors; vitest unit over src/components/AppBlocks + src/tests/api/track + src/server/metrics + src/server/schema 1820 tests; component (chromium) 14 tests (was 15 — one row removed). Mutation matrix re-run: disabling the dispatcher branch reddens 5 of the 6 new tests, the survivor being the negative control; removing the extraction-site clamp reddens exactly 1, printing the leaked value. Pairs with civitai/civitai-app-starters#317. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-20 22:08:18 +08:00 · 2026-09-19 11:47:17 -05:00
parent 80ee34781d
commit 37a2ef5b34
6 changed files with 76 additions and 55 deletions
@@ -79,10 +79,18 @@ describe('the label enums are closed', () => {
  });

  test('BLOCK_MESSAGE_REJECTED is an INVENTORY key, so the label bound admits it', () => {
-    // It arrives as an ordinary inbound bridge message, so `boundBridgeMessageType`
-    // sees its type like any other. Not in INVENTORY ⇒ the dispatcher's own
-    // `no_handler` bookkeeping for it would read `'other'`, and the entry is also
-    // what documents that no per-host handler exists for it.
+    // 🔴 ITS PURPOSE IS THE COMPILE-TIME GATE, AND NOTHING ELSE TODAY. An earlier
+    // revision of this comment justified the entry by the dispatcher's `no_handler`
+    // bookkeeping — a path the same change makes UNREACHABLE, because the new
+    // `BLOCK_MESSAGE_REJECTED` branch returns above the `no_handler` branch. The
+    // real reason is `hostHandlerParity.ts`'s one-directional gate: every type in
+    // the PUBLISHED SDK's block→host union must be an INVENTORY key, so this entry
+    // is what lets this repo bump `@civitai/app-sdk` past the version that adds the
+    // message. It is also where the N/A rationale for both hosts is documented.
+    // ⚠️ Keeping it does widen what the public beacon admits by one label value
+    // nothing legitimate can emit — `boundBridgeMessageType` now passes
+    // `'BLOCK_MESSAGE_REJECTED'` through for a forged POST instead of clamping it to
+    // `'other'`. Bounded and harmless, but it is a real cost of the entry.
    expect(Object.prototype.hasOwnProperty.call(INVENTORY, 'BLOCK_MESSAGE_REJECTED')).toBe(true);
    expect(boundBridgeMessageType('BLOCK_MESSAGE_REJECTED')).toBe('BLOCK_MESSAGE_REJECTED');
    // Fire-and-forget: nothing awaits it, so there is nothing to NACK. A reply
@@ -2,7 +2,7 @@
 * App Blocks BRIDGE telemetry — the label sets and wire bounds, and NOTHING ELSE.
 *
 * 🔴 IT IS A SEPARATE MODULE FROM `bridgeTelemetry.ts` FOR ONE MEASURED REASON:
- * `bridgeTelemetry` imports `hostHandlerParity`'s 46-key `INVENTORY` (~6.7 KB
+ * `bridgeTelemetry` imports `hostHandlerParity`'s 47-key `INVENTORY` (~6.8 KB
 * minified) to bound the `type` label. `src/server/schema/track.schema.ts` needs
 * the label ENUMS so the beacon's zod schema and the emitter cannot drift — and
 * `track.schema` is imported by `TrackView` on pages that mount no block at all,
@@ -35,9 +35,13 @@
 *   no_token     — a handler ran, found no usable block credential, and refused.
 *   validator_rejected
 *                — the BLOCK refused our reply at its own trust boundary and
- *                  dropped it, so its request hangs to the SDK timeout. The fifth
- *                  silence, and the only one of the five with a confirmed
- *                  production incident.
+ *                  dropped it, so its request hangs to the SDK timeout. The only
+ *                  outcome here with a confirmed production incident behind it.
+ *                  ⚠️ NOT "the fifth and final" silence — the SDK's own
+ *                  `handleMessage` still drops silently and uncounted on an origin
+ *                  mismatch, on a malformed envelope, and on a well-formed reply
+ *                  whose `requestId` matches no pending request. This value covers
+ *                  the validator path only.
 *
 * 🔴 `no_token` is reported BY THE HANDLER, not by the dispatcher — the dispatcher
 * has no idea a token exists. It rides the same counter because an operator asking
@@ -59,10 +63,16 @@
 *    deliberate UNDERCOUNT on a sustained break. Read it as *which types are being
 *    rejected and when it started*, never as an exact total.
 *
- * ⚠️ ADDING THIS SIXTH VALUE GREW THE COUNTER'S LABEL PRODUCT BY 20% —
- * (approved apps + 1) x 47 x 2 x 6. `/api/track/block-message`'s docblock asks for
- * that product to be read before a label is added; an outcome VALUE is the cheaper
- * axis than a fifth label, which is why this arrived as one.
+ * ⚠️ ADDING THIS SIXTH VALUE GREW THE COUNTER'S WORST-CASE LABEL PRODUCT BY 22.6%,
+ * not the 20% an earlier revision of this line claimed: the outcome axis alone is
+ * +20%, but `BLOCK_MESSAGE_REJECTED` also added an INVENTORY key, so the type axis
+ * moved too. Re-derived at 50 approved apps: (50+1) x 48 x 2 x 6 = 29,376, against
+ * 51 x 47 x 2 x 5 = 23,970 before. Both include the `'other'` slot each axis adds —
+ * that line's `x 47` omitted it. `/api/track/block-message`'s docblock asks for the
+ * product to be read before a label is added; an outcome VALUE is the cheaper axis
+ * than a fifth label, which is why this arrived as one. ⚠️ And it is a CEILING, not
+ * allocated heap: nothing pre-initialises the label space, so the sixth value costs
+ * zero series until a rejection actually occurs.
 */
 export const BRIDGE_MESSAGE_OUTCOMES = [
  'handled',
@@ -102,7 +112,7 @@ export const BRIDGE_MESSAGE_BATCH_MAX = 200;
 * 🔴 IT IS A SANITY CEILING, NOT A RATE CONTROL, AND THE DIFFERENCE MATTERS. An
 * earlier revision of this comment derived it as "30 msg/sec × a 10 s flush window
 * = 300 legitimate max, so nothing real can reach it". That derivation is wrong
- * for THREE of the five outcomes and was cited as justification in two other
+ * for FOUR of the six outcomes and was cited as justification in two other
 * files, so it is corrected here rather than quietly dropped:
 *
 *   - `handled` and `no_token` are the only two the bridge's 30 msg/sec inbound
@@ -180,7 +180,7 @@ export function flushBridgeMessages(): void {
  // fires the series reads "enormous" instead of "rejected" — see
  // `BRIDGE_MESSAGE_COUNT_MAX` for why the ceiling is a SANITY bound and not, as
  // an earlier revision of this comment claimed, a figure no real client can
-  // reach: three of the five outcomes are reported above the bridge's inbound
+  // reach: four of the six outcomes are reported above the bridge's inbound
  // limiter and are not bounded by it at all.
  const events = [...counts.values()]
    .slice(0, BRIDGE_MESSAGE_BATCH_MAX)
@@ -312,43 +312,33 @@ export function usePostMessage(opts: UsePostMessageOptions): UsePostMessageResul

      const now = Date.now();

-      // ── THE FIFTH SILENCE, reported by the only party that can see it ────────
+      // ── The one drop path this host cannot observe, reported by the block ────
      // The block's transport refused one of OUR replies at its own trust boundary
      // and dropped it, so its request is now hanging to the SDK timeout. We cannot
-      // observe that ourselves: the SDK's validator runs in the iframe AFTER we have
-      // replied, so from here the exchange completed and the `handled` below already
-      // counted it. `BLOCK_MESSAGE_REJECTED` (@civitai/app-sdk/blocks, fire-and-
-      // forget, no requestId) is the block telling us.
+      // see that: the SDK's validator runs in the iframe AFTER we replied, so from
+      // here the exchange completed and the `handled` below already counted it.
+      // `BLOCK_MESSAGE_REJECTED` is the block telling us. Why the label is the
+      // REQUEST and not the rejected reply, how to read the series, and what this
+      // does NOT cover are all in `bridgeLabels.ts` — stated once, there, because an
+      // earlier revision of this block restated them here and the copy had already
+      // drifted from the original inside this very commit.
      //
-      // 🔴 THE LABEL COMES FROM THE PAYLOAD, NOT THE MESSAGE TYPE. `payload.type` is
-      // the block→host REQUEST left hanging (`GET_IMAGES_BY_IDS`), which is what an
-      // operator needs and — not incidentally — the only spelling that survives
-      // `boundBridgeMessageType`: the INVENTORY it clamps against holds no `*_RESULT`
-      // key, so the rejected REPLY's type would collapse to `'other'`. It is
-      // block-supplied and therefore untrusted, so it is clamped HERE.
+      // 🔴 THE LABEL IS CLAMPED HERE, AT THE EXTRACTION SITE, NOT LEFT TO THE SINK.
+      // The other `report(...)` callers pass `data.type` and let
+      // `recordBridgeMessage` clamp on the way out; that is not enough for a value
+      // pulled out of an untrusted payload, because `onOutcome` is a seam — every
+      // browser test in `usePostMessageOutcomes.browser.test.tsx` supplies its own
+      // sink, and with the clamp downstream one of them observed the raw
+      // `NOT_A_REAL_MESSAGE`. Clamping here makes the value this branch emits the
+      // value that lands in the series, whatever the sink; the default sink clamps
+      // again, idempotently.
      //
-      // 🔴 CLAMPED AT THE EXTRACTION SITE, NOT LEFT TO THE SINK — and that is a
-      // correction, not belt-and-braces. The other `report(...)` callers pass
-      // `data.type`, which `recordBridgeMessage` clamps on the way out; but
-      // `onOutcome` is a documented SEAM (every browser test supplies its own sink,
-      // and `report`'s whole job is to be sink-agnostic), so a value pulled out of
-      // an untrusted payload and bounded only by the default sink is bounded by
-      // nothing a reader of this branch can see. Measured while writing
-      // `usePostMessageOutcomes.browser.test.tsx`: with the clamp downstream, the
-      // test's own sink observed the raw `NOT_A_REAL_MESSAGE`. Clamping here makes
-      // the value this branch emits the value that lands in the series, whatever
-      // the sink; the default sink clamps again, idempotently.
-      //
-      // 🔴 EXACTLY ONE INCREMENT, AND NOT `handled`. Returning here keeps the report
-      // itself out of the `handled` denominator — one rejection must move one series
-      // by one, or the counter double-books and stops being readable as a rate.
-      //
-      // ABOVE the rate limiter and the dedup map, deliberately, for the same reason
-      // the `no_handler` branch is: a flood of junk must not burn the 30 msg/sec
-      // budget legitimate BLOCK_ERROR reporting needs. Magnitude is consequently
-      // unbounded here, as it already is for `no_handler`/`deduped` — see
-      // `BRIDGE_MESSAGE_COUNT_MAX`. Dedup would also be actively wrong: these carry
-      // no `requestId`, and two rejections of the same type are two facts.
+      // 🔴 EXACTLY ONE INCREMENT, AND NOT `handled` — returning here keeps the report
+      // out of the denominator, or one rejection moves two series by one and every
+      // ratio read against `handled` goes quietly wrong. ABOVE the limiter and the
+      // dedup map, for the same reason the `no_handler` branch is: a flood of junk
+      // must not burn the budget legitimate BLOCK_ERROR reporting needs. Dedup would
+      // also be wrong — these carry no `requestId`, and two rejections are two facts.
      if (data.type === 'BLOCK_MESSAGE_REJECTED') {
        const rejected = (data.payload as { type?: unknown } | null | undefined)?.type;
        report(
@@ -17,9 +17,11 @@ import type { BridgeMessageOutcome } from '~/components/AppBlocks/bridgeTelemetr
 * then UNREGISTER it and prove the SAME message now reports `no_handler`. One
 * variable moves, both arms are read, and the before/after pair is the evidence.
 *
- * The four DISPATCHER outcomes are all exercised against the real hook. The fifth,
- * `no_token`, is a HANDLER-side report and is pinned on the hosts themselves, in
- * `PageBlockHostNoTokenNack.browser.test.tsx` — which asserts BOTH the reply the
+ * FIVE of the six outcomes are dispatcher-side and are all exercised against the
+ * real hook here — including `validator_rejected`, which the dispatcher produces
+ * from the block's own `BLOCK_MESSAGE_REJECTED` report rather than observing.
+ * `no_token` is the one HANDLER-side report and is pinned on the hosts themselves,
+ * in `PageBlockHostNoTokenNack.browser.test.tsx` — which asserts BOTH the reply the
 * handler sends and the row that reaches the real beacon buffer, because those
 * are produced by different lines and a test of one says nothing about the other.
 */
@@ -283,15 +285,26 @@ describe('usePostMessage bridge outcome counter', () => {

  test.each([
    ['a type the protocol does not declare', { type: 'NOT_A_REAL_MESSAGE' }],
+    // `{ type: 42 }` and `{}` reach the SAME branch (the `typeof !== 'string'` arm),
+    // so only one of them is kept — each row costs a full renderWithProviders +
+    // iframe mount in chromium. `undefined` is distinct: it exercises the `?.`, and
+    // without it that dereference throws.
    ['a non-string type', { type: 42 }],
-    ['no type at all', {}],
    ['no payload at all', undefined],
  ])('clamps %s to `other` rather than minting a series', async (_label, payload) => {
    // The payload is BLOCK-supplied and the value becomes a prom label on a host
    // that retains every distinct label set in heap forever, so the clamp is the
-    // security property of this branch, not tidiness. `report` ->
-    // `recordBridgeMessage` -> `boundBridgeMessageType` does the clamping; this
-    // pins that the branch actually routes through it.
+    // security property of this branch, not tidiness.
+    //
+    // 🔴 THE CLAMP UNDER TEST IS THE ONE IN THE BRANCH, NOT THE SINK'S. These tests
+    // supply their own `onOutcome`, so `recordBridgeMessage` — and the
+    // `boundBridgeMessageType` call inside it — never runs. An earlier revision of
+    // this comment said the clamping happened downstream in the sink, which would
+    // tell a reader the branch's own `boundBridgeMessageType(...)` is dead code and
+    // safe to delete; deleting it reddens these four rows, and the failure would
+    // then read as "the test is wrong". The branch clamps itself, on purpose,
+    // because `onOutcome` is a seam and a value pulled from an untrusted payload
+    // must not be bounded only by the default sink.
    const recorded: Recorded[] = [];
    await mount({ onOutcome: (e) => recorded.push(e), registered: true });
    postFromBlock('BLOCK_MESSAGE_REJECTED', payload);
@@ -38,7 +38,7 @@ import { PublicEndpoint } from '~/server/utils/endpoint-helpers';
 //
 // 🔴 BOUNDED IS NOT THE SAME AS SMALL — READ THE PRODUCT BEFORE ADDING A LABEL.
 // The domain is (approved apps + 1) x 48 x 2 x 6: ~29k series per pod at 50
-// approved apps, roughly 9x the existing `renders_total` product and the largest
+// approved apps, ~14x the existing `renders_total` product (~2,040) and the largest
 // App Block label set in the module. It is bounded, and CARDINALITY is the property
 // this route enforces — that is the prom-heap axis, and it is genuinely closed. So
 // a fifth label is not a free addition, and neither is a laxer clamp.