Skip to content

Fix bugs found in codebase-wide review, with regression tests#19

Merged
jmmoser merged 1 commit into
mainfrom
claude/bug-review-q6keh8
Jul 8, 2026
Merged

Fix bugs found in codebase-wide review, with regression tests#19
jmmoser merged 1 commit into
mainfrom
claude/bug-review-q6keh8

Conversation

@jmmoser

@jmmoser jmmoser commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Core infrastructure:

  • utils.js: CallbackPromise now observes rejections from async executors
    instead of discarding them, so throws inside async resolvers reject the
    promise rather than leaving it pending forever with an unhandledRejection;
    InvertKeyValues Map branch no longer relies on Iterator.prototype.forEach
    (missing on Node 20, the minimum supported engine)
  • Layer.js: context timeouts now remove their entry from
    __contextToCallbackTimeouts (previously leaked one entry per timed-out
    request); clearContexts materializes entries before clearing so callers
    no longer receive an always-empty live iterator
  • MultiplexLayer: layerContext called incrementContext(this) from a plain
    module function where this is undefined, so every multiplexed send
    threw a TypeError; duplicate disconnect() now returns after resolving
  • TCP layer: the socket 'error' handler never settled the connect promise,
    so a refused connection hung connected() forever; disconnect() during
    Connecting now destroys the in-flight socket instead of leaking the
    established connection; Scan never initiated a connection and therefore
    never yielded results; connected() resolves false when no connect was
    ever started
  • UDP layer: handleDestroy now closes the bound socket instead of only
    unref'ing it (the port stayed bound and receiving for the process life)

Modbus:

  • PDU.Decode validates wire length fields (byte counts, exception codes,
    write echoes) against the received data; malformed frames now produce a
    decodable error that rejects the pending request instead of throwing an
    uncaught ERR_OUT_OF_RANGE from the socket data handler
  • readCoils/readDiscreteInputs unpack the packed bytes into the requested
    number of coil statuses (LSB-first) instead of resolving raw bytes with
    undefined pad bits
  • requests now arm a timeout (options.timeout, default 10s) so a dropped
    response rejects instead of pending forever and leaking its context
  • per-request unitID/protocolID use ?? instead of ||, so unitID 0
    (broadcast) is no longer silently replaced by the default
  • constants: SerialLineFunctions renamed (deprecated misspelled alias
    retained) and merged into FunctionNames so serial-line responses no
    longer decode as 'Unknown'

CIP core:

  • object.js: OptionalServiceList is class attribute 5 per CIP Vol 1 (was
    duplicated with OptionalAttributeList at 4); GetAttributeSingle fallback
    referenced CommonClassAttribute.getCode which does not exist (group
    object intended)
  • Identity: Status.configured reads bit 2 per CIP Vol 1 Table 5-2.3 (bit 3
    is reserved and always decoded 0 on compliant devices)
  • datatypes: STRINGN honors the decoded character width instead of always
    decoding UTF-16LE; BOOL reads enough bytes to contain the addressed bit
    (positions 8+ always decoded false before); ARRAY/ABBREV_ARRAY
    EncodeSize sums per-element sizes so variable-size item types no longer
    throw mid-write or corrupt the stream
  • EPATH datatype segment: ARRAY encodeSize accounts for 1/2/4-byte bounds
    to match encodeTo (bounds > 255 threw a RangeError mid-encode)
  • service.js and Logix5000 objects/datatypes modules: add .js extensions
    to ESM imports (these modules threw ERR_MODULE_NOT_FOUND when imported);
    __shared.js exports SymbolType by name as symbol.js expects

EIP / Logix5000 / PCCC:

  • CIPInternalLayer/CIPConnectionLayer: pass the required offsetRef to
    request.response(); every PCCC-over-CIP response previously threw a
    TypeError so no forwarded request could ever complete
  • CIPInternalLayer.exploreAttributes queries through maxAttribute
    inclusive (loop stopped one short)
  • EIP: connect() queues callbacks instead of overwriting, so a second
    connect while RegisterSession is in flight no longer silently drops the
    first caller's callback; host strings without a port parse correctly
  • Logix5000: getSymbolInstanceID strips member access from dotted tags
    (the stripping line was commented out, so writeTag on any dotted tag
    hung); writeTag rejects cleanly for unknown tags (the previous guard
    could never fire and the resulting TypeError was swallowed);
    readTemplateClassAttributes wraps its body so protocol errors reject;
    readTemplate clamps the per-fragment remaining-bytes field to 16 bits
    for definitions larger than 64KB
  • remove empty src/core/cipmodbus module; fix latent offset/this bugs
    inside the commented-out CIP Modbus layer

Every fix is covered by a regression test that fails against the previous
code (verified: 39 of the new/updated tests fail on the unfixed sources).

Core infrastructure:
- utils.js: CallbackPromise now observes rejections from async executors
  instead of discarding them, so throws inside async resolvers reject the
  promise rather than leaving it pending forever with an unhandledRejection;
  InvertKeyValues Map branch no longer relies on Iterator.prototype.forEach
  (missing on Node 20, the minimum supported engine)
- Layer.js: context timeouts now remove their entry from
  __contextToCallbackTimeouts (previously leaked one entry per timed-out
  request); clearContexts materializes entries before clearing so callers
  no longer receive an always-empty live iterator
- MultiplexLayer: layerContext called incrementContext(this) from a plain
  module function where `this` is undefined, so every multiplexed send
  threw a TypeError; duplicate disconnect() now returns after resolving
- TCP layer: the socket 'error' handler never settled the connect promise,
  so a refused connection hung connected() forever; disconnect() during
  Connecting now destroys the in-flight socket instead of leaking the
  established connection; Scan never initiated a connection and therefore
  never yielded results; connected() resolves false when no connect was
  ever started
- UDP layer: handleDestroy now closes the bound socket instead of only
  unref'ing it (the port stayed bound and receiving for the process life)

Modbus:
- PDU.Decode validates wire length fields (byte counts, exception codes,
  write echoes) against the received data; malformed frames now produce a
  decodable error that rejects the pending request instead of throwing an
  uncaught ERR_OUT_OF_RANGE from the socket data handler
- readCoils/readDiscreteInputs unpack the packed bytes into the requested
  number of coil statuses (LSB-first) instead of resolving raw bytes with
  undefined pad bits
- requests now arm a timeout (options.timeout, default 10s) so a dropped
  response rejects instead of pending forever and leaking its context
- per-request unitID/protocolID use ?? instead of ||, so unitID 0
  (broadcast) is no longer silently replaced by the default
- constants: SerialLineFunctions renamed (deprecated misspelled alias
  retained) and merged into FunctionNames so serial-line responses no
  longer decode as 'Unknown'

CIP core:
- object.js: OptionalServiceList is class attribute 5 per CIP Vol 1 (was
  duplicated with OptionalAttributeList at 4); GetAttributeSingle fallback
  referenced CommonClassAttribute.getCode which does not exist (group
  object intended)
- Identity: Status.configured reads bit 2 per CIP Vol 1 Table 5-2.3 (bit 3
  is reserved and always decoded 0 on compliant devices)
- datatypes: STRINGN honors the decoded character width instead of always
  decoding UTF-16LE; BOOL reads enough bytes to contain the addressed bit
  (positions 8+ always decoded false before); ARRAY/ABBREV_ARRAY
  EncodeSize sums per-element sizes so variable-size item types no longer
  throw mid-write or corrupt the stream
- EPATH datatype segment: ARRAY encodeSize accounts for 1/2/4-byte bounds
  to match encodeTo (bounds > 255 threw a RangeError mid-encode)
- service.js and Logix5000 objects/datatypes modules: add .js extensions
  to ESM imports (these modules threw ERR_MODULE_NOT_FOUND when imported);
  __shared.js exports SymbolType by name as symbol.js expects

EIP / Logix5000 / PCCC:
- CIPInternalLayer/CIPConnectionLayer: pass the required offsetRef to
  request.response(); every PCCC-over-CIP response previously threw a
  TypeError so no forwarded request could ever complete
- CIPInternalLayer.exploreAttributes queries through maxAttribute
  inclusive (loop stopped one short)
- EIP: connect() queues callbacks instead of overwriting, so a second
  connect while RegisterSession is in flight no longer silently drops the
  first caller's callback; host strings without a port parse correctly
- Logix5000: getSymbolInstanceID strips member access from dotted tags
  (the stripping line was commented out, so writeTag on any dotted tag
  hung); writeTag rejects cleanly for unknown tags (the previous guard
  could never fire and the resulting TypeError was swallowed);
  readTemplateClassAttributes wraps its body so protocol errors reject;
  readTemplate clamps the per-fragment remaining-bytes field to 16 bits
  for definitions larger than 64KB
- remove empty src/core/cipmodbus module; fix latent offset/this bugs
  inside the commented-out CIP Modbus layer

Every fix is covered by a regression test that fails against the previous
code (verified: 39 of the new/updated tests fail on the unfixed sources).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NzFBvKrXRNpxmUpeFLeWWF
@jmmoser jmmoser merged commit 61e3102 into main Jul 8, 2026
1 check passed
@jmmoser jmmoser deleted the claude/bug-review-q6keh8 branch July 8, 2026 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants