Skip to content

fix: decode special log records in ReadRange without status flags#77

Open
Copilot wants to merge 7 commits intomasterfrom
copilot/fix-readrange-bug-special-log-record
Open

fix: decode special log records in ReadRange without status flags#77
Copilot wants to merge 7 commits intomasterfrom
copilot/fix-readrange-bug-special-log-record

Conversation

Copy link
Contributor

Copilot AI commented Mar 17, 2026

  • Analyze the issue: ReadRange fails when TREND_LOG contains special log records (e.g., log-interrupted)
  • Identify root cause: decodeRange unconditionally consumes closing tag 1 + context tag 2 (status flags), but special records only have closing tag 1 with no status flags
  • Fix decodeRange in src/lib/asn1.ts:
    • Validate closing tag 1 before consuming it
    • Check for context tag 2 presence before decoding status flags
    • Add support for log-status choice (context tag 0 - special records)
    • Add isLogStatus and logStatus fields to differentiate special records
  • Add unit test for special log records without status flags (mixed with normal records)
  • Run existing tests - all 203 unit tests + 33 integration tests pass
  • Run linting - passes
  • Run build - passes
  • Address code review comments:
    • Extract LogRecord, LogStatusFlags, LogRecordStatusFlags interfaces to types.ts
    • Update ReadRangeAcknowledge.values to use LogRecord[] instead of any[]
    • Update decodeRange to return Decode<LogRecord[]>
    • Use assert.strictEqual for undefined checks in tests
    • Use BACNetBitString for bitstring casts instead of { value: number[] }
    • Add LogRecordValue union type to narrow LogRecord.value from unknown
    • Keep snake_case property names (log_disabled, buffer_purged, log_interrupted, out_of_service, in_alarm) to avoid breaking changes
  • Run security scan (CodeQL) - no issues found
  • Add ASHRAE 135 §12.25 standard references to code comments
Original prompt

This section details on the original issue you should resolve

<issue_title>ReadRange bug when response contains a special log record</issue_title>
<issue_description>Hello

When reading a TREND_LOG that contains a special log record (e.g. log-interrupted), records after it are silently missing from the result — no error, no warning.

I spent some time debugging this with Wireshark and found that the raw UDP packet is perfectly valid and complete. The issue is in decodeRange.

What's happening

Special log records don't have status flags. A normal record ends with:

1f 2a 04 00   ← closing tag 1, opening tag 2, BIT_STRING, closing tag 2

A special record ends with just:

1f            ← closing tag 1, nothing after

But decodeRange unconditionally consumes 2 tags after the datum value without validating their tag numbers. So it reads into the bytes of the next record, shifts the offset by an unpredictable amount (depends on the byte values that follow), and loses sync for everything after.

The behavior is non-deterministic — sometimes records after the special one are silently dropped, sometimes the buffer appears truncated. Took a while to figure out why the same code would behave differently depending on which records were in the response.

Reproduction

const response = await client.readRange(
    { address: '10.x.x.x:47808' },
    { type: bacnet.ObjectType.TREND_LOG, instance: 100101 },
    startIndex, 20,
    {}
);
// response contains fewer records than expected
// no error thrown
// Wireshark shows the full response is received correctly

Thanks so much for the patch ;-)

Thomas</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
<comment_new><author>@robertsLando</author><body>
Hi Thomas and thanks for your issue! WOuld you like to submit a PR? </body></comment_new>
</comments>

💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

Copilot AI and others added 2 commits March 17, 2026 08:45
Special log records (log-interrupted, buffer-purged, log-disabled) use
the log-status choice which does NOT include status flags. The previous
code unconditionally consumed the status flags bytes, causing it to read
into subsequent records and lose sync.

This fix:
- Validates closing tag 1 before consuming it
- Checks for context tag 2 (status flags) presence before decoding
- Adds support for log-status choice (context tag 0)
- Adds isLogStatus and logStatus fields to differentiate special records
- Adds comprehensive test for mixed normal and special log records

Co-authored-by: robertsLando <11502495+robertsLando@users.noreply.github.com>
- Extract LogRecord, LogStatusFlags, LogRecordStatusFlags interfaces
- Update ReadRangeAcknowledge.values to use LogRecord[] instead of any[]
- Update decodeRange to return Decode<LogRecord[]>
- Use assert.strictEqual for undefined checks in tests

Co-authored-by: robertsLando <11502495+robertsLando@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix ReadRange bug when response contains a special log record fix: decode special log records in ReadRange without status flags Mar 17, 2026
Copilot AI requested a review from robertsLando March 17, 2026 08:50
@robertsLando robertsLando marked this pull request as ready for review March 17, 2026 09:29
Copilot AI review requested due to automatic review settings March 17, 2026 09:29
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot

ASHRAE 135 Standard Reference

This library implements ASHRAE Standard 135 (BACnet). Always consult the official specification and reference implementations when reviewing or implementing features:

[bacnet-stack](https://github.com/bacnet-stack/bacnet-stack) — The most widely used open-source BACnet implementation in C. Use as the primary source of truth for protocol behavior and packet formats.
[BACnet.org technical papers](https://bacnet.org/developer-resources/) — Free PDFs covering specific topics (foreign devices, BBMDs, routing, etc.)
[Wireshark BVLC dissector](https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-bvlc.c) — Useful for verifying packet formats and field layouts

When implementing or modifying protocol features, reference the relevant standard section in a code comment. Example:

// Encode object identifier per ASHRAE 135-2024 §20.2.14
encodeApplicationObjectId(buffer, objectType, instance)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds typed support for decoding BACnet TREND_LOG “log-status” records in ReadRange acknowledgements, including handling records that omit status flags per ASHRAE 135.

Changes:

  • Extended baAsn1.decodeRange to decode the log-datum CHOICE [0] log-status and to tolerate missing status flags for log-status records.
  • Introduced LogRecord (and related flag interfaces) and updated ReadRangeAcknowledge.values to use it.
  • Added a unit test covering mixed normal + log-status log records in a single range buffer.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
test/unit/service-read-range.spec.ts Adds coverage for decoding log-status records that omit status flags.
src/lib/types.ts Introduces LogRecord typing and narrows ReadRangeAcknowledge.values to LogRecord[].
src/lib/asn1.ts Updates decodeRange to parse log-status records and make status flags optional when appropriate.
src/lib/services/WriteProperty.ts Formatting-only changes (no behavioral change).

You can also share your feedback on Copilot code review. Take the survey.

Reference ASHRAE 135 Standard Section 12.25 for LogRecord structure
in JSDoc comments for decodeRange function and LogRecord types.

Co-authored-by: robertsLando <11502495+robertsLando@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 17, 2026

@copilot

ASHRAE 135 Standard Reference...

Added ASHRAE 135 §12.25 standard references to the decodeRange function JSDoc and LogRecord, LogStatusFlags, LogRecordStatusFlags type definitions in commit 5609612.

@robertsLando
Copy link
Member

@copilot Address review comments please. Ignore the ones marked as RESOLVED

Address code review comments:
- Use BACNetBitString for bitstring casts instead of { value: number[] }
- Add LogRecordValue union type to narrow LogRecord.value from unknown

Co-authored-by: robertsLando <11502495+robertsLando@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 17, 2026

@copilot Address review comments please. Ignore the ones marked as RESOLVED

Addressed the unresolved review comments in commit a33fbbb:

  1. BACNetBitString casts: Changed bitstring casts from { value: number[] } to BACNetBitString to properly reflect the shape returned by decodeBitstring (includes bitsUsed property).

  2. LogRecordValue type: Added a LogRecordValue union type to narrow LogRecord.value from unknown to a union of supported value types (number | boolean | BACNetBitString | null | string | BACNetObjectID).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 127.0.0.2
    • Triggering command: REDACTED, pid is -1 (packet block)
  • 255.255.255.255
    • Triggering command: REDACTED, pid is -1 (packet block)

If you need me to access, download, or install something from one of these locations, you can either:

@robertsLando robertsLando requested a review from Copilot March 17, 2026 09:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request extends the ReadRange ACK decoding path to properly handle BACnet TREND_LOG “log-status” records (which omit status-flags), and adds stronger typing around decoded log records.

Changes:

  • Add unit coverage for decoding log-status records without status flags in ReadRange acknowledgements.
  • Introduce LogRecord / related types and update ReadRangeAcknowledge.values typing accordingly.
  • Update decodeRange logic to decode log-status choice records and treat status-flags as optional for non-log-status records.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
test/unit/service-read-range.spec.ts Adds a regression test for decoding mixed normal + log-status log records.
src/lib/types.ts Adds LogRecord-related types and updates ReadRangeAcknowledge.values to LogRecord[].
src/lib/asn1.ts Updates decodeRange to support log-status choice records and optional status-flags.
src/lib/services/WriteProperty.ts Formatting-only changes.
Comments suppressed due to low confidence (1)

src/lib/asn1.ts:2534

  • The comment lists multiple log-datum CHOICE alternatives, but the implementation currently only handles tag 0 (log-status), tag 2 (REAL), and some ENUMERATED cases. If other alternatives (e.g., boolean, unsigned, signed, null, string, object-id) appear, decodeRange will return undefined and ReadRangeAcknowledge.values will be omitted. Consider either decoding the remaining alternatives or making the supported subset explicit (and non-fatal) so mixed/other record types don't prevent returning parsed values for known records.
		// log-datum [1] CHOICE per ASHRAE 135 §12.25:
		// [0] log-status (BACnetLogStatus bitstring - special records)
		// [1] boolean-value
		// [2] real-value
		// [3] enum-value
		// [4] unsigned-value
		// ... and more
		tag = decodeTagNumberAndValue(buffer, offset + len)
		len += tag.len
		let value: ApplicationData | undefined
		let isLogStatus = false
		if (tag.tagNumber === 0) {
			// log-status choice: BACnetLogStatus bitstring per ASHRAE 135 §12.25
			// Special log records (log-disabled, buffer-purged, log-interrupted)
			// do NOT have statusFlags [2] after the log-datum
			value = bacappDecodeData(
				buffer,
				offset + len,
				maxOffset,
				ApplicationTag.BIT_STRING,
				tag.value,
			)
			isLogStatus = true
		} else if (tag.tagNumber === 2 && tag.value === 4) {
			value = bacappDecodeData(
				buffer,
				offset + len,
				maxOffset,
				ApplicationTag.REAL,
				tag.value,
			)
		} else if (tag.tagNumber === 4 && tag.value === 1) {
			value = bacappDecodeData(
				buffer,
				offset + len,
				maxOffset,
				ApplicationTag.ENUMERATED,
				tag.value,
			)
		} else if (tag.tagNumber === 3 && tag.value === 1) {
			value = bacappDecodeData(
				buffer,
				offset + len,
				maxOffset,
				ApplicationTag.ENUMERATED,
				tag.value,
			)
		}

You can also share your feedback on Copilot code review. Take the survey.

@robertsLando
Copy link
Member

@copilot fix review

Co-authored-by: robertsLando <11502495+robertsLando@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 17, 2026

@copilot fix review

Fixed in f5bbc1f:

  1. Changed all flag names to camelCase (logDisabled, bufferPurged, logInterrupted, outOfService, overridden, fault, inAlarm)
  2. Narrowed LogRecordValue to number | BACNetBitString (the types actually decoded) and added JSDoc documenting which CHOICE alternatives are supported

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 127.0.0.2
    • Triggering command: REDACTED, pid is -1 (packet block)
  • 255.255.255.255
    • Triggering command: REDACTED, pid is -1 (packet block)

If you need me to access, download, or install something from one of these locations, you can either:

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Don't introduce unnecessary braking changes. Keep the previous property names in interfaces

Co-authored-by: robertsLando <11502495+robertsLando@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 17, 2026

@copilot Don't introduce unnecessary braking changes. Keep the previous property names in interfaces

Reverted in f1f7176. Restored the original snake_case property names (log_disabled, buffer_purged, log_interrupted, out_of_service, in_alarm) to avoid breaking changes.

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.

ReadRange bug when response contains a special log record

3 participants