eliott/worldmonitor
1
0
Fork 0
mirror of https://github.com/koala73/worldmonitor.git synced 2026-04-25 17:14:57 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(shipping/v2): alertThreshold: 0 preserved; drop dead validation branch (#3242 followup)

Browse Source 
Before: alert_threshold was a plain int32. proto3 scalar default is 0, so
the handler couldn't distinguish "partner explicitly sent 0 (deliver every
disruption)" from "partner omitted the field (apply legacy default 50)" —
both arrived as 0 and got coerced to 50 by `> 0 ? : 50`. Silent intent-drop
for any partner who wanted every alert. The subsequent `alertThreshold < 0`
branch was also unreachable after that coercion.

After:
- Proto field is `optional int32 alert_threshold` — TS type becomes
  `alertThreshold?: number`, so omitted = undefined and explicit 0 stays 0.
- Handler uses `req.alertThreshold ?? 50` — undefined → 50, any number
  passes through unchanged.
- Dead `< 0 || > 100` runtime check removed; buf.validate `int32.gte = 0,
  int32.lte = 100` already enforces the range at the wire layer.

Partner wire contract: identical for the omit-field and 1..100 cases.
Only behavioural change is explicit 0 — previously impossible to request,
now honored per proto3 optional semantics.

Scoped `buf generate --path worldmonitor/shipping/v2` to avoid the full-
regen `@ts-nocheck` drift Seb documented in the #3242 PR comments.
Re-applied `@ts-nocheck` on the two regenerated files manually.

Tests:
- `alertThreshold 0 coerces to 50` flipped to `alertThreshold 0 preserved`.
- New test: `alertThreshold omitted (undefined) applies legacy default 50`.
- `rejects > 100` test removed — proto/wire validation handles it; direct
  handler calls intentionally bypass wire and the handler no longer carries
  a redundant runtime range check.

Verified: 18/18 shipping-v2-handler tests pass, typecheck + typecheck:api
clean, all 4 custom lints clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Sebastien Melki
2026-04-22 10:21:42 +03:00
parent 0071c1253b
commit f33394648f
7 changed files with 45 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
docs/api/ShippingV2Service.openapi.json
View File

File diff suppressed because one or more lines are too long

7
docs/api/ShippingV2Service.openapi.yaml
View File

@@ -277,7 +277,12 @@ components:
maximum: 100
minimum: 0
format: int32
description: Disruption-score threshold for delivery, 0-100. Default 50.
description: |-
Disruption-score threshold for delivery, 0-100. Default 50.
proto3 `optional` so the handler can distinguish "partner explicitly sent
0 (deliver every alert)" from "partner omitted the field (apply default
50)". Without `optional`, both serialise to the proto3 scalar default of
0 and the handler can't tell them apart — flagged in #3242 review.
required:
- callbackUrl
description: |-

6
proto/worldmonitor/shipping/v2/register_webhook.proto
View File

@@ -20,7 +20,11 @@ message RegisterWebhookRequest {
// the entire CHOKEPOINT_REGISTRY. Unknown IDs fail with 400.
repeated string chokepoint_ids = 2;
// Disruption-score threshold for delivery, 0-100. Default 50.
int32 alert_threshold = 3 [
// proto3 `optional` so the handler can distinguish "partner explicitly sent
// 0 (deliver every alert)" from "partner omitted the field (apply default
// 50)". Without `optional`, both serialise to the proto3 scalar default of
// 0 and the handler can't tell them apart — flagged in #3242 review.
optional int32 alert_threshold = 3 [
(buf.validate.field).int32.gte = 0,
(buf.validate.field).int32.lte = 100
];

15
server/worldmonitor/shipping/v2/register-webhook.ts
View File

@@ -66,14 +66,13 @@ export async function registerWebhook(
]);
}
// Proto default int32 is 0 — treat 0 as "unset" to preserve the legacy
// default of 50 when the caller omits alertThreshold.
const alertThreshold = req.alertThreshold > 0 ? req.alertThreshold : 50;
if (alertThreshold < 0 || alertThreshold > 100) {
throw new ValidationError([
{ field: 'alertThreshold', description: 'alertThreshold must be a number between 0 and 100' },
]);
}
// alert_threshold is `optional int32` (#3242 followup #4) — undefined means
// the partner omitted the field, so apply the legacy default of 50. An
// explicit 0 is preserved (deliver every alert). The 0..100 range is
// enforced by buf.validate at the proto layer; the handler doesn't need
// a runtime branch (the previous `< 0` check was dead code after the
// `> 0 ? : 50` coercion).
const alertThreshold = req.alertThreshold ?? 50;
const ownerTag = await callerFingerprint(ctx.request);
const newSubscriberId = generateSubscriberId();

2
src/generated/client/worldmonitor/shipping/v2/service_client.ts
View File

@@ -39,7 +39,7 @@ export interface BypassOption {
export interface RegisterWebhookRequest {
callbackUrl: string;
chokepointIds: string[];
alertThreshold: number;
alertThreshold?: number;
}
export interface RegisterWebhookResponse {

2
src/generated/server/worldmonitor/shipping/v2/service_server.ts
View File

@@ -39,7 +39,7 @@ export interface BypassOption {
export interface RegisterWebhookRequest {
callbackUrl: string;
chokepointIds: string[];
alertThreshold: number;
alertThreshold?: number;
}
export interface RegisterWebhookResponse {

36
tests/shipping-v2-handler.test.mjs
View File

@@ -207,16 +207,13 @@ describe('ShippingV2Service handlers', () => {
);
});
it('rejects alertThreshold > 100 with ValidationError', async () => {
await assert.rejects(
() => registerWebhook(proCtx(), {
callbackUrl: 'https://hooks.example.com/wm',
chokepointIds: [],
alertThreshold: 150,
}),
(err) => err instanceof ValidationError && err.violations[0].field === 'alertThreshold',
);
});
// alert_threshold 0..100 range is now enforced at the proto/wire layer
// by buf.validate (gte/lte on `optional int32 alert_threshold`).
// Direct handler invocation bypasses wire validation; the handler no
// longer carries a redundant runtime range check (was dead code after
// the previous `> 0 ? : 50` coercion). The wire path is exercised by
// the sebuf gateway integration; this unit test would only assert
// behaviour the handler intentionally delegates upstream.
it('happy path returns wh_-prefixed subscriberId and 64-char hex secret; issues SET + SADD + EXPIRE pipeline with 30-day TTL', async () => {
const calls = stubRedisOk();
@@ -246,7 +243,22 @@ describe('ShippingV2Service handlers', () => {
assert.equal(pipeline[2][2], String(86400 * 30));
});
it('alertThreshold 0 (proto default) coerces to legacy default 50', async () => {
it('alertThreshold omitted (undefined) applies the legacy default of 50', async () => {
const calls = stubRedisOk();
await registerWebhook(proCtx(), {
callbackUrl: 'https://hooks.example.com/wm',
chokepointIds: [],
// alertThreshold omitted — proto3 `optional int32` arrives as undefined
});
const record = JSON.parse(calls[0][0][2]);
assert.equal(record.alertThreshold, 50);
});
it('alertThreshold explicit 0 is preserved (deliver every alert)', async () => {
// #3242 followup #4 — proto3 `optional` lets the handler distinguish
// "partner explicitly sent 0" from "partner omitted the field". The
// pre-fix handler coerced both to 50, silently dropping the partner's
// intent to receive every disruption.
const calls = stubRedisOk();
await registerWebhook(proCtx(), {
callbackUrl: 'https://hooks.example.com/wm',
@@ -254,7 +266,7 @@ describe('ShippingV2Service handlers', () => {
alertThreshold: 0,
});
const record = JSON.parse(calls[0][0][2]);
assert.equal(record.alertThreshold, 50);
assert.equal(record.alertThreshold, 0);
});
it('empty chokepointIds subscribes to the full CHOKEPOINT_REGISTRY', async () => {