Plan 0.6.0 as the quality cut the comprehension panel and architecture review ask for
Test / lint (pull_request) Successful in 22s
Test / test (18) (pull_request) Successful in 30s
Test / test (20) (pull_request) Successful in 29s
Test / test (22) (pull_request) Successful in 30s
Test / test (24) (pull_request) Successful in 29s
Test / test (26) (pull_request) Successful in 30s
Mirror / push (push) Has been cancelled

This commit was merged in pull request #13.
This commit is contained in:
2026-09-20 22:42:07 +02:00
parent 92fbd6a9f2
commit 36d32591c3
2 changed files with 193 additions and 13 deletions
+5 -5
View File
@@ -125,7 +125,7 @@ rule and an index of the titles below.
phase: SMPP 3.4 5.3.2.32 makes the TLV the alternative for a body the mandatory field cannot phase: SMPP 3.4 5.3.2.32 makes the TLV the alternative for a body the mandatory field cannot
carry, several SMSCs use it, and Jasmin relays one faithfully — reading `short_message` alone carry, several SMSCs use it, and Jasmin relays one faithfully — reading `short_message` alone
handed the application an empty message handed the application an empty message
([interop-tests/findings/03-jasmin.md](interop-tests/findings/03-jasmin.md)). `messageOctets()` is ([interop-tests/findings/03-jasmin.md](../interop-tests/findings/03-jasmin.md)). `messageOctets()` is
the single answer to where a body is, so the message path, the reassembler and `dlrFromPdu()` the single answer to where a body is, so the message path, the reassembler and `dlrFromPdu()`
cannot disagree about it, and `esm_class` still says whether that body starts with a UDH wherever cannot disagree about it, and `esm_class` still says whether that body starts with a UDH wherever
it was carried, which leaves concatenation reading exactly as before. Filling both contradicts the it was carried, which leaves concatenation reading exactly as before. Filling both contradicts the
@@ -142,7 +142,7 @@ rule and an index of the titles below.
`sar_msg_ref_num`/`sar_total_segments`/`sar_segment_seqnum` the other way to say what a UDH says, `sar_msg_ref_num`/`sar_total_segments`/`sar_segment_seqnum` the other way to say what a UDH says,
Jasmin documents it as its own segmentation and jsmpp writes it, and reading the UDH alone handed Jasmin documents it as its own segmentation and jsmpp writes it, and reading the UDH alone handed
the application one `sms` per fragment the application one `sms` per fragment
([interop-tests/findings/05-java-clients.md](interop-tests/findings/05-java-clients.md)). ([interop-tests/findings/05-java-clients.md](../interop-tests/findings/05-java-clients.md)).
`concatOf()` is the single answer to how a PDU says it is a segment, as `messageOctets()` is to `concatOf()` is the single answer to how a PDU says it is a segment, as `messageOctets()` is to
where a body is, and both are exported for the same reason: an application on the low-level where a body is, and both are exported for the same reason: an application on the low-level
surfaces would otherwise rewrite the read this fixed. It carries the spelling beside the surfaces would otherwise rewrite the read this fixed. It carries the spelling beside the
@@ -224,7 +224,7 @@ rule and an index of the titles below.
ME-, SIM- and TE-specific classes immediate display and missed the 0xF0 group entirely — the only ME-, SIM- and TE-specific classes immediate display and missed the 0xF0 group entirely — the only
one SMPP 3.4 5.2.19 names, since it marks 0x0F to 0xBF reserved and hands 0xF0 to 0xFF to GSM one SMPP 3.4 5.2.19 names, since it marks 0x0F to 0xBF reserved and hands 0xF0 to 0xFF to GSM
03.38, and the one SMPPSim demonstrated 03.38, and the one SMPPSim demonstrated
([interop-tests/findings/02-smppsim.md](interop-tests/findings/02-smppsim.md), C17). ([interop-tests/findings/02-smppsim.md](../interop-tests/findings/02-smppsim.md), C17).
`messageClassOf()` is the single answer to whether a `data_coding` carries a class and which, as `messageClassOf()` is the single answer to whether a `data_coding` carries a class and which, as
`concatOf()` is to how a PDU says it is a segment: 03.38 section 4 puts the class in bits 1-0, `concatOf()` is to how a PDU says it is a segment: 03.38 section 4 puts the class in bits 1-0,
carried where bit 4 says so in every group below 0x80 and always in the 0xF0 group, and carried where bit 4 says so in every group below 0x80 and always in the 0xF0 group, and
@@ -326,7 +326,7 @@ rule and an index of the titles below.
Java-client interoperability phase: accepting any parse that merely did not error answered Java-client interoperability phase: accepting any parse that merely did not error answered
`ESME_ROK` to a `deliver_sm` whose three trailing octets were never read, dropping the `ESME_ROK` to a `deliver_sm` whose three trailing octets were never read, dropping the
`receipted_message_id` that makes a receipt a receipt `receipted_message_id` that makes a receipt a receipt
([interop-tests/findings/05-java-clients.md](interop-tests/findings/05-java-clients.md)). Goal 2 ([interop-tests/findings/05-java-clients.md](../interop-tests/findings/05-java-clients.md)). Goal 2
settles it against goal 3: octets this codec cannot name are a PDU it did not read, so a region settles it against goal 3: octets this codec cannot name are a PDU it did not read, so a region
that does not end on `command_length` — the padded read included — is refused with the `tlvs` that does not end on `command_length` — the padded read included — is refused with the `tlvs`
reason and `ESME_RINVTLVSTREAM` a truncated TLV value already gets. What the rule costs is paid reason and `ESME_RINVTLVSTREAM` a truncated TLV value already gets. What the rule costs is paid
@@ -574,7 +574,7 @@ rule and an index of the titles below.
Jasmin interoperability phase: Jasmin dispatches one `submit_sm` per connector at a time and will Jasmin interoperability phase: Jasmin dispatches one `submit_sm` per connector at a time and will
not send segment 2 until segment 1 is answered, so holding a group unanswered until it was whole not send segment 2 until segment 1 is answered, so holding a group unanswered until it was whole
deadlocked every multi-segment message against a production gateway deadlocked every multi-segment message against a production gateway
([interop-tests/findings/03-jasmin.md](interop-tests/findings/03-jasmin.md)). Goal 1 has the answer ([interop-tests/findings/03-jasmin.md](../interop-tests/findings/03-jasmin.md)). Goal 1 has the answer
a real SMSC gives — one `message_id` per `submit_sm`, immediately — so the group's id base is a real SMSC gives — one `message_id` per `submit_sm`, immediately — so the group's id base is
generated when it opens and each segment is answered `<base>-<n>`, the notation `sms-id.ts` owns generated when it opens and each segment is answered `<base>-<n>`, the notation `sms-id.ts` owns
and `DlrMerger` reads back. The id is therefore fixed by the first segment, which is why an `smsId` and `DlrMerger` reads back. The id is therefore fixed by the first segment, which is why an `smsId`
+188 -8
View File
@@ -9,8 +9,9 @@ govern it, and nothing here is a source anything else may cite.
## Status ## Status
The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18 The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18
to 26, and 0.5.0 is on npm. What is left is housekeeping around the release, a few things worth to 26, and 0.5.0 is on npm. 0.6.0 is next, and it is a quality cut rather than a feature one — the
adding, and the gaps a comparison with other SMPP libraries found. comprehension gate and the defects under [0.6.0](#060) come first, then the gaps a comparison with
other SMPP libraries found.
## The agreed API ## The agreed API
@@ -177,6 +178,191 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14.
- [x] [#8](https://github.com/larvit/larvitsmpp/issues/8) The socket's remote host and port on log - [x] [#8](https://github.com/larvit/larvitsmpp/issues/8) The socket's remote host and port on log
messages: under Worth doing, not blocking. Maintainer's call, 2026-09-14. messages: under Worth doing, not blocking. Maintainer's call, 2026-09-14.
## 0.6.0
A nine-reader comprehension panel read the whole project on 2026-09-20 and scored it 7 overall,
mean 6.8. Navigation (7–8) capped nobody. **Locality capped every unit reader at 5–6 and Shape
capped both architects at 6**, and those two are what this release lifts. The gate is 7 on all four
dimensions, higher where it is cheap. Maintainer's call, 2026-09-20. A systems-architect review the
same day returned ALIGN with one blocking-severity finding, which is the first item under Locality
and is also what the panel ranked hardest — two methods, one answer.
### Correctness, ahead of everything below
- [ ] **Read C-Octet Strings as `latin1`, so an address survives the wire.** `defs/types.ts` reads
with `toString('ascii')` at four sites and writes with `write(text, 'ascii')` at three. Node
masks bit 7 when decoding and not when encoding, so the codec writes `0xE9` and reads back
`0x69`: an inbound `source_addr` of `Kaffeé` reaches the application as `Kaffei`, with no raw
escape hatch as `short_message` has in `shortMessageOctets`. Affects `source_addr`,
`destination_addr`, `system_id`, `message_id`, `service_type` and the cstring TLVs, and makes
`objToPdu(pduToObj(x))` non-idempotent for them. Goals 1 and 3. Verified in the container:
`Buffer.from([0xE9]).toString('ascii')` is `'i'`.
- [ ] **Answer `alert_notification` and `outbind` by not answering them.** Both are response-less in
SMPP 3.4, both fall through `route()`'s default into `unhandled()`, which calls
`sendReturn(pduObj, 'ESME_RINVCMDID')`; `pduReturn()` then finds no response command, and the
failure reaches the application as `sessionError` on every occurrence. `alert_notification`
appears nowhere in `src/` but `defs/commands.ts`. One case arm each: log and return.
- [ ] **Range-check `maxOctets` with its five siblings.** `limitsOf()` in `session-options.ts`
covers `idleTimeout`, `maxOutstanding`, `maxReassembly`, `reassemblyTimeout`, `responseTimeout`
and `shutdownTimeout`; `maxOctets` is documented, consumed by `Reassembler`, and absent from
both that list and `CheckableOptions`. `server({ maxOctets: 0 })` starts, then refuses every
multipart message and reports each as lost traffic.
- [ ] **Read `multiple` in `parseTlvs()` and `writeTlvs()`, or delete it and `tlvMap`.** Five TLVs
declare `multiple: true` (`callback_num`, `callback_num_atag`, `callback_num_pres_ind`,
`broadcast_area_identifier`, `broadcast_error_status`) and nothing reads it; `parseTlvs()` keys
by tag name, so a peer sending two `callback_num` TLVs silently keeps the last. `tlvMap` on
`broadcast_sm_resp` is declared, set once and read nowhere. This is the "Dormant filters" row
of the 0.4.0 defect table in a new spelling — metadata that reads as a guarantee.
- [ ] **Arm the merge for the segments the SMSC did take, or say why not.** `collectSent()` sets
`failure` if any segment errored, including the `UnansweredError` a mid-send drop produces, and
`session.ts` only calls `dlrMerger.expect(sent.smsIds)` when `!sent.err`. So a link drop during
a multipart send leaves per-segment `dlr` events firing while `messageDlr` never can, traced
only by one `debug` line. `session-extras.test.ts` has the adjacent case — a drop *after* the
send — and not this one. If goal 2 forbids reporting on a message we cannot fully account for,
that is the answer; it is stated in no file today either way.
### Locality — 5–6 today, and the gate is 7
- [ ] **Give `IncomingRequests` a port instead of the `Session` it drives.** It holds its owner and
calls eight members of it 18 times, including `this.session.close()` on an inbound `unbind` —
a collaborator ending its owner's life. `OutgoingRequests` is the mirror half of the same
boundary and takes no session at all. AGENTS.md's "Nothing reaches back up" is false because of
this, and `docs/decisions.md` already states the rule under The session's life: "a collaborator
that has to ask does not own its decision". It is also the missing test seam — inbound routing,
reassembly dispatch, `onRequest` ordering and bind-direction refusal have no unit test because
the class cannot be built without a live socket. Carry the eight members as `IncomingDeps`,
exactly as `sendPastDrain` is carried now. No public surface changes. **Do this before the
store (goal 8), or the back-edge is baked into the store's published interface.**
- [ ] **Route `sms.ts` through its handlers, all of it.** `createSms()` already injects
`handlers.send`, and then reaches `sms.session.sendReturn()`, `sms.session.bindAllows()` and
`sms.session.acceptsOptionalParams()` anyway — two channels to one collaborator. `Sms.session`
stays public as data the application reads. The cheaper half of the item above, and the one
that shows the shape.
- [ ] **Give the held-message protocol one name and one home.** `emitSms()` is the unit 8 of 9
readers named and 4 would least want to modify, and every one proposed the same fix. It runs
five mechanisms in one scope: a hold keyed by array identity, a `working` counter seeded from
`listenerCount('sms')`, a `WeakMap` keyed by the `Sms` object, a `setImmediate`-deferred
release, and a captured `linkGeneration` — with the counter decremented from `session.ts`'s
`captureRejectionSymbol` in another file. A `MessageHold` owning `hold/release/listenerGaveUp`
collapses three files into one readable object. Every way of getting it wrong is silent: a hung
shutdown, or a receipt refused.
- [ ] **Derive `Reassembler`'s octet total instead of maintaining it at five sites.** `this.octets`
and each `group.octets` must agree, adjusted in `collect`, `trim`, `takeOldest`, `sweep` and
`clear`, and `collect()` discovers its own eviction by re-reading the map by identity. Push the
budget into `ExpiringGroups` as a weighed capacity, and have `trim()` report whether the
current group survived. Named by 6 of 9 readers.
- [ ] **Split the two questions `OutgoingRequests.linkDown()` answers.** `Session.drain()` calls it
twice for opposite conclusions — "nothing to drain, success" and "the link died under us,
failure" — and `outgoing-requests.ts` reads it a third way. Two named predicates. Named by 7
of 9 readers, who each reconstructed the ordering by hand.
- [ ] **Name `pastDrain()`'s retry condition and what makes the loop end.** The exit is a
three-term disjunction over two collaborators, whose comment covers the first term only, and
the method is named for what it bypasses. Do not change what it asks: `gate.isUp()` rather than
`linkDown()` is deliberate and recorded.
- [ ] **Replace `resolveBody`'s `settles` boolean with the decision it stands for.** One boolean
chooses both whether to overwrite `data_coding` and which params to read it from, across four
helpers all named some abstraction of "body". Return a named source — `'short_message' |
'payload' | 'caller'` — and branch once. Ranked hardest by three readers and picked by one as
the unit they would least want to touch, because a mistake here does not throw, does not fail
the types, and reaches the peer as somebody's message rendered wrong.
### Shape — 6 today, and the gate is 7
- [ ] **Group `src/` into a second level, and retire whichever record loses.** 34 files on one
plane, where `src/defs/` at 7 proves the shape is known one level down. `docs/decisions.md`
says "`src/` stays flat until a module has to move for another reason. Valid while that map is
what a reader navigates by" — and both architects reported that the map is now AGENTS.md rather
than the tree, which is that premise failing. `todo.md` already carries the opposite
instruction under Worth doing. Two records, opposite answers; one has to go. Do it in the same
change as the `IncomingRequests` port or the imports are rewritten twice.
- [ ] **Split `test/session-extras.test.ts` by the question each block answers.** 3,010 lines, 19
unrelated `describe` blocks whose names are already the file names they should be. With
`session.test.ts` it is 54% of all test code and 84% the size of `src/`. "extras" names neither
a question nor a module — it names the rest — and AGENTS.md's own convention forbids exactly
that. `max-lines` covers `src/**` only, so nothing has stopped it growing.
- [ ] **Collapse the three objects named `defaults`.** `client.ts`, `server.ts` and
`session-options.ts` each export or hold one; `port: 2775` is written twice and the idle
timeout is derived two ways to the same 40 000. "What is the default for X" has three answers
depending on the entrypoint, and nothing fails when they drift. Named by both architects as the
most likely first bug a new contributor ships.
- [ ] **Rename `EncodingName`'s `ASCII` to `GSM7`, with `ASCII` a deprecated alias for one minor.**
It is GSM 03.38, where `$` is 0x02 and `@` is 0x00, and `segmentUnits.ASCII = 153` is a septet
budget under a name that says octets. The 2026-09-09 decision removed `consts.ENCODING.ASCII`
for exactly this reason and left the option's own vocabulary carrying it. Pre-1.0 the minor is
the breaking unit, so this is as cheap as it will ever be, and `todo.md` already requires the
`consts.ENCODING` names settled before the custom-encoding registry — this is the other half.
- [ ] **Split `session-options.ts` into the things it is.** Option types and their validator, the
`SessionEvents` map, and the bind-direction rules (`bindCommands`, `bindTypeFromCommand`,
`standsInFor`, `bindCarries`) are three questions in one file, and the `defaults` table mixes
option defaults with four hard bounds that are not options. Both architects named it as where
the codebase rots first: at 34-wide it is where anything session-shaped lands.
- [ ] **Name the base-versus-segment distinction in the message id types.** `Sms.smsId` is a base,
`sendSms().smsIds[]` are segment ids, `Dlr.smsId` is a segment id and `MessageDlr.smsId` is a
base again — four fields, one type, `string`. The whole multipart receipt mechanism turns on
telling them apart and only `parseSegmentId()` knows.
### Self-sufficiency — 6–7 today, and the gate is 7
- [ ] **Move the one-line facts out of the decision log and back to the code.** Five of nine readers
independently reported being sent to `docs/decisions.md` for a question they hit while reading,
with no link from the code; one counted roughly fifty index redirects. The four worth inlining
as one line each: that `segmentUnits`' three numbers are in two units (septets and octets),
which body settles `data_coding`, that a receipt's body is read as octets whatever its
`data_coding` says, and the `<base>-<n>` id notation. The reasoning stays in the log; the
definition belongs at the code.
- [ ] **Document the two delivery-receipt merge bounds.** `maxDlrMerges` (1000) and
`dlrMergeTimeout` (24 h) are hardcoded, are not options, and appear in no README and no test —
while README states the equivalent held-message bounds explicitly ("Neither bound is an
option"). A sender with more than 1000 concurrent multipart `dlr: true` messages silently
evicts the oldest at `warn`. The inherited architect hit this on the 3am walk.
- [ ] **Add a ten-line SMPP glossary to the README.** Both juniors and the no-domain mid reported
the same largest cost: nothing in the repo says what a PDU, `esm_class`, `data_coding`, TON/NPI
or `submit_sm`-versus-`deliver_sm` are, and the inline spec citations mark a rule without
stating it. One of them put it at a third of their reading time. Four commands, three octets,
one sentence each.
### Doc claims this review falsified
- [ ] **Make "every README example is executed by the suite" true, or stop claiming it.** Goal 9 and
the Done table both promise it; `test/readme.test.ts` transcribes the examples by hand and has
drifted — 15 fenced `javascript` blocks in the README against 10 tests, and the test named "the
documented sending options" passes none of the five options the README's example passes. Read
the fenced blocks at test time and assert each appears verbatim in the executed source, so an
edit to either fails the gate.
- [ ] **Correct AGENTS.md's "Nothing reaches back up".** False while `IncomingRequests` holds a
`Session`: either the first Locality item makes it true, or the sentence names the exception
until it does.
- [ ] **Narrow the `src/defs/*` lint exemption to the four table files.** Its stated reason — "the
spec tables are data: their length tracks the specification, not any complexity" — is false for
`defs/types.ts`, which is 595 lines of wire codec with 25 functions and is the file that parses
hostile input from the network. It carries more over-budget methods than any other file in the
repo, under a suppression written for something else.
- [ ] **Run the interop suite before cutting a minor, and date the claim.** README states
"Interoperable. Tested as a client against Jasmin and SMPPSim, and as a server against Kannel,
jsmpp, Cloudhopper, python-smpplib and php-smpp" in the present tense; `interop-tests/README.md`
is honest that the run was 2026-09-08. Nothing runs the peers on a schedule or before a tag, so
the claim rots silently. Goals 1 and 9.
## Worth doing, not blocking ## Worth doing, not blocking
- [ ] **Cut the three teardown sentences `test/teardown.ts` already says.** Under AGENTS.md's - [ ] **Cut the three teardown sentences `test/teardown.ts` already says.** Under AGENTS.md's
@@ -228,12 +414,6 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14.
each Gitea pull request to GitHub for it to review there. Maintainer's ask, 2026-09-14; not each Gitea pull request to GitHub for it to review there. Maintainer's ask, 2026-09-14; not
started until asked. started until asked.
- [ ] **Group the session's collaborators under `src/session/`.** `session.ts` imports
`dlr-merger`, `incoming-requests`, `link-timers`, `outgoing-requests`, `pdu-transport`,
`reconnect-loop` and `send-sms`, and nothing else does, so the directory would make that
boundary visible. The `OutgoingRequests` extraction this was to be done with landed on
2026-09-01, so it is the remaining half. Raised by review, 2026-09-01.
- [ ] **`leftOf()` and the link gate's own budget are one concept counted twice.** - [ ] **`leftOf()` and the link gate's own budget are one concept counted twice.**
`idle-waiters.ts` reads what is left of a budget as `Math.max(1, deadline - now)`, because 0 `idle-waiters.ts` reads what is left of a budget as `Math.max(1, deadline - now)`, because 0
means "forever" there; `link-gate.ts` runs the same subtraction and calls `<= 0` expired. means "forever" there; `link-gate.ts` runs the same subtraction and calls `<= 0` expired.