From b3cbbba615dd3f8cf21fe38783fcf8f8f3e0b046 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 14 Sep 2026 16:32:23 +0200 Subject: [PATCH] Record the gaps found against other SMPP libraries, and make configurable and extendable a goal --- AGENTS.md | 36 +++++++----- todo.md | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 181 insertions(+), 28 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e6cdd3e..35c56d8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,16 +32,22 @@ one wins. They do not override the hard rules below. about a message the application sent reaches it as a report rather than as an inbound message, and says whether it is final, so nothing has to read the PDU to tell those apart. An option retunes a default or opts out of it; an option does not switch on the thing the caller obviously wanted. -6. **A small, stable public surface over reshapeable internals.** Only what `src/index.ts` exports is +6. **Configurable and extendable, never at the defaults' expense.** Where an application needs other + than the default and cannot build it from what is exported — a rate limit counted per PDU, an + alphabet, a receipt format — it gets an option or a hook rather than a fork. A call that passes no + options stays exactly as easy and as safe, and a hook is a seam the library calls, never a way into + its internals. +7. **A small, stable public surface over reshapeable internals.** Only what `src/index.ts` exports is published. A new option has to beat "the application can do this itself", and has to keep a promise this library can verify. The low-level surface is a passthrough: policy binds what the library composes, never what the caller wrote. -7. **Nothing that needs state wider than one session.** No throughput throttling, no persistence - across a restart, no coordination between processes — and no seam handing the application state to +8. **Nothing that needs state wider than one session.** No persistence across a restart, no + coordination between processes, no pool of sessions — and no seam handing the application state to persist for one of those either, which commits to the same scope through the back door and - publishes an internal shape to do it. This is the scope floor, and it is why an otherwise - reasonable feature is declined without a fresh argument each time. -8. **It builds, tests and runs the same everywhere.** Container-only toolchain, no runtime + publishes an internal shape to do it. A limit wider than a session, such as an account's rate + limit, is the application's to hold, behind a hook the library calls (goal 6). This is the scope + floor, and it is why an otherwise reasonable feature is declined without a fresh argument each time. +9. **It builds, tests and runs the same everywhere.** Container-only toolchain, no runtime dependencies, the Node 18 floor verified in CI rather than asserted, every README example executed by the suite. @@ -289,7 +295,7 @@ Grouped by what each one constrains. - **`PduRefusedError` is exported, and `sessionError` names it in the event's type.** Maintainer's call, 2026-09-05, from a product review: one event carries both a PDU the peer malformed and the session's own failure, and `instanceof` is the only way to separate them that hard rule 4 allows — - without the class as a value an application is left string-matching `err.message`. Goal 6 is paid by + without the class as a value an application is left string-matching `err.message`. Goal 7 is paid by exporting the discriminant and the struct it carries and nothing else: `PduHeader` is named because an application that logs or forwards a header wants a name for it, `PduRefusalReason` is not because `reason` is compared against string literals, and an accessor @@ -316,7 +322,7 @@ Grouped by what each one constrains. runtime: `Object.hasOwn(encodings, x)` is the only test the published surface offered and it narrows nothing, so `isEncodingName()` is exported beside `isCommandName()` and `isErrorName()`, which serve their own tables that way. Rejected: a `Result` signature on all three, which costs - every typed consumer a narrow forever — goal 6, and the tag is the last cheap chance to spend it — + every typed consumer a narrow forever — goal 7, and the tag is the last cheap chance to spend it — to guard a state the compiler refuses. Where the domain really is open the check is already there: `sendSms()` takes its options as `unknown` and refuses `encoding` by name, which is what a caller without types gets. `smppTime.encode()` is where that reasoning lands the other way and is recorded @@ -421,13 +427,13 @@ Grouped by what each one constrains. the suite ran took the 0x40 this library sends on a concatenated segment, but Route Mobile and Kaleyra both document `esm_class` 0x43 for one, and a caller facing either had to hand-build every segment through `send()` — giving up the split, the per-segment ids, the send window and the - receipt merge, which is what goal 6 means by beating "the application can do this itself". The four + receipt merge, which is what goal 7 means by beating "the application can do this itself". The four modes of SMPP 3.4 5.2.12 are a `MESSAGING_MODE` constant group and the option takes one of their names, so 0x43 is a composition this library makes rather than a value a caller states, and the UDH indicator a segment carrying a header needs cannot be cleared by anything the option can express. It takes three of those four: 2.10.3 carries transaction mode on `data_sm` alone, and none goes out of here, so `FORWARD` stays in the group that mirrors the spec table and `sendSms()` refuses it by - that reason rather than as an unknown name — a mode this library cannot deliver is a promise goal 6 + that reason rather than as an unknown name — a mode this library cannot deliver is a promise goal 7 will not let it make. `DATAGRAM` with `dlr: true` is refused on the same footing: 2.10.2 defines the report away, so arming `DlrMerger` for one is goal 2's wrong answer, where the mode alone and a report under any other mode both go out untouched. Those three names left `ESM_CLASS`, where they @@ -484,7 +490,7 @@ Grouped by what each one constrains. `encodingByDataCoding()` reads the alphabet off that same test rather than repeating the group masks beside it. It is exported for the reason `concatOf()` is — an application that needs a class other than 0 would otherwise rewrite the read this fixed. Rejected: a `messageClass` field on the - `sms` event, which pays goal 6 for three classes nothing here acts on, where the boolean the + `sms` event, which pays goal 7 for three classes nothing here acts on, where the boolean the application already had covers the one it does. Compressed text is out of scope and stays out — nothing here implements 3GPP TS 23.042, so a compressed body reaches the application as whatever its declared alphabet makes of it — but bit 5 does not move the class bits, so 0x30 is read as @@ -666,14 +672,14 @@ Grouped by what each one constrains. returned `42 44 46` — `"BDF"` — reported as built, while a string `message_payload` was cut to its low octets whatever `data_coding` said. Goal 2 owns it, as it owns the `sendSms()` guard above. The line falls at the string: a `Buffer` is octets the caller already chose and goes out as given - under any `data_coding`, which is what keeps goal 6's escape hatch open — the raw UDH, 8-bit binary + under any `data_coding`, which is what keeps goal 7's escape hatch open — the raw UDH, 8-bit binary and deliberately malformed bodies `interop-tests/` builds are all still buildable — and a string with no `data_coding` is untouched, detection carrying every character it was picked for. The guard is `unencodable()` again rather than a second reading, and `unencodableText()` is the character, its code point and its index said once for both refusals — unexported where `unencodable()` is published, since wording `{ char, index }` into a sentence rewrites no read a caller would get wrong, where asking the codec is, and publishing it would freeze this library's - error prose as API for an application whose own refusal should read like itself. Goal 6, from the + error prose as API for an application whose own refusal should read like itself. Goal 7, from the architecture review of [#99](https://github.com/larvit/larvitsmpp/pull/99), 2026-09-09. It is reached through `encodeBody()` in `message.ts`, which is where the `data_coding`-to-text pair already lives: @@ -828,7 +834,7 @@ Grouped by what each one constrains. dropped with the link — is traffic the peer will not send again, so each one reaches `sessionError` as well as the log. Rejected there: an exported `MessageLostError` carrying the group, on the `PduRefusedError` pattern — no `sms` ever fired for that group, so there is nothing in it the - application could act on, and goal 6 does not buy a second exported class to make a count + application could act on, and goal 7 does not buy a second exported class to make a count distinguishable. Accepted: a completing segment whose own answer the socket would not carry still reaches the application, because the message is whole and correct and the failed answer is on `sessionError` — a peer that re-sends after the drop is the smaller risk than dropping a message @@ -840,7 +846,7 @@ Grouped by what each one constrains. of the multipart change: `server()` filled the session's only `onRequest` slot, so the escape hatch the error above names was reachable only by hand-wiring a `Session` over a raw socket, giving up bind acceptance, `authenticate`, the session set and the drain `close()` runs over it — which is - what goal 6 means by beating "the application can do this itself". What the library verifies is the + what goal 7 means by beating "the application can do this itself". What the library verifies is the ordering rather than the hook's honesty about answering: the hook is consulted only for a non-bind request on a session already bound, so no bind — a second one on a live session included — and nothing a peer sends before one can be intercepted however the hook is written. One diff --git a/todo.md b/todo.md index 1c46646..3214429 100644 --- a/todo.md +++ b/todo.md @@ -9,7 +9,8 @@ govern it, and nothing here is a source anything else may cite. ## Status The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18 -to 26. What is left is release work and a few things worth adding before or after 0.5.0. +to 26, and 0.5.0 is on npm. What is left is housekeeping around the release, a few things worth +adding, and the gaps a comparison with other SMPP libraries found. ## The agreed API @@ -97,7 +98,7 @@ of it and the place issues are filed. Maintainer's calls, 2026-09-13 and 2026-09 - [x] 0.5.0 rather than 1.0.0, while usage is this low. Maintainer's call, 2026-09-14. - [x] `NPM_TOKEN`, which `.gitea/workflows/release.yaml` needs, is a Gitea organization secret. -- [ ] Tag `v0.5.0` on Gitea to publish. The first publish creates `@larvit/smpp` on npm, provided the +- [x] Tag `v0.5.0` on Gitea to publish. The first publish creates `@larvit/smpp` on npm, provided the token can publish under `@larvit`. - [ ] `npm deprecate larvitsmpp` pointing at `@larvit/smpp`. Maintainer's call to run it; not something CI should do. @@ -242,9 +243,6 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14. end to end. The interop suite is the natural place. - [ ] **Move to TypeScript 7** once `typescript-eslint` supports it; `renovate.json` pins TypeScript below 6.1 for exactly that reason. -- [ ] **Coverage reporting.** `node --test --experimental-test-coverage` works today; nothing - publishes the numbers. - - [ ] **An `onReceipt` hook.** Receipt text is only loosely specified and operators disagree on it, but `dlrFromPdu()` is wired into `IncomingRequests` with no seam of its own: an application facing a format we do not parse has to take the whole PDU on `onRequest` and reimplement the @@ -252,18 +250,167 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14. Mirror the `onRequest` seam — return a `Dlr` to own the receipt, `undefined` to fall through to the built-in parser. +## Gaps against other SMPP libraries + +From comparing 0.5.0 with `smpp`, `@semyonf/smpp`, `@leissner/node-red-smpp`, `node-smpp-next`, +`smpp-js-sdk`, `smppjs`, cloudhopper-smpp, jsmpp, go-smpp, Kannel, Jasmin and php-smpp, 2026-09-14. +Each lands under AGENTS.md goal 6: an option or a hook, with the call that passes none unchanged. + +### Sending + +- [ ] **A limiter hook, with a messages-per-second cap built on it.** Maintainer's call, 2026-09-14, + reversing the earlier decline: Kannel, Jasmin, go-smpp and `smpp-js-sdk` all limit throughput. + Count PDUs, not `sendSms()` calls — a long message is one `submit_sm` per segment and the + operator counts those, which an application wrapping `sendSms()` cannot see. The hook is where an + account-wide limit lives: a bucket the application holds across sessions or processes, which + goal 8 keeps out of here. The built-in cap is the per-session case; the default stays uncapped. + Open: the hook's shape (a wait that resolves when a PDU may go, cut short by the send's + `signal`), which requests it gates — messages, never `enquire_link`, `unbind` or a response — and + whether its wait counts against `responseTimeout`. + +- [ ] **Back off and resend on `ESME_RTHROTTLED`.** The SMSC refused the PDU, so resending cannot + duplicate it and goal 2 holds, and the retry needs nothing wider than the session. Needs a + decision: on by default with a bounded budget, as goal 5 suggests, and whether `ESME_RMSGQFUL` + counts too. A throttled answer is also what a limiter hook wants to hear about. + +- [ ] **`sendSms()` takes the rest of `submit_sm`.** `service_type`, `priority_flag`, `protocol_id`, + `replace_if_present_flag` and TLVs on every segment, and `registered_delivery` beyond final + receipts: on failure only, and intermediate notifications. Today each needs `send()`, which gives + up splitting, the alphabet checks and receipt merging. TLVs are the common case: India's DLT + rules put `PE_ID` (0x1400) and `TEMPLATE_ID` (0x1401) on every `submit_sm`, and USSD rides on + `ussd_service_op`. Refuse a TLV the send composes itself (`sar_*`, `message_payload`). Open: one + spelling for receipts, since `dlr: true` and a raw `registered_delivery` could disagree, and + whether goal 4's rule on optional parameters binds a TLV the caller named. + +- [ ] **Choose how a long message is spelled on the wire.** Only an 8-bit UDH reference goes out + (`message.ts`), though the reader takes a 16-bit UDH, `sar_*` and `message_payload` alike. Some + SMSCs take only `sar_*` or `message_payload`; php-smpp offers all three. A 16-bit reference also + makes a collision rarer: the 8-bit one wraps every 255 sends on a session. + +- [ ] **Failover across SMSC hosts.** `client()` takes one `host` and `port`; Kannel, Jasmin and + php-smpp take several. Asked 2026-09-14 whether this needs state wider than a session: a list the + reconnect loop walks holds only which host the one session is on, so goal 8 does not decline it, + unlike a pool (Declined). Needs a decision: in order or round robin, and when to try the first + host again. + +### The server + +- [ ] **`sendSms()` on a `server()` session sends `submit_sm` toward the ESME.** Kannel answers + `ESME_RINVCMDID` (`interop-tests/kannel.test.ts`, "MO to Kannel"), and goal 1 says that PDU never + goes out. A server has no other way to send an MO message either: `sendMo()` in that test builds + one from `submitSmParams()` and `ConcatReference`, neither exported. Choosing `deliver_sm` by + `linkEnd` gives MO messages the splitting and checks, keeps one method for one goal, and refuses + the options 3.4 has `deliver_sm` leave empty (`scheduleDeliveryTime`, `validityPeriod`). + +- [ ] **Error TLVs on a response this library builds.** `buildBody()` in `pdu.ts` writes no body for + any non-zero status, so a server cannot answer a `data_sm` with `delivery_failure_reason`, + `network_error_code` or `additional_status_info_text`, and a 5.0 peer gets none of its error TLVs. + 3.4 omits the body on error for `submit_sm_resp` by name; read each response's section before + widening it. Reading needs nothing: an error response carrying a body already parses. + +- [ ] **PROXY protocol on `server()`.** Behind HAProxy or an AWS NLB every session's remote address is + the balancer's, so `authenticate` cannot allow-list by IP and logs name the wrong peer. v1 is + text; v2 is binary and the only one an NLB sends. `smpp` accepts v1 from anyone; accept either + only from addresses the option names. + +- [ ] **`outbind`.** In the command table, handled nowhere: a client cannot take an SMSC's `outbind` + and bind back, and `server()` cannot send one. Rare; take it on with a peer that uses it. + +- [ ] **Register vendor-specific commands.** 3.4 reserves `command_id` `0x00010200`–`0x000102FF` for + SMSC vendors; today one arrives as a `PduRefusedError`. `smpp` has `addCommand()`. The same + shape question as registering an encoding. + +### Encodings + +- [ ] **Register a custom encoding.** Maintainer's ask, 2026-09-14. `EncodingName` is a closed union + of three (`defs/encodings.ts`). An entry needs a name, a `data_coding`, `encode`, `decode`, + `match`, whether `detect()` may pick it, and enough for `splitMessage()` to budget a segment + without halving a character. Take encodings as a client or server option rather than mutating a + module table as `smpp` does, so two sessions in one process cannot disagree about a name. A taken + name is an `err`. Settle `consts.ENCODING`'s names first. + +- [ ] **The alphabets SMPP 3.4 names that no encoding carries.** `consts.ENCODING` lists the + `data_coding` ids (5.2.19); only `ASCII`, `LATIN1` and `UCS2` can be sent. Those with a published + definition, and what each costs: + - 0x01 IA5 (ITU-T T.50, ASCII in practice): trivial. + - 0x06 ISO-8859-5 (Cyrillic) and 0x07 ISO-8859-8 (Hebrew): 96-entry tables. + - 0x05 JIS X 0208, 0x0D JIS X 0212, 0x0A ISO-2022-JP and 0x0E KS C 5601: two-octet sets. + `TextDecoder` reads them through ICU — EUC-JP and EUC-KR once each octet's high bit is set, + `iso-2022-jp` as is; checked for JIS X 0208 and KS C 5601 on Node 24.18.0. Nothing built in + encodes them, so ship tables or build the reverse map on first use by decoding the 94×94 grid. + A Node without full ICU throws from `new TextDecoder()`, which hard rule 1 wraps into an `err`. + - 0x09 pictogram has no published definition; leave it out. + +- [ ] **GSM 7-bit national language shift tables.** 3GPP TS 23.038 defines them for Turkish, Spanish + (single shift only), Portuguese and ten Indian languages — Bengali, Gujarati, Hindi, Kannada, + Malayalam, Oriya, Punjabi, Tamil, Telugu and Urdu — selected per message by UDH elements 0x25 + (locking) and 0x24 (single). They keep that text near GSM's segment size instead of UCS2's 67 + characters. Reading means honouring those elements in `decodeMessage()`; sending means `detect()` + picking a table, with each element's 3 octets off the segment budget. `smpp` has Turkish, Spanish + and Portuguese, used only when the caller writes the UDH. + +- [ ] **Packed GSM 7-bit, opt-in.** Everything goes out unpacked, SMPP's convention (AGENTS.md, "GSM + 7-bit is sent unpacked"); go-smpp carries a packed codec for SMSCs that want septets. Find an SMSC + that needs it before building it. + +- [ ] **`consts.ENCODING` spells five alphabets twice.** `CYRILLIC`/`ISO_8859_5`, + `HEBREW`/`ISO_8859_8`, `JIS`/`X_0208_1990`, `EXTENDED_KANJI_JIS`/`X_0212_1990` and + `LATIN1`/`ISO_8859_1`; `FLASH` is a message class, not an alphabet. One name each before + registration starts taking names. A breaking change to an export. + +### Observability + +- [ ] **Metrics.** Inbound traffic has `data`, `incomingPdu` and `incomingPduObj`; outbound has no + event, and nothing counts requests in flight, queued for a window slot, waiting for a link, or + unanswered. `smpp` and `@semyonf/smpp` emit `metrics`; cloudhopper keeps per-session counters. An + `outgoingPdu`/`outgoingPduObj` pair mirrors the inbound events; the counters can be one read-only + snapshot, read from the owner of each count rather than a second tally that can drift. + +### Packaging, tests and CI + +- [ ] **The source maps point at files the package does not ship.** `sourceMap` and `declarationMap` + write maps whose `sources` are `../src/*.ts`, and `files` publishes only `dist`, so 225 KB of the + 555 KB package leads nowhere. Add `src` to `files`, which makes the maps work — go to definition + lands in the TypeScript — or drop both maps from the build. `inlineSources` would fix only the + `.js.map` files. + +- [ ] **A coverage report and a floor in the gate.** `node --test --experimental-test-coverage + --test-coverage-include='src/**' test/*.test.ts` on Node 24.18.0, 2026-09-14: 98.77% lines, + 93.85% branches, 98.28% functions. Gate at 98, 93 and 98 with `--test-coverage-lines`, + `--test-coverage-branches` and `--test-coverage-functions`, which Node 22 and later take — a job + of its own on 24, since the matrix runs compiled JavaScript — and add it to `main`'s required + checks. Raise the floor as coverage rises; never lower it. + +- [ ] **Tests on macOS and Windows.** GitHub's hosted `macos-*` and `windows-*` runners are free for + public repositories and `actions/setup-node` runs on both, so the job is a `.github/workflows` + file with a matrix. It cannot gate: pull requests and required checks live on Gitea, which has no + such runners without a self-hosted machine of each, so on the mirror it reports after merge. It + runs Node on the runner, as the Ubuntu jobs already do — GitHub's macOS runners have no Docker + and its Windows runners run no Linux containers — so goal 9 needs no new exception. On Windows + the scripts' `*.test.*` globs reach Node unexpanded, which Node 21 and later expand themselves; + Node 18 and 20 cannot run there as the scripts stand. Waits for "Retire the GitHub repository". + +- [ ] **Mutation testing.** `@stryker-mutator/tap-runner` runs `node:test` suites and measures whether + a test notices a change, which coverage cannot; `@semyonf/smpp` runs Stryker in CI. The session + suites are timer-heavy, so start with the codec and the encodings. + +- [ ] **A Node-RED node, as a package of its own.** `@leissner/node-red-smpp` is the only SMPP node in + the Node-RED library, and by a read of its source it never parses a receipt and never answers the + SMSC's `enquire_link`. Its UI is a fair list of what operators set. It builds on this package, + never inside it. + ## Declined -- **Merge state surviving a process restart.** Declined by AGENTS.md goal 7, maintainer's call, +- **Merge state surviving a process restart.** Declined by AGENTS.md goal 8, maintainer's call, 2026-09-02. A restart loses every incomplete receipt group and a peer has no reason to resend one it already had answered, so the loss is real — but surviving it means handing the application the merge state to persist, which the scope floor covers as squarely as holding the state here would, and - which publishes the shape of `DlrMerger`'s groups against goal 6. Nothing is foreclosed: the seam + which publishes the shape of `DlrMerger`'s groups against goal 7. Nothing is foreclosed: the seam can still be added after 0.5.0 as a minor. -- **Throughput throttling — a TPS cap, and backing off on `ESME_RTHROTTLED`.** Declined by AGENTS.md - goal 7: an operator's rate limit is scoped to the account, while the widest thing this library owns - is a session, so a bucket here cannot see a second process binding the same account and is wrong in - exactly the case it exists for. `sendSms()` surfaces `ESME_RTHROTTLED` to the caller instead, and - `maxOutstanding` stays — a window slot frees on the peer's next response, which is self-limiting in - a way a rate ceiling is not. +- **A pool of sessions.** Declined by AGENTS.md goal 8, maintainer's call, 2026-09-14: sends shared + across several sessions need state wider than any one of them. `node-smpp-next`'s `createPool()` is + the example. An application can run several clients and choose between them. + +- **CommonJS.** ESM only, maintainer's call reaffirmed 2026-09-14, though `node-smpp-next` ships both. + `require()` of an ES module works unflagged from Node 20.19 and 22.12.