Repository navigation
feat(modbus): read a device's coils and registers, by address or by a named map - #1792
Merged
Merged
Conversation
…issue #1723 Ten modules under src/lib/db/providers/industrial/modbus and nine test files under tests/unit/db/modbus. No shared file is touched: the type union, the exhaustive compatibility records, the dialect registries, the connection form, the engine counters, the READMEs and docs/SECURITY.md are all unchanged, and the registration lines are added when the shared-file order reaches this provider. Each rule carries the measurement it rests on, in the module that implements it. The request timer is not an improvement, it is the only thing that ends a stalled read. Measured under Node 24 and Bun 1 against a device made to vanish without sending FIN or RST, with the interface left in place and packets dropped: with setTimeout(3000) the read ended at 3005 ms and 3029 ms; with no timer nothing happened for 143995 ms and the socket still read as ESTABLISHED. The socket state is therefore never consulted for health, and a timeout's wording comes from our own timer rather than from err.code, which never arrives in that case. The map takes the PDU address, counted from zero, and v1 parses no manual reference number. Read from the primary source, Modicon PI-MBUS-300 Rev. J of June 1996: printed page 18 defines 4XXXX as one-based, and pages 58 and 62 define 6XXXXX as zero-based Extended Memory, a different table. The six-digit form tools use, 400001, does not appear in that document at all, so a parser for it would collide silently with the meaning the specification gives six digits. Word and byte order are required per point and have no default, because a wrong order reads back as a plausible number with no exception raised. 64-bit integers decode as bigint, because the measured value is past Number.MAX_SAFE_INTEGER. A connection whose map is absent lists nothing and never throws, and its counts answer unavailable rather than zero. The red schema error toast is raised only in the catch block of use-connection-manager, so a throw would greet every unconfigured connection with an error, and a zero would be a claim about the device that Modbus cannot support. The valve re-reads every outbound frame because the builders are our code and the valve is about everything that is not. Three fixture servers took a write from a client that presented nothing, and over MODBUS/TCP Security a certificate carrying a ReadOnly role wrote as well, so the read-only guarantee is this provider's own. Verification, in this worktree against the repository's own harness: 143 tests pass, tsc reports no error in these files, biome and eslint are clean. Every rule is also mutation tested in a separate worktree: 35 mutations, 35 caught. Two of those mutations exist because they were not caught at first, which showed that an MBAP length test and a pacing test did not bind the rules their names claimed. (cherry picked from commit d798a9c23a426295adda70caf9a4fea2978ba107)
…s on purpose ModbusProvider extends BaseDatabaseProvider and answers every declaration surface. The tree is built from the connection's register map with no device traffic: no Modbus request lists units, tables, registers or names, and the specification calls the mapping between the data model and the device totally vendor specific. The class declares no queryDialect, and that is a gap rather than a choice. The dialect union is itself a registration surface: a member arrives with three records, and docs/ADDING_A_PROVIDER.md:1088 says the build fails until all three exist. Declaring it here before those records exist does not typecheck, measured. The cost is real and is not hidden: a json provider with no dialect of its own is given MongoDB's tab type, so until the registration change lands this provider's console reads as MongoDB. tests/unit/db/modbus/index.test.ts pins the absence, so the registration change cannot land without turning that test red and making someone decide deliberately. Three answers are deliberate rather than incidental. Counts answer unavailable when no map is set and never zero, because with no map this provider does not know how many coils the device has and zero would be a claim about the hardware. Health omits activeConnections rather than sending a fabricated zero, which is the rule that field's own docblock states. The monitoring surfaces answer empty rather than inventing figures a device does not publish. The read console refuses with the reason: the client, the register map, the block planner and the frame reader are in place and tested, and the request parser that turns one JSON document into those calls is the next change. Refusing is better than returning something that looks like a read. Verification in this worktree: 159 tests pass, tsc reports no error in these files, biome and eslint clean. Those checks now run from a gate script rather than by hand, and the gate earned its place immediately: it refused this commit once, for four type errors in the new test file that bun test did not see while reporting 159 passing. (cherry picked from commit 551bbe1bb6b74c986b759a70923aec1dbe4a5597)
…ain Error differently
The first version of the provider threw plain Error for its three refusals: the
read console, the maintenance surface and an unknown point. That was a defect,
found by measuring rather than by review.
The product distinguishes the two. isQueryError in src/lib/db/errors.ts tests
instanceof QueryError, and a reader such as the Kubernetes provider's object
layer turns a QueryError into { unavailable } while letting anything else
propagate as an unexpected failure. So a refusal thrown as a plain Error reaches
the user as a crash instead of as the sentence it was written to carry, and the
sentence is the whole point of refusing rather than returning something empty.
The provider argument of QueryError is left out deliberately. It takes a
DatabaseType, and this type-id is not a member of that union until the
registration change lands; it is filled in there.
Three tests bind it, one per refusal, so the plain Error cannot come back. They
assert the instance rather than the message, because the message is allowed to
change and the type is not.
(cherry picked from commit 96aada8a4019e4ea48d0db260762f7877c6b8dfe)
…d and which already diverged provider-surface.ts declared this provider's capabilities a second time, in a MODBUS_SURFACE object. Nothing in the product read it: the provider declares its capabilities inline in getCapabilities. So its seventeen tests were passengers. They were green whatever the provider did, and they protected a duplicate rather than the product. Measured, and worse than unused. The duplicate declared queryDialect: "modbus" while the real class deliberately declares none, which is the one point the provider documents as a gap. The two had already contradicted each other on exactly the fact the documentation calls out. Worst of the three, and the reason this is a fix and not a tidy-up: index.ts's own docblock pointed a reader at provider-surface.test.ts as the file that binds the gap. A reader following that pointer would have found a test asserting the opposite. The pointer now names index.test.ts, which is the file that actually binds it. What was worth keeping were the two sentences the product has to say out loud, about the read-only guarantee and about revocation not being checked. They move into constants.ts, which index.ts imports, because a sentence nothing imports is a sentence that drifts. The capability assertions are rewritten against the real getCapabilities, so they now fail when the provider changes. 160 tests pass, tsc reports no error in these files, biome and eslint clean, all through the gate script. (cherry picked from commit 7395ec74b7c9259dceee8eac9a3a167ebc82e702)
… was asserted for one type values.ts declares eight scalar types and the tests exercised seven. int64 was the one left out, which meant the rule it shares with uint64, that a 64-bit integer decodes as a bigint because the measured value is past Number.MAX_SAFE_INTEGER, rested on a single case. Measured rather than suspected: turning int64 into a Number left the whole suite green. One case cannot tell a rule from a constant. The replacement reads the same four words two ways and the answers differ by the full unsigned range, -1 against 18446744073709551615, which is the only shape of test that tells the two decoders apart. A second case pins the minimum, where a number would lose the value silently rather than visibly. The scale and offset test stays a single case, and now says why that is enough: the two candidate formulas disagree on this data, 15 against 10.5, and the test asserts the second answer is not produced rather than leaving a reader to work it out. Both are bound by mutations. (cherry picked from commit bfb9c7560d7f80e6071f18f40180201a543f09ad)
…hape rather than its intent The three shared helpers do not treat DB_HTTP_BLOCK_PRIVATE_HOSTS the same way, and that asymmetry is the whole difficulty. Read from egress-policy.ts rather than inferred: assertPublicLiteralHost checks the flag itself and returns early when it is off; assertPublicDnsAnswers checks no flag at all and always enforces; guardedNodeOptions returns an empty object when the flag is off, so the lookup is never installed and the DNS guard is never reached. The flag check therefore lives in the caller, and the only correct use of the DNS guard is to let socketEgress decide whether to hand one back. Calling the DNS guard directly, or installing the lookup unconditionally, refuses every name-addressed device even with the flag off. For this provider that is the common case rather than an edge: a programmable controller sits on a local network and is usually addressed by name. One test exists for that regression alone, and a mutation proves it discriminates: installing the lookup unconditionally fails the name test and leaves the IP literal test green, because a literal never calls the lookup. A fixture of literals only would have left the branch dead and the mutation harmless, so both address forms are present. refusesLinkLocal is a choice whose cost runs the other way here than it does for a broker. True also refuses a device that lost its DHCP lease and self-assigned a 169.254 address, which is a real device an operator may need to read. False leaves the range where a cloud instance's metadata lives reachable. The default is true, the safer end, and the other is an explicit argument so the decision is visible at the call site. Issue #1723 MB-S8 leaves it open. The dial carries its own timer for the reason section 5.1 measured, and replaces it with the request timer once connected: they measure different things, and a dial timer left running would end a healthy connection mid-read. One attempt and no retry, because a read that may have reached a device is never sent again. 171 tests pass through the gate. (cherry picked from commit a1e74c89be60032b70d51b22041f365de3371228)
…aching the network A mutation run found the dial timer unbound: setting it to zero left the whole suite green, while the commit that introduced it said the dial carries its own timer. Measuring why exposed a second, worse fault. The connector seam the tests were meant to use was dead code. The line that makes the socket still called node:net directly, because the edit that introduced the seam did not match after formatting. So the three dial tests were dialling a real blackhole address and passing on the real socket's timeout rather than on ours: green for the wrong reason, which is the failure mode a passing test hides best. The seam is now wired, and the tests supply a socket that announces nothing. Two mutations hold it: zeroing the timer fails all three dial tests, and removing the destroy on timeout fails exactly one, the test that asserts nothing is left half open. The second is the discriminating one, which is what makes the set worth having rather than three tests that fail together for one reason. A dead path is worse than a missing one. A missing path fails; a dead path makes the measurement lie, and the mutation that should have caught it stays harmless because nothing reaches the code it changes. 174 tests pass through the gate. (cherry picked from commit 84971122d5b27bb54b42ac8485df55e57471d334)
…n by scope rather than by shape Two unwrapped paths, one of them caught by this provider's own test and only intermittently, which is the worst way to find anything: a race decided whether the suite was green. Six runs here passed and another session's run did not. The criterion is scope, not type, and it is written in the comment this code already cited: a failure that breaks ONE read is converted so the caller can report that kind alone, and a failure that breaks every read alike rises, because otherwise every surface answers unavailable while the connection looks healthy. A malformed register map breaks every read alike. Measured, six surfaces of six: connect, listContainers, countObjects, listObjects, getHealth and getOverview all parse the same document, so there is no kind left that works. That is not a failed read, it is a connection whose configuration is wrong, so it becomes a DatabaseConfigError and rises. The previous commit made it a QueryError, which fixed the 500 and still dressed a configuration fault as a query failure. A socket that errors or closes has also ended the connection, not one read, so it becomes a ConnectionError carrying the host and port in its own fields rather than in its sentence. A raw ECONNRESET used to reach the caller unwrapped. Both boundaries are selective, and that is tested by identity rather than by class: the shared egress guard's own refusal and an unrelated TypeError come back as the same object, so an error read and re-wrapped on the way out would still fail the test. Converting everything would be as wrong as converting nothing, because a programming mistake dressed as a device problem sends the reader to the device. The scope question is answered in advance for the seven classes not yet on a user path, in a table beside the boundary, with the reason for each. They get a line when their path exists: a conversion for an unreachable class is a branch no test can enter. The flaky test is no longer flaky because its two outcomes are no longer different. 191 tests pass through the gate; seven mutations hold the two boundaries, including one that breaks pass-through identity. (cherry picked from commit 5f05d67955a48f58d28aa8b8d63dd6d807a23cdc)
…omparing a constant with itself Every limit in constants.ts was mutated to see which the suite actually holds. Six are bound. Four were not, and all four failed the same way: the tests held the mechanism and left the number free. UNIT_ID_SERIAL_RESERVED_FROM was tested with units 7 and 250, a sample on either side of the threshold rather than the threshold. Any value between them passes, so moving it to 200 broke nothing. 247 and 248 are the only pair that can pin it. DIAL_TIMEOUT_MS and REQUEST_TIMEOUT_MS were never read: every test that touches a timer injects its own value, so the declared figures were decoration. Both now have a test on the default path, the one that reads them. MAX_TRANSACTIONS_IN_FLIGHT changes no behaviour, because the session's queue serialises regardless. That makes it exactly the kind of value a test has to assert directly: TCP 4.4.1.2 requires a client to publish the figure, so the number itself is the contract, and a separate test shows the queue enforces one independently of it. The dial timeout then exposed a sharper mistake of mine. The first fix asserted armed === DIAL_TIMEOUT_MS, which compares the constant with itself: mutating it to 1 ms armed 1 ms and the test agreed. A number that is a contract has to be written out as a literal, with the measurement that chose it, so both timers now assert 3000 and assert it is above the 2137 ms an absent unit cost through the measured gateway, which is the reason the figure is what it is. Eight constant mutations join the harness. 198 tests pass through the gate. (cherry picked from commit 62a55964b1abce176d5571139e7081d347bb032c)
… is not a bound Six fragmentation and bound cases were run against the frame reader. Four passed on the first attempt and two did not, and the two say something worth recording. The four that passed: byte at a time, a split at every point inside the seven byte header, one frame followed by the first byte of the next, and chunk sizes chosen not to align with frame boundaries. TCP knows nothing of frame boundaries, so a reader is only correct if it is correct for every split. These passed because the partial frame lives in an instance field rather than a local, which is the fault that loses data when a large answer arrives in several segments. The two that failed are the reason this is a fix. The reader joined the whole incoming chunk to its buffer and checked the header afterwards, so five megabytes in one write were held before any rule ran, and the bytes stayed held after the refusal. That is the exact fault measured in modbus-serial and jsmodbus, and the reason this module is ours rather than theirs. A bound that only runs after the body is buffered is not a bound, and the docblock claiming otherwise was wrong about its own code. The reader now takes from a chunk only what a frame can still need. A complete Modbus TCP ADU is 260 bytes, seven of header plus 254 less the unit id, so that is the most it ever holds for an incomplete frame, and the figure is written out with its arithmetic. A refused frame's bytes are dropped rather than kept: holding them serves nothing and holding them is the fault itself. Several frames in one write are served from that chunk by walking an offset, with no copy. Four mutations hold it, including one that moves the partial frame back into a local variable, which is the data-loss shape, and one that changes the retained maximum. 205 tests pass through the gate. (cherry picked from commit 27fa32a436ca3faa1f9fe46eb58601db410fef2f)
The reader clears its buffer when a header is refused, and the test named for that rule asserted only the throw. A reader that kept the refused header passed it unchanged, and the growth test allowed it too because its bound was the header's own seven bytes. Measure the held bytes after a refusal on both refusal paths, and read a well formed answer afterwards: a reader that kept the bytes reads the stale length a second time and never hears a device that recovers. (cherry picked from commit 01d8a114233b579aa59f774b562d2dd50690bae3)
knip is a required check and it named three: the 802 port constant, a re-export of the map error from the object lister, and the span helper. All three were published and unread, so the check was red on this branch while the test suite stayed green. The 802 port stays a fact in the comment beside 502. It becomes a name again when the provider reaches the socket and has two ports to choose between; a test written only to read the constant would assert nothing. Readers of the map error already import it from register-map, including the test for the object lister, so the re-export served no one. The span helper is read only inside the planner. (cherry picked from commit 05e40e4528e9625749c77f0d5d7080e720187138)
Deleting it turned knip green by removing the evidence. The TLS dial exists in client.ts and the provider does not reach the socket yet, so no code is in a position to choose between 502 and 802: the name is waiting for a connection, not left over from one. The gate carries it as a single expected unread export with an end condition. When index.ts imports ./client the allowance goes red and comes out, so the gap cannot outlive the wiring that closes it. (cherry picked from commit 426937b5be8a6cbe9451ebc7141153e281c5959d)
The editor text is one JSON read request in three shapes: named points of the map, one range by address, or the device identification objects. Every shape is settled here, before a socket exists, because a request that cannot be built is text the caller should hear about while they are still looking at it. The schema is strict. A key the parser does not know is refused rather than ignored: ignoring it runs a different read from the one that was written, and the answer is then read as the answer to a question nobody asked. The protocol's bounds are not repeated here. checkRead owns them and this parser calls it, so a widened limit cannot pass in one file while it fails in the other, and the tests for those four rules assert the words checkRead writes. A name is checked against the map because Modbus has no discovery: a name means what the user's map says it means, and the useful moment to say a name is absent is while it is still in hand. Three refusals exist for values that would otherwise be dropped without a word: a count that does not divide by the type's span, an order key with no type to order, and a type on a table that carries bits. (cherry picked from commit 583a2ee959b9b5f73917562545a76eb6cf1148c6)
matchToRequest was written for this, tested on its own, and called by nothing. exchangeOnce resolved frames[0] instead, which gets two cases wrong: a late answer to a request already given up on reads as the answer to this one, and a stray frame arriving in the same chunk as the real answer hides it, because only the first frame was ever looked at. The expectation is now a required option rather than one a caller may omit: an exchange that cannot say what it asked for cannot tell a stray from an answer, so there is no sensible default to leave it at. A frame nobody asked for is dropped and the wait goes on. A frame that claims to answer this request and cannot still fails. The device's own refusal resolves as an exception rather than as an answer, so a caller cannot read an exception code as data. (cherry picked from commit 044950b59a97e950ea94fc1e7df545c750b8d2cf)
The wire resolved with a complete ADU, which means the frame reader had already read the header to know where the frame ended and whoever received those bytes would read the same header again. Two readings of one header is one reading too many, and the second is the one that drifts. Exchange moves to framing, beside Match, because what a frame MEANS is a framing question and both the socket and the session need the type. Match is now Exchange plus the ending that is not an ending: a frame nobody asked for. The session never looks inside an answer, it only carries one, so its tests echo the request back as the answer: what they are about is ordering, timing and pacing. (cherry picked from commit 7c79bd54916abfb1ee33d55600e593ade590fded)
Function code 43, MEI type 14: the protocol's only self-description, and the only read a device answers without a register map. One answer is parsed here and the walk is not. A device may answer in several transactions, and following that chain needs a connection this module does not have. What it owns is the shape of one answer and what an object's bytes mean. Every length the device declares is checked against the bytes that arrived, in three separate ways, because they fail differently: a count that overruns the frame, a length that overruns the frame, and a length past what any frame can carry. Each one would otherwise show a neighbouring object's bytes as this object's value, which a reader cannot tell from data. A value is text only when its bytes really are UTF-8, decided by re-encoding rather than by looking for odd bytes: Buffer's decoder substitutes U+FFFD instead of failing, so bytes that are not UTF-8 come back as a string either way and the only reliable question is whether encoding it again gives the same bytes. The private objects 0x80 to 0xFF carry whatever a vendor chose. The per-object bound is derived from MBAP_LENGTH_MAX rather than written beside it, so it cannot stay still while the frame bound moves. (cherry picked from commit 365d8d726135fc024dcacfa8340f5093a4f233ce)
A payload can carry bytes that read as a perfectly good MBAP header, because every byte of a register is data the device chose. The reader advances by the length the header declares and never looks inside a body, so it is immune by construction, but every test until now used payloads that happened to contain no header-shaped bytes. Three cases now say so: a value carrying a whole header, two frames where the second's header appears verbatim inside the first's body, and the same two split byte by byte. The close after a framing error was held by a stand-in Error, which binds any failure closes and says nothing about a framing error in particular. The real class now travels that path, and the reason for the close and the identity of the error the caller receives are asserted separately, because they are two claims. The 802 port goes back to being a sentence beside 502. Kept as an exported constant it was a published name with no reader; kept as an unexported one it was an unused local eslint names. What stands in production because it is absent is nothing: a connection cannot be made at all yet. It returns as a name in the change that dials, where some code actually chooses between the two ports. (cherry picked from commit 1547b66b256232ff0e6956144d2c15dde5492cb7)
The type-id joins the DatabaseType union, the dialect union and the tab-type union in one change, because none of the three compiles without the records that follow it. Measured rather than listed: adding the id to the unions and reading what the compiler asked for. 33 records across 22 files, of which 36 of the 69 errors were one missing DIALECT_GENERATORS record reported again as implicit-any cascade. The surfaces the compiler does NOT force are the ones that matter most here, and each was found by running the suite rather than by reading a checklist: the factory's switch, whose default throws a string, so a missing case is silent and the provider could never be constructed; the supported-types sentence beside it, pinned by regex; the nine READMEs and their numerals; the outward-facing catalog copy in twelve files, where a numeral summarises a list that has to name the engine; and the census tests that enumerate every engine by hand. The palette gains stone. Every base hue and every -alt step was an engine's by the time this one registered, and of the four greys Tailwind ships that no identity used, stone, neutral and zinc clear every contrast test while gray-800 does not clear AA in light, measured by running the test against all twelve candidates. A warm grey stands furthest from slate, which is OPC UA's, and those two are the industrial pair a reader has to tell apart on one tree. The provider now declares its own dialect, which closes the gap its own test pinned: a json provider with no dialect of its own is given MongoDB's tab type by resolveTabType, so the console read as MongoDB. (cherry picked from commit b2c0177bba572d602fa90935e636fa3315594200)
Eight test files were red on this branch and the gate said green, because the gate ran my own directory and the registration touched forty shared files. The gate now derives its list instead: every test that names the engine. That is what found the rest of this commit. The provider doc did not exist. docs/providers/ holds one file per engine and modbus was the only one missing, which is not a stale number but an absent file, and two censuses were failing on it. The container path was declared and not checked. assertContainerPathShape pairs a declaration with a call, and the declaration alone lets a path of the wrong length reach the map lookup, which then answers no such point and names the wrong mistake. Wiring it turned four of my own tests red at once: they read at the empty path, which the product never sends - the route checks the shape before a provider is reached - so the tests were pointed at a unit path and the guard kept. The seed schema rejected the engine outright. Its enum is kept in step by hand and the file says so: a missing id is not a compile error, it is a seed file the server refuses with invalid enum value for a connection type the product supports. MEASURED: modbus was refused at the type level while mtconnect reached the field checks. The migration generator emitted SQL for it, and two more hand-kept exclusion lists inside the tests had to name it. Four populations carried a total that moves with the fleet: the edit abstainers, the column census, the source census and the maintenance surface. The refusal classes now carry what the reader acts on. Eight of nine fell through as plain Errors, so the user's own bad request arrived with the code reserved for our defect. Request, map, decode and valve are four different next actions and now four different classes, and the valve deliberately stays ours. A union-keyed record holds one direction only. MEASURED on FUNCTION_CODE_OF_TABLE: an extra key typechecked clean while a missing member failed, because the annotation sits on the binding and Object.freeze's return is not a fresh literal. With satisfies inside the call, both directions fail. (cherry picked from commit d854c2f094339a4751d504d842841f8d94767a36)
…t all A test that names only the class cannot tell six refusals apart. MEASURED by making each one carry another's sentence: two survived with every test green, so decodeScalar could refuse an unknown type with the wrong words and nothing said so. Both now assert the sentence a reader acts on. The container path guard was wired this evening and nothing asserted it: removing the call left the suite green. Three tests now hold it, including the control that the shape the tree actually sends is still accepted. The derivation behind the gate's surface stripe grew a second source. Naming the engine finds the tests the registration touched, and misses the ones that must change without ever mentioning it, which is how the numeral dictionary slipped through. It now also takes every test changed on this branch. (cherry picked from commit 1ccfcbeb4b4a26a46e0bef850e43bcbd7d4d5e1a)
The engine was registered, connect() returned successfully, and no byte ever left the process: index.ts imported constants, pdu, register-map and values, and nothing imported client. A registered engine whose connect succeeds and whose reads refuse is worse than an unregistered one, because the showcase line in nine READMEs promises it. read.ts is the step that was missing. Everything it calls already existed and was tested; what did not exist was the order. A request is parsed, the points are planned into as few block reads as the limits allow, each block is one exchange, and the words come back decoded under the orders the map states. query() dials for the read and destroys the socket in finally. Nothing is held between reads, which is the rule connect() already documented: a libmodbus server serving one client at a time left a second client unanswered for 3003 ms while the first stayed open. The request is settled BEFORE the socket, so a refusal the caller can fix costs no connection, and that order is asserted. getHealth() now does what its docblock claimed for a while: a dial and one quantity-1 read at a point the map names. A connect alone is not health and neither is the socket's state; health is an answer, or the absence of one. The measurement is a TCP server written by hand rather than a mock, so the frame reader, the valve, the transaction ids and the decoder all run. It asserts both ends: the bytes the device received are a read for the address the map names, and the rows carry the value those bytes answered with. Two reads in a row both carry transaction id 1, which is the evidence the first socket was really closed. The identification walk is bounded three ways at once, and a walk cut short keeps what it has: the objects already read are real. (cherry picked from commit 68b30fb3adfb818c1e331c9eeb2d377124b403b6)
…wrote NON_SQL_DESTRUCTIVE_VOCABULARY is a registration surface nobody's list held: a type carries a row exactly when readsSqlText says its text is not SQL, and this engine joined that set in the registration without joining this record. The row names NO operation on purpose, in the shape MTConnect and Prometheus use: the encoder builds four read function codes and the identification request and nothing else, so naming an operation would claim this provider can send it. Three censuses move with it and were updated together rather than one at a time: the row count, the engines that decide alone, and the engines whose statements stay out of the AI analysis. Nine of my files were unformatted and no gate looked. biome runs on the host for knip's reason - the container has no linux-arm64 binary - and the stripe covers my own two directories, because the other unformatted files on this branch are inherited and belong to the session whose rebase resolves them. (cherry picked from commit d8d8ce0334899b819fe2192fb2b1a07c70484f07)
…ad as connected Health read one point when the map named one and asked the device nothing when it did not, so a connection whose map is empty answered successfully against any address at all, reachable or not. A plain dial, and deliberately not an identification read. 43/14 is not mandatory: a device that refuses it is one that does not know the function rather than one that is not there, and reporting that as a health failure would blame the wrong side, which is the fault class this provider keeps running into. The dial claims exactly one thing, that the address answered a TCP connection. Nothing is sent and nothing is read. That sentence is in the provider doc rather than in the answer because HealthInfo has no field for it, and a sentence in a size field would be a worse answer than the unknown that is there. Three tests hold it: an open listener passes with no ADU sent, a closed port fails, and a connection WITH a map does not take this path at all, which is what keeps the stronger check from being quietly replaced by the weaker one. (cherry picked from commit b215f4e7f5ffc8262995321a9b528a8898843dd6)
The previous commit went in through a CLOSED gate: the two commands were written on separate lines rather than chained, so the gate's verdict was printed and not read. Two things it had found went in with it. A health test ran with no server and asserted the shape of the answer. The moment health began to dial on an empty map, that test started spending three seconds proving a dial timeout and failing. The claim is about the answer's shape, not about a socket, so it moved to the file that has a device to answer it rather than being weakened where it stood. The new tests were unformatted, which the format stripe added an hour ago caught and which the same unread verdict let through. (cherry picked from commit 672e9314aa951ba2d88231ccc49cb4bb24885e24)
Three sentences cited the standard where the true subject was us. Maintenance refused with "Modbus offers no maintenance operation", which is false about the protocol: function code 08 can clear a device's counters and restart its communications. What is true is that this provider builds four reads and the identification read and nothing else, so the sentence says that and names 08 as the thing it does not build. An undefined exception code was reported as one "which Modbus does not define". That is a claim about the specification and it could not be checked here, because the specification PDF in hand is compressed and its text could not be read. If the code is in fact defined, the sentence blamed the device for a missing row in our own table, so it now says the table is ours. The per-object identification bound is arithmetic of ours, derived from the frame bound rather than printed in the standard, and the sentence presented it as the protocol's number. Measured with a string extractor rather than a line regex: the first pass matched code across line ends and reported 67 sentences, most of which were not sentences. Twelve cite the protocol, and the valve's own line is the control that the scan sees what it claims to scan. (cherry picked from commit 93c1f97ab7970cf39a49b9d7b5da8d57b5ed056e)
…ter that bled Five mutations against what was wired today, and two survived. The order of parse and dial was bound only by accident: the tests that noticed it have no server, so dialling first fails them for the wrong reason, and the one test written for the rule counted ADUs. A dial sends nothing, so an ADU count cannot tell "refused before the socket" from "dialled, then refused" - both answer zero. The device now counts CONNECTIONS, and the accepted read is the control that an accepted request really does dial. Dropping socket.destroy() from the finally broke no test at all. Every read dials its own socket, so a leaked one is invisible to both the transaction ids and the connection count: a second read dials again either way. The device now counts CLOSES, which is the only end of this that can see a socket left open. And the counters themselves were wrong before they were useful. They were module-level and reset per test, and a close event from the previous test's socket arrived after the reset and landed in the next test's count: connections 2, closes 3, with no bug in the provider at all. A late event cannot know which test it belongs to, so each device keeps its own counters now. 97 mutations, 97 caught. (cherry picked from commit 771f53ccc810a33237a60fd0f7a7a5ee931f5459)
connect() parsed the register map and opened nothing, so it returned successfully for an address with no device behind it - MEASURED at 0 ms against a closed port. The test-connection route reports a health failure after a successful connect as a degraded success, so Test Connection answered success: true for a device that does not exist, and the route's own note says the dialog gates its save on that answer. A reader told "connected, but no health data" goes to fix their map when the fault is the address. Both directions measured. Before: a closed port returned successfully. After: it is refused with "The connection to 127.0.0.1:1 failed: connect ECONNREFUSED", which names the address, and an open listener is still accepted. The route's success: false follows from the connectPhase branch I read at :128 and did not execute; what I ran is the provider. A dial and not an identification read: 43/14 is not mandatory, so a device that refuses it is one that does not know the function rather than one that is not there. The socket is closed at once, because what section 4.7 measured as dangerous is HOLDING a connection - a second client went 3003 ms unanswered while the first stayed open - and not opening one twice. The map check still runs first, and a malformed map is refused with no socket opened at all. "An empty map is a normal state" moved to the wired tests with the dial, because proving it now needs an address that answers. (cherry picked from commit 88c654ac2a7884d5e39de693fab5a0238353ab68)
…real route The measurement that justified the dial is now a test rather than a paragraph. Before the dial, an address with no device answered HTTP 200 with success: true, degraded: true and the message "Connected, but this server answered no health data", and the route's own note says the dialog gates its save on that answer. MTConnect answered 503 in the same run, so it was never the route's general behaviour. Measured over the real route handler, not a mock of it: an address with no device is 503 CONNECTION_ERROR naming 127.0.0.1:9, with no success and no degraded flag, both with an empty map and with a one-point map - the two were byte-identical before, which is why both are asserted. An address that answers is a plain 200 with no degraded flag, and the device answers a real read for it. My first run of this measurement was invalid and I noticed from the output rather than from the result: the resolveConnection mock had the wrong signature, the route saw an undefined connection and answered 500 for all three cases. A 500 on every case is not a measurement of anything. (cherry picked from commit 1ab0d88ff12000293d4faa4699b7017ab4a0dd06)
…from
MEASURED, and this was the worse half of the same hole. Against a port that
ACCEPTS a connection and never answers, an empty-map connection answered
{"success":true,"message":"Connection successful"} in 0 to 9 ms, three runs
identical. The dial succeeded because TCP was accepted, and health could not fail
because there was nothing to read. The refused-port case at least said degraded
and named the address; this one carried no hint at all. And an empty map is a NEW
connection's default state, so that was the answer before a user had written a
map, on a wrong port as readily as on a right one.
The repair is an honest answer rather than another probe. No guessed read sits
behind it: a wrong unit id costs 2137 ms behind a gateway, so a timeout there
would blame a device that IS present, and 43/14 is not mandatory either.
connectWarnings and not testConnectionWarnings, because it needs no request - it
reads the connection's own map. Measured rather than assumed: the route collects
it at :140 and returns it as warnings, the hook composes the sentence a user reads
at use-connection-form.ts:344, and the modal renders it at :1979 with an amber
tone rather than a green tick. The route half is executed by these tests; the
hook and modal half was read, and the test says so.
Three cases, because two of them would have been mistaken for a fourth: a silent
port with no map carries the caution, a REAL device with no map carries the SAME
caution - the caution is about the map, not the port - and a real device with a
point carries none at all.
(cherry picked from commit 92d180c6f2d9f9efe7d6226a03d7141829f486ac)
added 18 commits
October 16, 2026 05:33
`describe()` names what the caller wrote, and its last arm answers with the type for a value that is none of null, a list, text or a number. Every case in the table passed one of the four the arms above it catch, so the line was the only one in this file the suite never ran. A boolean reaches it. Measured rather than guessed: the lines above it are hit 11, 37, 45, 77 and 61 times and this one zero, which is what an unreachable arm looks like next to a reachable one.
The walk sliced one byte off the answer before parsing it. `InboundFrame.body`
is already the PDU after the function code, so it starts at the MEI type, which
is where the six-byte head MB 6.21 defines starts and where the parser reads
from. The parser's own sentence says so ("the PDU after the function code") and
`identification.test.ts` builds exactly that shape.
Measured on the wire rather than read off the page. A three-object basic answer
was assembled as MB 6.21 describes it, pushed through the real `FrameReader`,
and both shapes handed to the parser:
frame.body 0e 01 01 00 00 03 00 04 "Acme" 01 04 "AC-1" 02 03 "1.4"
the body itself objects=3 [[0,"Acme"],[1,"AC-1"],[2,"1.4"]] more=false
body.subarray(1) objects=0 [] more=false
One byte along, every field moves: the conformity level is read from More
Follows, More Follows from the next object id, and the object count from the
first object's id.
Two symptoms, one cause, and the fix is measured against both. A device whose
More Follows byte is 0x00 answered into an empty list, so `identify` returned no
rows and no warning - a device that publishes its identification read as a
device that has none. A device sending 0xFF put that byte where the conformity
level is read and the parser threw instead.
Why nothing caught it. There was no `read.test.ts` in this provider: all 44
lines of `readIdentification` sit in the uncovered list the coverage gate
prints, and the live screen run read the four tables but never ran `identify`.
The gap did not merely fail to find the defect, it is what kept it hidden.
The new file drives `runRead` behind a scripted `Exchanger`, with no socket: the
walk's three bounds, the two warnings that keep a half-read list, the refusal
sentences, and the two shapes a range answer comes in. Canary, run twice: with
the slice put back, six cases go red, named in the run - the three walk cases,
the level case, the non-increasing guard and the round bound.
Two `require("node:net")` calls stood where the module's own import already
reaches: `Socket` was imported for its type and taken again at runtime through
`require`. `bun run lint` is a CI gate and ESLint refuses a `require()` import,
so this file failed the lint job with two errors while every other gate on the
branch was green.
Measured, not assumed: `bun run lint` exits 1 before and 0 after, with the two
errors named in the run, and the file's nine cases pass either way - the change
is how the class is reached, not what it does.
`expectationOf` declared its five-field result inline, and those five fields are `Expectation`, field for field, which `framing.ts` already exports and `matchToRequest` already takes. The function exists to produce exactly what that function consumes, and until now only structural typing said so: a reader had to collect the shape out of the middle of a signature and compare it by eye with the interface two modules over. Nothing about the bytes changes. `framing.ts` imports only `constants.ts`, so the type import adds no cycle. A side fact worth recording because it is what sent someone looking: bun's coverage reporter emits line records for the fields of an inline return type, and those lines can never execute, so they sat in the uncovered list against a gate that admits no exemption. Naming the type removes the lines rather than silencing them.
`transactionsInFlight` had no reader anywhere: one match in the whole tree, its own declaration. The field behind it is live - the cap at `MAX_TRANSACTIONS_IN_FLIGHT` reads it, and so do the two lines that raise and lower it - but the public getter over it was never called by the provider, the tests or the end-to-end suite. The test that cares about concurrency measures it from the other side already: the fake wire counts how many exchanges are outstanding at once, which is what the rule is about, and that count does not depend on the session exposing one. Writing a case for an accessor nothing calls would have turned the gate green while leaving the member exactly as dead.
Four surfaces, each uncovered for its own reason rather than by one oversight. The tree's generated requests. The object tree writes a read for whatever was clicked, and a click lands at two depths: a point, whose name the map already holds everything for, and a unit, which has no point to name and so gets the one shape that needs no map. Only the first was ever generated in a test. Both texts now go through `parseReadRequest`, the same function the provider runs them through, so "runs as is" is measured rather than claimed - a generated request that does not parse is a click that opens an editor and refuses on execute. The overview. Every surface case so far handed it a malformed map, which refuses before the answer is built, so the answer itself had never been read. It is assembled from the map because a device publishes none of what an overview asks for, and the two figures a device does not publish are words rather than guessed-at numbers. The second case holds that distinction: a connection with no map reports `unknown`, not `0 points`. The transaction id. The module says an id is never reused on one connection and that the counter wraps at sixteen bits. That is a claim about a full cycle, and a case taking three ids measures the first half of the sentence and none of the second. The cycle is now taken in full: 65535 ids, each once, none of them zero, and the next one after the wrap is the first again. Three endings of an exchange. Besides the timer and an answer, an exchange ends when the device hangs up, when a chunk is refused at the MBAP header, or when the caller lets the wire go. The first two reached the caller through lines no case ran; the third destroys the socket under the wire. Each is driven with a supplied socket, so none of them races a real dial.
"FC 43 with MEI type 14. The only other request this provider can produce." described `buildIdentification` and sat above `expectationOf`, thirty lines higher, where it read as a claim about the expectation reader instead. Moved to the function it is about. Nothing executable changed. Worth its own commit because the class of fault is worth naming: a sentence that is correct, that no gate can see, and that is wrong only about WHERE it stands. The formatter, the linter, the type checker and the coverage gate all pass over it, so the only thing that finds one is somebody reading the lines in order.
Five of the eight line citations this engine's comments carry pointed at the
wrong line, and three of the five were wrong against every tree: the anchors
they name have not moved between main, the branch base and this branch.
use-connection-manager.ts:335-362 the "Schema Error" toast is at 411,
inside the catch at 399; 335-362 holds a
different setSchemaError(null)
use-connection-form.ts:344 cautionSentence is at 408 in all three
ConnectionModal.tsx:1979 an SSH private key textarea; the caution
banner is the testResult.tone === "warning"
arm
ResultsGrid.tsx:736 a comment; autoFocus is at 746
ResultsGrid.tsx:823 blank; onDoubleClick is at 846
The fix names the symbol instead of correcting the number, because the number
is the part that cannot stay true. `connection-fingerprint.ts` already carries
this rule and a test enforces it: that module names `getOrCreateProvider` and
`src/lib/db/factory.ts` with no line number, and the suite asserts
`not.toMatch(/factory\.ts:\d/)` on its source. A citation that survives a
rebase is worth more than one that is exact for a day.
The three citations that are correct are left as they are, measured rather than
assumed: `errors.ts:245`, `kubernetes/objects.ts:348` and `route.ts:146-160`
all say what their comment claims, and no commit in this stack touches any of
those three files, so none of them moves when this branch lands.
MEASURED FIRST, because the trigger was not a design review. Bun's coverage
reporter emits a line record for `export class FrameReader {` that never
credits a hit, and the 100% gate admits no exemption. The control settles what
it is: a byte-for-byte copy of the same class, in the same file, driven by the
same test in the same run, reported 42 where the original reported 0. The body
was never the cause, so no case could ever have closed it. Across a full run,
all 37 raw reports carrying this file read `DA:42,0`, and `merge-lcov.mjs`
takes the maximum per line, so the merge cannot have turned a hit into a zero -
our side was cleared before the tool was blamed. The report is also inconsistent
with itself: it declares `FNF:8 FNH:6` while carrying no `FN` name at all, and
the import on line 21 reads 190 hits.
TRIED AND DID NOT WORK, named so the next hand does not spend the night on
them: an explicit constructor; moving the declaration down one line; splitting
`export` off the declaration; moving the class to the end of the file; moving
it to a file of its own, where every number transferred verbatim; and renaming
it off `FrameReader`, which is the repository's only duplicated `export class`
name - the other is `rabbitmq/amqp/frame-reader.ts` - a strong candidate that
turned out wrong. Ten blank lines at the top shifted every record by exactly
ten, so the line map was live throughout.
THE CHANGE. The reader held one field and two operations: no inheritance, no
`instanceof` test, no second implementation, one `new` in the whole tree. A
class buys none of what it costs here, and the module's other seam,
`matchToRequest`, is already a function. It is now a closure behind
`createFrameReader`. The exported surface is deliberately unchanged:
`FrameReader` is still the type a caller names, now an interface, and
`reader.buffered` is still a value because the returned object keeps the
getter. Call sites change in one way only, from `new FrameReader()` to
`createFrameReader()`.
AND THE OBJECTION, because leaving it out would teach the next hand that we
reshape code to suit a tool: nobody would have asked for this change on its own
merits, and the reason it was made is a defect in the coverage reporter.
Measured on this tree: `framing.ts` has no uncovered line, the gate reads
165842/165842, the suite is 1527 files and 57006 cases with none failing, and
typecheck, format, lint and knip exit 0.
Canary, two rounds, each one putting the class back: `coverage:check` exits 1
naming `framing.ts: 1 line(s) [42]`, and exits 0 at 165842/165842 once the
class is gone again. The first writing of that canary reported two green rounds
and proved nothing, because the shell handed `git checkout` the whole file list
as a single path and the class was never restored; the rewrite counts the
restored declaration before each round and prints every exit code, so a round
that measures nothing cannot read as a round that passed.
…agree The catalog note told a publisher two different things on two adjacent lines. Both began "do not publish the thirty-eight-engine wording against a version that", and then one said "predates Apache Spark" and the next "predates MTConnect", each followed by its own chain of variants. A publisher reading it cannot tell which threshold to apply, and the chain the reader follows decides which body goes to the catalog. The second line was wrong beyond being a duplicate, which is why it is the one that goes. It offered "the thirty-five-engine variant for a release with OPC UA and not MTConnect" and named `pcsc-listing.html` at 0df2b6a as that body. That body carries thirty-four engines, not thirty-five, and the rung skips Apache Spark, which is the engine that sits between OPC UA and MTConnect. Measured by reading the listing at each commit: 0df2b6a lists thirty-four, f447c9b lists thirty-five with OPC UA and no Apache Spark, main at d7ec5b1 lists thirty-six with Apache Spark and no MTConnect. What was missing is this branch's own rung. The count above it moved to thirty-eight when Modbus registered, but the chain still started at thirty-five, so the two steps below the new count had nowhere to be read from. It now starts where the count does: thirty-seven for a release with MTConnect and not Modbus, then thirty-six for one with Apache Spark and not MTConnect, then the chain as it already was. The thirty-seven-engine body carries no commit of its own here on purpose: that release is not out, and naming a branch tip would point a publisher at something no tag carries. The note already leaves a body unnamed where none is published, so this follows what it does rather than inventing a reference.
…wice Adding Modbus to the mailed listing needed eleven characters the 1494-character budget did not have, and the room was taken from the opening sentence: "a SQL IDE to Rancher-managed Kubernetes clusters" became "to Rancher-managed clusters". That left the submitted body saying something different from the copy it is cut down from. `CATALOG_LISTING.md` and `docs/RANCHER.md` both still say "Rancher-managed Kubernetes clusters", and a reader comparing the two would find a difference nobody decided. The room comes from a claim the listing already makes twice instead. The second paragraph said "Runs on your own infrastructure" and the bullet list says "Self-hosted, air-gap friendly: no external services" four lines later, which is the same promise in two places. Dropping the first leaves the promise standing where it is already made, and the sentence that carried it keeps what only it says: who develops and supports the product. Measured: the pasted length goes from 1493 to 1471 against the 1494 limit, so the headroom goes from one character to twenty-three - enough that the next engine does not have to buy its room from a sentence again. The size gate and the copy gates pass, 80 cases over three files.
`yazilanlar` is a Turkish word, and it named the list that collects what the fake socket was asked to write. It is now `written`, which is what the list holds. Worth saying how it survived: a scan for Turkish writing looks for the letters Turkish has and English does not, and this word carries none of them once it is spelled without its diacritics. The repository-wide sweep came back clean on this branch - eleven hits, every one of them deliberate (two maintainer names in the operator bundles, a collation defect documented with `Müşteri` against `Müsteri`, a multi-byte measurement, a Unicode fixture list) - and this identifier was invisible to it. The second pass that found it searched for Turkish words spelled in ASCII instead, and this file was the only place in `src/` or `tests/` carrying one. The rename is seven occurrences in one file, nothing exported, and the file's thirty-five cases pass unchanged.
A Modbus device answers no discovery request, so the register map is the only thing that can name a point - and until now nothing could write one. The provider read `registerMap` off the connection through a cast, no type declared it, and the only places that set it were tests. The dialog's own caution said "until a map names a point there is nothing to read", describing a state a user could not leave. WHERE IT LIVES WAS TREATED AS OPEN AND IS NOT. `keyof DatabaseConnection` is a write list: `types.ts` says so where it explains why `WithTunnelFarEnd` is deliberately NOT a field, and three exhaustive readers answer the one question it asks - `FIELD_OWNERSHIP`, `CONNECTION_RELEVANCE` and `CONNECTION_FIELDS`, on what a user may fill in, send and have stored. A document the user writes is on that list, and length is no argument against it: BigQuery carries a whole key file's JSON today. All three readers named themselves in `tsc` the moment the field appeared, which is the mechanism working rather than a test remembering. Not `password`, which is where BigQuery's key sits, because that one IS a credential. Modbus TCP defines no authentication at all, so there is nothing here to leak, and `secret` would mask the document in the dialog - a document nobody can read is a document nobody can correct. `resolution` in `CONNECTION_RELEVANCE`, where Kubernetes' namespace list is `cosmetic`, and the difference is the point: the namespace list FILTERS names the cluster defines, while this document DEFINES them. `volts` is unit 1, holding register 100 under one map and can be unit 3, coil 5 under another, so a read keyed to a connection whose map changed would match a name to an address it was not taken from - the one direction `connectionResolutionKey` exists to make impossible. AND A BOUND, because there was none. Each point was checked and nothing checked how many there were, so with the field in place a document of any size would reach the parser, the payload and storage without meeting a rule. 65536 bytes, checked before `JSON.parse` for the reason the frame reader bounds at the header: a rule that runs after the work has not bounded the work. The figure is measured rather than chosen - the provider's own example map is 79 bytes a point and the widest possible point is 248, so this admits about 264 of the widest or 829 plain ones, where a device's named points run to tens or low hundreds. Canary, and the first writing of it was wrong in a way worth recording. The modal's case proves the box is drawn for an engine that declares the field, but it reads the test file's MIRROR of the config, so removing the field from the real `connectionFields` left it green. The real-config case in `db-ui-config.test.ts` is the one that goes red, and both are kept: one binds the box to the declaration, the other binds the declaration to the engine. Removing the bound turns two parser cases red, named in the run.
`api-docs-types.test.ts` compares the `DatabaseConnection` block in `docs/API_DOCS.md` against the interface and failed the moment the field existed in one and not the other. That gate is the reason this is a separate commit rather than a footnote: a field a client can send has to be readable by whoever writes the client. The line says what the document is, that it is bounded at 65536 bytes with the check before the parse, and why it is public-classified - Modbus TCP defines no authentication, so the map carries nothing to leak.
Both were found by an independent reviewer flipping them and watching nothing happen: `registerMap: "resolution"` changed to `"cosmetic"` left the suite at 1510 of 1510, and `MAX_REGISTER_MAP_BYTES` multiplied sixteen-fold did the same. A decision no case holds is a decision the next hand can undo without being told. The figure gets its own case because of what kind of number it is. Its neighbours are pinned as values - `MAX_ADU_BYTES` at 260, `MAX_IDENTIFICATION_OBJECT_BYTES` at 244 - and those DERIVE from the protocol, so moving either would be moving Modbus. 65536 was chosen from a measured range, which is exactly the kind of number that needs a declaration of its own. The second case states the headroom rather than restating the figure, and it computes both ends from real documents: a plain point and one with every optional field set. The classification case separates the two maps that matter. A different document moving the key proves little on its own, so the case also takes two maps of equal length naming one point each - `volts` at unit 1, holding register 100 and `volts` at unit 3, coil 5 - because the same NAME over a different address is what the key has to tell apart. AND THE REASON IN THE DOCBLOCK WAS WRONG, which is the more useful half of this. It argued from `CONNECTION_RELEVANCE`'s summary criterion - which database, and as whom - and `registerMap` answers neither: the same device answers, and Modbus TCP authenticates nobody. A correct decision defended by a criterion it does not meet is an argument the next reader will rightly reject. The reason is what READS the key: `use-provider-metadata` makes it part of `readKey`, `use-connection-pulse` keys the pulse on it, `SchemaDiff` keys its snapshot on it, so a map that changed while the key did not would leave the previous map's points drawn. That sentence replaces the old one in `types.ts` and in `use-connection-payload.ts`.
The case said its two documents were "two maps of equal length naming one point each". They were not: 86 bytes against 70, because `holding_register` is longer than `coil`. The claim it proves was still true - the key does separate the same name over a different address - but the sentence under it was not, and a reader would take it as "length is held constant here, so the address is what moved". The key carries the whole document, so any byte of difference moves it on its own. A case whose two maps differ in length therefore passes without ever reaching the address, which makes the length the premise rather than a detail. The second map now carries a unit of measure as padding, bringing it to the same eighty-six bytes, and the test asserts the two lengths are equal before it compares the keys: the premise is measured where it used to be asserted in a comment. Found by an independent reviewer who counted the bytes. It is the same class as the `resolution` reason this branch corrected an hour earlier - a right conclusion resting on a wrong ground - and the fix is the same shape: let the test carry what the comment was claiming.
…not have The dialog offers the panel for this engine and says so - "MODBUS/TCP Security listens on 802 and the SSL panel turns it on" - and the provider tested `config.ssl?.enabled === true`. `SSLConfig` has no `enabled`: it has `mode`, and `enabled` is the SSH tunnel's word. The test was therefore never true, so `mbaps` could not be reached from the dialog at all, whatever the user set. Measured, not inferred: `enabled` appears on no SSL type in the tree, and the only reader of `ssl.enabled` in `src/` was this one line. Six sentences in the product describe the mode that line made unreachable. The dialog's comment says the panel turns it on; the refusal for exception 01 says "on an mbaps connection this can also mean it refused the certificate's role"; `MODBUS_NO_REVOCATION_SENTENCE` tells the user what mbaps does not check; `valve.ts` and `docs/providers/modbus.md` describe a certificate carrying a role; and the doc states "mbaps checks the chain and the host name, and not revocation, and says so". None of it could happen. The panel is now read by `nodeTlsMaterial`, the one place every other engine reads it - MTConnect, RabbitMQ, S3, Kubernetes and the HTTP transport all go through it - so `disable` is no TLS rather than TLS that verifies nothing, a mode the panel does not define is refused, and half a client pair is refused rather than dialled with half of one. The dial converts it with `nodeTlsConnectOptions`, as the AMQP dial does, which carries the two things the hand-spread object dropped: `servername` for a name-addressed device, and `checkServerIdentity` against the host the user typed rather than the address the lookup returned. Reading the panel now lives in `connection-options.ts`, which is where every other panel-reading provider keeps it. That is what makes it measurable: a pure step over a connection record, tested without opening a socket. Nine cases there for what the panel yields, three in `client.test.ts` for what the dial does with it - the two halves were broken in opposite directions at once, so each needs its own. One of those cases was written wrong first and the correction is worth keeping: it expected a panel with no mode to be refused, and the shared reader defaults the mode to `verify-full`. That default is the point. The old condition read a missing field and dialled PLAIN; a panel switched on now verifies even when the mode is the one thing the user did not set.
The panel fix had unit cases on both halves - what the panel yields and what
the dial does with it - and no case that put the two together against something
that really speaks TLS. These four do, on loopback, with the repository's own
throwaway CA.
They are a reviewer's three readings turned into cases, and the three are a
set. Plain TCP answers five rows with the 7000 series, which says the working
path is sound. The same device over TLS with the panel's CA answers the same
five rows, which is the reading the product could not produce before. A CA that
signed nothing here is refused with a sentence about the certificate, which is
what separates a trust anchor that is APPLIED from one that is carried and
ignored: a provider passing empty material would fail this case too, but it
would fail the one above it as well.
The fourth case keeps the symptom. With the panel unread, Modbus bytes go to a
TLS listener and the listener resets - `read ECONNRESET`, which is what the
reviewer saw and what named the cause. An empty `tls: {}` would have failed as
a handshake that cannot verify, so the two defects are told apart by the
sentence, not by the failure. Asserting the message has no "certificate" in it
pins that difference, so a future hand that stops reading the panel sees the
user's own sentence rather than a generic error.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
added 4 commits
October 16, 2026 06:59
…ues in Three refusal sentences sorted a list of accepted values with a bare `.sort()`, which SonarCloud reports as three critical bugs: a sort with no comparator orders by UTF-16 code unit, and a caller who meant alphabetical gets another order in another alphabet. MEASURED before choosing. All three sets are closed lists of ASCII identifiers declared in this repository: the request's own keys (`read`; `unit`, `table`, `address`, `count`, `type`, `wordOrder`, `byteOrder`; `identify`), the four table names, and the eight types `values.ts` decodes. Sorting them by code unit put them in the same order as `String.localeCompare` did in en-US, de-DE, sv-SE, tr-TR and lt-LT, so no sentence changes a word. `localeCompare` is not what was written down, although these sentences are read by a user: the values in them are identifiers from this source, not text a device or a reader supplied, so a collation would buy nothing and would make a sentence's word order depend on the runtime's locale data. An explicit code-unit comparator keeps the order the sentences already had, and the reason sits above it.
… byte The test device looked its reader up in a two-key object indexed by the function code read out of the ADU, which CodeQL reports twice as `js/unvalidated-dynamic-method-call`. MEASURED: `adu[7]` is one byte, so the index is a number in 0..255; each object held exactly two own keys; and the guard above the lookup narrows the code to those two, which leaves no other target reachable. The pattern was already as narrow as the scanner asks for. But a scanner cannot read a guard, and a red gate is red whatever the pattern does, so the dynamic key is gone rather than argued with: the table is now chosen by the same condition that was guarding it. Behaviour unchanged, and the device is what proves it: 375 tests across the Modbus unit, provider and mbaps files pass, the provider reads the same five 7000-series rows over a real socket as before.
`blocks.map((b) => b.unitId).sort()` passes today for one reason: the ids are 1 and 3, and one digit reads the same as text as it does as a number. MEASURED: `[3, 10].sort()` answers `[10, 3]`, so the assertion would hold a wrong order the day a unit id of 10 reaches this test, and it would hold it silently. The trap is already written down twenty lines away, in objects.test.ts, which measures that `[1, 3, 10]` orders differently as numbers and as text before asserting the order. This is the same sort with no comparator, in a file that never met a two-digit id. The two string sorts in this file are left as they are: the default comparator is a total order on text, which is what those assertions ask for.
…y so Six of the nine cases in e2e/modbus-provider.spec.ts dial 127.0.0.1:1502. The e2e job serves nothing there, so on this branch's run they dialled it and got `connect ECONNREFUSED` eighteen times, which is the whole of the E2E red. MEASURED rather than assumed: the `e2e` job of ci.yml declares no `services:`, runs no compose command and no `docker run` - its steps are checkout, Bun, dependencies, Playwright browsers, the test run, the subpath run and the report upload. The Modbus live check of docker/modbus/ci-step.yml does start a container on 1502, but it is pasted into the `functional-smoke` job, which is not where the Playwright projects run, so pasting it would not have changed this. So the spec asks the device whether it is there and skips naming the address, which is what e2e/mtconnect-provider.spec.ts and e2e/opcua-screens.spec.ts do with their live cases. The probe asks a MODBUS question, not whether the port accepts: it sends one register read and requires the reply's MBAP header to echo the transaction and unit it sent. tests/live/ modbus-ready-probe.ts measured why - against a published container port Docker answers the accept in 5 ms while the server behind it took 332 ms to listen, so an accept is not a readiness fact. The threshold is "speaks", not "serves this fixture's values", so a WRONG fixture stays red while an absent one skips. MEASURED both directions locally: with the fixture up, 9 passed and 0 skipped; with MODBUS_E2E_TARGET pointed at a port nothing serves, 6 skipped and 3 passed - the same six. docker/modbus/e2e-project.md said the opposite of two things and both are corrected there. It said no project block lands, because the second-server projects exist for the bucket Test Connection spends and this spec never clicks Test Connection; the second half is true and the conclusion was still wrong, because use-connection-form.ts:1321-1343 is one probe for both buttons and it POSTs the rate-limited /api/db/test-connection. The block this PR adds is right, and so is its entry in SECOND_SERVER_SPECS: that test passes 63, not the 57 the old text quoted.
|
cevheri
approved these changes
Oct 16, 2026
yusuf-gundogdu
added a commit
that referenced
this pull request
Oct 16, 2026
… kept main had moved six PRs ahead (#1794, #1799, #1800, #1792 Modbus, #1804, #1801) and GitHub called the branch unmergeable. 73 files conflicted in 131 hunks, and the resolution rule was one sentence: WHERE BOTH SIDES ADDED AN ENTRY, BOTH ENTRIES STAY, main's first because Modbus landed first. NUMBERS DERIVED FROM THE MERGED TREE, not carried from either side: EXTERNAL 39, SHIPPED 40, WIRE_COMPATIBLE 27 (unchanged by either engine), connectable 66. The four denominators stay apart - "thirty-nine engines" is EXTERNAL, "forty type-ids" and "all forty ids" are SHIPPED, "sixty-six named engines" is EXTERNAL plus the wire-compatible relatives. WHAT THE LINE-BY-LINE MERGE GOT WRONG, each caught by a gate rather than by reading: - seven src and four test files lost a closing brace where two entries' tails were identical, so the entry was rebuilt from main's file with this engine's record moved in whole; - `migration-generator.ts` kept the record but lost BOTH plain-list entries; - `dialect-editors.test.ts`, `object-source-declarations.test.ts`, `mcp-documentation.test.ts`, `connection-fingerprint.test.ts` and `BRAND_MESSAGING.md` each carried a duplicated claim, one copy from each side, with different numbers in three of them; - the golden dialect fixture cannot be merged line by line at all: it was regenerated with the command its own docblock gives, and the gate compares it byte for byte; - the operator CSVs were regenerated with `make -C operator bundle` after the source manifest was corrected, not edited by hand. AND THREE DEFECTS OF MY OWN RESOLUTION, found by scanning for them rather than by trusting it: a double comma in README.md, a space before a comma in both chart READMEs, and a stale "and" left in the operator CSV's engine list. The scans are in the record: `,,`, ` ,`, duplicated adjacent lines, and engine-name counts per fleet list. NINE TRANSLATIONS carry both engines in every fleet list and the numeral in each file's own form: a word in en/zh/es/pt/ru/hi/ur and a figure in ja/ko. The Urdu pair had to be matched by code point, because the word I typed was not the word the file holds. THE RECENCY CHAIN MOVED, which a gate holds at both ends: Sparkplug B is now the newest, Modbus came before it, and `not.toContain("Modbus is the newest")` is what keeps the previous head from quietly claiming a title it no longer holds. MEASURED, twice, lane taken, chart dependency built: 1550 files, 57560 tests, 57555 pass, 5 skip, 0 fail, 0 not run, exit 0. format, lint, typecheck, knip, readme:check, security:check, distribution:check, channels:showcase:check and chart:check all exit 0. ONE FINDING THAT IS NOT MINE: main dropped "Apache Spark" from the engine list in docs/AGENT_GUIDE.md (the base has it, origin/main does not). The name is restored here because the line had to be edited for this engine anyway and shipping a list that silently lost an engine is worse; it is reported rather than hidden.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #1723. Modbus TCP, read only: coils, discrete inputs, input registers, holding registers, and the identification document. The device enforces nothing of its own, so the provider sends four read function codes and FC 43/14, and nothing else.
What it reads
A connection is one device address. Without a register map it reads a range by address; with one, each point has a name, a unit id, a table, a zero-based PDU address and a type. The box says which, and says it in the vendor's terms: vendor documentation often counts addresses from one, and these are the addresses that go on the wire.
Test layers
oitc/modbus-serverpinned by digest, loopback only, with values multiplied so an off-by-one shows as eleven units rather than one.Three defects the layers found
Each is fixed here, and each is held by a case that goes red when the fix is reverted.
readIdentificationhad no unit test at all, and the live screen test had never runidentify, so neither layer had ever been down this path.SSLConfigcarriesmode,caCert,clientCertandclientKey; the provider readssl.enabled, which is the SSH tunnel's field and does not exist onSSLConfig. The condition was therefore never true: a connection to an mbaps device went out as plain TCP, and the device reset it. Reading the panel throughconnection-options.ts, where every other provider reads it, also restoresservernameand the host check that hand-rolled material had dropped.Counts
EXTERNAL_DATABASE_TYPESmoves from 37 to 38. Every count that follows from a registration is re-derived rather than edited by hand.Merge order
This is the second of five providers from one chain. The order measured cheapest is MTConnect, Modbus, NETCONF, Sparkplug B, SNMP, and MTConnect is already in. The order is not only about conflicts: three READMEs say MTConnect came before Modbus, a case anchors that sentence, and the engine count follows the same sequence, so a different order makes those sentences wrong rather than merely conflicting. If a different order is wanted, saying so before the third opens costs nothing; afterwards it costs a rewrite of the prose in nine languages.