diff --git a/AGENTS.md b/AGENTS.md index 27de277..9002658 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -944,6 +944,31 @@ Grouped by what each one constrains. only because nothing stops the reconnect loop without `emitClose()` following it: `drain()` and `end()` are the only callers of `stop()`. A third caller has to shut the gate itself. +- **A segment the SMSC took and named no id for is `undefined` in `smsIds`, not an empty string.** + Maintainer's call, 2026-09-12: `paramText()` resolves an absent `message_id` and one a peer wrote + empty to the same `''`, which `string[]` then presented as an id — taking `smsIds[0]`, or keying a + correlation table by the array, compiled and then misbehaved, and one message's empty entry + collides with another's. Telesign names an id for the first segment of a concatenated submit only, + so it is a documented operator's shape rather than a hypothesis. Nothing else moves: + `parseSegmentId('')` matched nothing, so `DlrMerger` already abandoned such a send and `undefined` + reaches that same refusal. `expect()` takes the wider type rather than a filtered `string[]` + because the arity is what `idNumbering()` refuses on: filtering `['a-1', undefined, 'a-3']` leaves + a numbering that spells out a whole message, and merges one that was never whole. `dlrFromPdu()` + reads an id through `nonEmptyText()`, so no receipt could ever have matched an empty entry — and + that reading stays separate from this one rather than sharing a helper, since it must leave a + Buffer-valued `receipted_message_id` unresolved for `messageType()` to read the PDU as unmarked. + What settles it here is the resolved text rather than the parameter, because `writeParams()` + substitutes the field's own default: a peer that omits `message_id` and one that writes it empty + build the same octets, leaving a raw-parameter test nothing to tell apart. Rejected: keeping `''` + and documenting it, which leaves the published type promising what the value does not keep — + goal 2, a wrong answer about what the peer named. Rejected: dropping the unnamed entries, which + breaks the positional correspondence with `pduObjs` that README promises and loses which segment a + PDU belongs to. Rejected: `{ id?: string; pduObj: PduObject }[]`, which makes that positional + promise structural where today the compiler cannot check it; deferred to the next major, the first + place two documented fields may become one. Accepted: every consumer reading `smsIds` narrows, + including the majority whose SMSC names every id; indexing narrows too, except for the consumer + who sets `noUncheckedIndexedAccess`, which typed `smsIds[0]` as `string | undefined` already. + ### Internals and tests - **A listener that rejects is routed by Node's `captureRejections`, not by hand-dispatching.** Both diff --git a/README.md b/README.md index a4d62df..6c0fcbb 100644 --- a/README.md +++ b/README.md @@ -163,19 +163,19 @@ in `UCS2`. You get one id per segment: const { err, pduObjs, smsIds, unanswered } = await session.sendSms({ from, message, to }); ``` -`smsIds` is positional with `pduObjs`, and an entry is empty where the SMSC accepted the segment -without naming an id for it — some name one for the first segment only. No receipt ever matches an -empty entry. +`smsIds` is positional with `pduObjs`, and an entry is `undefined` where the SMSC accepted the +segment without naming an id for it — some name one for the first segment only. No receipt ever +carries an empty id, so an unnamed entry matches nothing. `err` is set when the SMSC refuses a segment, and it names the status it refused with. Because every segment goes on the wire together, `pduObjs` and `smsIds` then hold what the SMSC did accept — enough to reconcile against a later receipt, not enough to resend the rest, so treat a partial failure as a failed message. `unanswered` counts the segments that went out and were never answered: the SMSC may have taken each of them and lost only the response, so a message with `unanswered` above zero cannot -be sent again without risking a duplicate, however empty `smsIds` is. A message needing more than 255 -segments is refused before anything is sent, since the concatenation header numbers segments in a -single octet. `maxSegments` lowers that ceiling: -most handsets and SMSCs stop well short of 255, and refusing beats a message only half delivered. +be sent again without risking a duplicate, however few ids `smsIds` carries. A message needing more +than 255 segments is refused before anything is sent, since the concatenation header numbers +segments in a single octet. `maxSegments` lowers that ceiling: most handsets and SMSCs stop well +short of 255, and refusing beats a message only half delivered. ### Receiving @@ -624,6 +624,10 @@ promises and the rough edges taken off. it reports the id the segments were answered with, the id `sendResp()` was given, or the UUID v7 generated instead. Delete any `sms.smsId = …` line — assigning to it throws a `TypeError`, since modules are always strict mode — and pass the id to `sendResp()`. +- **`smsIds` from `sendSms()` is `(string | undefined)[]`**, one entry per segment and positional + with `pduObjs`, `undefined` where the SMSC took the segment without naming an id for it. Reading + an id narrows, indexing included: `smsIds[0]` is `string | undefined`, as it already was under + `noUncheckedIndexedAccess`. - **`checkuserpass` is now `authenticate`**, takes `{ password, session, systemId, systemType }` and returns `false` or `{ userData }`. - **Renamed options:** `enqLinkTiming` → `enquireLinkInterval`, server `timeout` → `idleTimeout`. diff --git a/interop-tests/findings/09-operator-fixtures.md b/interop-tests/findings/09-operator-fixtures.md index ae22177..bcea0a2 100644 --- a/interop-tests/findings/09-operator-fixtures.md +++ b/interop-tests/findings/09-operator-fixtures.md @@ -177,9 +177,9 @@ the fixtures pin that a reader would not otherwise expect: application wanting one report per message compares each `dlr.smsId` against the `smsIds` array `sendSms()` returned and merges them itself. - **Telesign answers only the first segment of a concatenated submit with a `message_id`.** - `sendSms()` returns `['', '', '']` — one entry per segment, positional with `pduObjs`, empty - where the SMSC named nothing. `dlr.smsId` is never empty, so an empty entry matches no receipt, - and no merge is armed. + `sendSms()` returns `['', undefined, undefined]` — one entry per segment, positional with + `pduObjs`, `undefined` where the SMSC named nothing; it returned `''` there until #101. + `dlr.smsId` is never empty, so an unnamed entry matches no receipt, and no merge is armed. ## Open questions @@ -203,6 +203,5 @@ the fixtures pin that a reader would not otherwise expect: `FAILED` and CM.com's `DELIVERD`. The documented-codes table in `operator-receipts.test.ts` is the place a new one goes, and it fails loudly for anything nothing names. - Whether `smsIds` carrying an empty entry for a segment the SMSC took but named no id for is the - right shape for a caller, or whether that case wants saying differently. It is documented in - `README.md` and pinned by the Telesign scenario; the question is a product one, not a correctness - one, and belongs to the phase 11 product-owner pass. + right shape for a caller. Settled in #101: the entry is `undefined`, and the decision with its + rejected alternatives is recorded in `AGENTS.md`. diff --git a/interop-tests/jasmin.test.ts b/interop-tests/jasmin.test.ts index eeb529f..c6c9b92 100644 --- a/interop-tests/jasmin.test.ts +++ b/interop-tests/jasmin.test.ts @@ -476,6 +476,7 @@ describe('C3+C7 - long MT through the fake upstream, receipts and id consistency assert.equal(sent.smsIds.length, testCase.expectedSegments); for (const id of sent.smsIds) { + assert.ok(id, 'expected Jasmin to name a message id for every segment'); assert.match(id, uuidPattern, 'expected a UUID-shaped message id from Jasmin\'s submit_sm_resp'); // The full round trip - submit_sm to Jasmin's smpps, mtrouter, AMQP, the connector bind, diff --git a/interop-tests/smppsim.test.ts b/interop-tests/smppsim.test.ts index 04bb07c..8a08817 100644 --- a/interop-tests/smppsim.test.ts +++ b/interop-tests/smppsim.test.ts @@ -97,6 +97,15 @@ function dlrLooksIntact(received: Received | undefined): received is Received { && received.pduObj.tlvs.message_state?.tagValue !== undefined; } +/** SMPPSim names an id for every segment, so an unnamed one is the finding, not the norm. */ +function namedIds(smsIds: (string | undefined)[]): string[] { + const ids = smsIds.filter(id => id !== undefined); + + assert.equal(ids.length, smsIds.length, 'expected SMPPSim to name a message id for every segment'); + + return ids; +} + /** * Resends a fresh message until one attempt's segments are every one matched by an intact `dlr`. * Retrying works around the peer+library interaction above without hiding it: a scenario only @@ -119,12 +128,14 @@ async function sendUntilAllDlrsArrive( assert.equal(sent.err, undefined); + const ids = namedIds(sent.smsIds); + const complete = await waitFor( - () => (sent.smsIds.every(id => dlrLooksIntact(dlrs.find(r => r.dlr.smsId === id))) ? true : undefined), + () => (ids.every(id => dlrLooksIntact(dlrs.find(r => r.dlr.smsId === id))) ? true : undefined), DLR_RETRY_BUDGET_MS, ); - if (complete) return sent.smsIds; + if (complete) return ids; } throw new Error(`no attempt got an intact DLR for every segment within ${String(DLR_MAX_ATTEMPTS)} tries`); @@ -149,14 +160,16 @@ async function sendUntilComplete( assert.equal(sent.err, undefined); + const ids = namedIds(sent.smsIds); + const complete = await waitFor(() => { - const allIntact = sent.smsIds.every(id => dlrLooksIntact(dlrs.find(r => r.dlr.smsId === id))); + const allIntact = ids.every(id => dlrLooksIntact(dlrs.find(r => r.dlr.smsId === id))); const reassembled = sms.find(s => s.message === message); return allIntact && reassembled ? { reassembled } : undefined; }, DLR_RETRY_BUDGET_MS); - if (complete) return { reassembled: complete.reassembled, smsIds: sent.smsIds }; + if (complete) return { reassembled: complete.reassembled, smsIds: ids }; } throw new Error(`no attempt got both an intact DLR per segment and a loopback reassembly within ${String(DLR_MAX_ATTEMPTS)} tries`); diff --git a/interop-tests/smscsim.test.ts b/interop-tests/smscsim.test.ts index 868f194..ba23042 100644 --- a/interop-tests/smscsim.test.ts +++ b/interop-tests/smscsim.test.ts @@ -38,13 +38,17 @@ async function sendAndAwaitDlrs(session: Session, dlrs: Dlr[], message: string): assert.equal(sent.err, undefined); + const ids = sent.smsIds.filter(id => id !== undefined); + + assert.equal(ids.length, sent.smsIds.length, 'expected smscsim to name a message id for every segment'); + const complete = await waitFor(() => ( - sent.smsIds.every(id => dlrs.some(dlr => dlr.smsId === id)) ? true : undefined + ids.every(id => dlrs.some(dlr => dlr.smsId === id)) ? true : undefined )); assert.ok(complete, 'every segment of the first send should get a DLR'); - return sent.smsIds; + return ids; } describe('smscsim - C1 bind, keepalive, unbind', () => { diff --git a/src/dlr-merger.ts b/src/dlr-merger.ts index c8537eb..0c20cae 100644 --- a/src/dlr-merger.ts +++ b/src/dlr-merger.ts @@ -37,12 +37,12 @@ const severity: Record = { }; /** The base and the part numbers one send's ids carry, or nothing when they do not spell out one message. */ -function idNumbering(smsIds: string[]): { base: string; parts: Set } | undefined { +function idNumbering(smsIds: (string | undefined)[]): { base: string; parts: Set } | undefined { const bases = new Set(); const parts = new Set(); for (const smsId of smsIds) { - const numbering = parseSegmentId(smsId); + const numbering = smsId === undefined ? undefined : parseSegmentId(smsId); if (!numbering) return undefined; @@ -93,7 +93,7 @@ export class DlrMerger { } /** Registers the ids one multipart send got back, so their receipts can be merged. */ - expect(smsIds: string[]): void { + expect(smsIds: (string | undefined)[]): void { if (smsIds.length < 2) return; const numbering = idNumbering(smsIds); diff --git a/src/send-sms.ts b/src/send-sms.ts index 43089b2..2223d5b 100644 --- a/src/send-sms.ts +++ b/src/send-sms.ts @@ -45,7 +45,8 @@ export type SendSmsInput = export type SendSmsResult = { err?: Error; pduObjs: PduObject[]; - smsIds: string[]; + /** Positional with `pduObjs`, and undefined where the SMSC took the segment and named no id. */ + smsIds: (string | undefined)[]; /** Segments that went out unanswered. The peer may have taken them, so sending again may duplicate. */ unanswered: number; }; @@ -248,12 +249,19 @@ function checkSegments(allowed: number, segments: number): Error | undefined { return undefined; } +/** An absent message_id and one a peer wrote empty are the same octets, and neither names an id. */ +function acceptedId(pduObj: PduObject, notation: SmsIdNotation | undefined): string | undefined { + const smsId = normaliseSmsId(paramText(pduObj.params.message_id), notation); + + return smsId === '' ? undefined : smsId; +} + function collectSent( sent: Result<{ pduObj: PduObject }>[], notation: SmsIdNotation | undefined, ): SendSmsResult { const pduObjs: PduObject[] = []; - const smsIds: string[] = []; + const smsIds: (string | undefined)[] = []; let failure: Error | undefined; let unanswered = 0; @@ -264,7 +272,7 @@ function collectSent( failure ??= one.err; } else if (one.pduObj.cmdStatus === 'ESME_ROK') { pduObjs.push(one.pduObj); - smsIds.push(normaliseSmsId(paramText(one.pduObj.params.message_id), notation)); + smsIds.push(acceptedId(one.pduObj, notation)); } else { const refusal = one.pduObj.cmdStatus ?? String(one.pduObj.cmdStatusId); diff --git a/test/operator-receipts.test.ts b/test/operator-receipts.test.ts index 753c82d..21eec8c 100644 --- a/test/operator-receipts.test.ts +++ b/test/operator-receipts.test.ts @@ -489,6 +489,38 @@ describe('an SMSC that reports each segment under an id of its own', () => { assert.equal(sent.err, undefined); assert.equal(smsc.octets.length, 3, 'every segment goes out whatever the peer answers'); - assert.deepEqual(sent.smsIds, [id, '', '']); + assert.deepEqual(sent.smsIds, [id, undefined, undefined]); + assert.equal(sent.smsIds.length, sent.pduObjs.length, 'an entry per accepted segment, positional with pduObjs'); + }); + + test('reports the segment that was named and merges nothing where the others were not', async t => { + const base = '01a09739-0a98-7ac0-8a9a-fcd3a7648e62'; + const smsc = await dummySmsc(t, { messageIds: [`${base}-1`] }); + const session = await bindToSmsc(t, smsc.port); + const merged: MessageDlr[] = []; + const reported = collect(1, push => { session.on('dlr', push); }); + + session.on('messageDlr', report => merged.push(report)); + + const sent = await session.sendSms({ dlr: true, message: 'x'.repeat(400), ...message }); + + assert.deepEqual(sent.smsIds, [`${base}-1`, undefined, undefined]); + smsc.deliver(receiptBody(`${base}-1`)); + + const [dlr] = await reported; + + assert.equal(dlr?.smsId, `${base}-1`); + assert.deepEqual(merged, [], 'a send with an unnamed segment spells out no message to merge'); + }); +}); + +describe('an SMSC that answers a submit with no message id at all', () => { + test('reads an empty message_id as no id, on a single-segment send as much as a split one', async t => { + const smsc = await dummySmsc(t, { messageIds: [] }); + const session = await bindToSmsc(t, smsc.port); + const sent = await session.sendSms({ dlr: true, message: 'one segment', ...message }); + + assert.equal(sent.err, undefined); + assert.deepEqual(sent.smsIds, [undefined]); }); }); diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 06ebfa9..2667867 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -2872,6 +2872,7 @@ describe('message id notation', () => { assert.deepEqual(sent.smsIds, [1, 2].map(part => `${sms.smsId}-${String(part)}`)); for (const smsId of sent.smsIds) { + assert.ok(smsId); await sendReceipt(peerOf(smpp), smsId); } diff --git a/test/session.test.ts b/test/session.test.ts index 484165e..db52bc8 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -2498,11 +2498,18 @@ describe('merged delivery report bounds', () => { test('arms nothing for a send whose ids do not number one message', () => { const dlrMerger = merger(); - dlrMerger.expect(['5cb0ea53b5d61093529174ca44e23871', '', '']); + dlrMerger.expect(['5cb0ea53b5d61093529174ca44e23871', undefined, undefined]); assert.equal(dlrMerger.size, 0, 'an id the peer never named numbers nothing'); dlrMerger.expect(['bf53ad8b-1', '40ccdce2-2']); assert.equal(dlrMerger.size, 0, 'nor do ids numbered off a base each'); + + const holed = '01a09739-0a9a-767d-ad1d-1c27667e1ca4'; + + // Dropping the unnamed entry rather than passing it through leaves 1 and 3 spelling out a + // whole 2-part message, and a send of 3 reports as delivered on two receipts. + dlrMerger.expect([`${holed}-1`, undefined, `${holed}-3`]); + assert.equal(dlrMerger.size, 0, 'nor does a hole between two parts of one base'); }); // A receipt for whole-3 would otherwise fill the slot whole-2 was registered for, truncating the report. diff --git a/todo.md b/todo.md index e639383..8d45797 100644 --- a/todo.md +++ b/todo.md @@ -113,13 +113,6 @@ session message is a change to every call site. - [ ] Create the `@larvit/smpp` package on npm and add `NPM_TOKEN` to the repository secrets, which `.github/workflows/release.yaml` needs. -- [ ] Decide whether `smsIds` should be `(string | undefined)[]`. An SMSC that names an id for the - first segment of a concatenated message only — Telesign documents exactly that — leaves the - rest empty, so `string[]` promises what the value does not keep: taking the first id, or - building a map from the array, compiles and then misbehaves, and an empty string collides with - another message's in a correlation table. The cost is that every consumer narrows, including - the majority whose SMSC always names an id. Raised by the phase 11 product review, 2026-09-08. - **This one is 1.0.0-or-never** — after release it needs a major version. - [ ] Tag `v1.0.0` to publish. - [ ] `npm deprecate larvitsmpp` pointing at `@larvit/smpp`. Maintainer's call to run it; not something CI should do.