Skip to content

Fix Header.parse scrambling every field - #24

Closed
gonzalocasas wants to merge 1 commit into
mainfrom
pr/mqtt-header-field-mapping
Closed

gonzalocasas wants to merge 1 commit into
mainfrom
pr/mqtt-header-field-mapping

Conversation

@gonzalocasas

Copy link
Copy Markdown
Member

Header.parse shifts every field by one, and has done since 2024-05-01. This is on main, so it affects anyone coordinating devices today.

The bug

instance = cls(value["sequence_id"], value["response_id"], value["device_id"], value["time_stamp"])

The constructor's first parameter is increment_response_ID, so the four values land one slot to the left: sequence_id into increment_response_ID, response_id into sequence_id, device_id into response_id, time_stamp into device_id — and time_stamp itself is regenerated as "now".

Observed on a live broker round-trip:

SENT     sequence_id=0  response_id=0  device_id='6b6897d9-…'  time_stamp='23:37:41.599256'
RECEIVED sequence_id=1  response_id='6b6897d9-…'  device_id='23:37:41.599256'  time_stamp=<now>

The message body survives; the header does not. Since Header carries the sequence and response IDs used to correlate requests with responses and to ignore self-sent messages, any multi-device coordination is running on scrambled values.

Origin

26577b9 (2024-05-01, "simplify messages.py and test inheritance from Message Class of compas_eve") added increment_response_ID as a new first parameter without updating the call site. The version before it read each field by name and validated.

The fix

Pass the fields as keyword arguments and restore the validation that commit dropped. Two related fixes came out of it:

  • The constructor used sequence_id or default, discarding a legitimate counter value of 0. Now is None.
  • parse() accepts either the decoded wire dict or a Header instance, since it is called both ways.

Verification

Seven new tests cover field mapping, zero-valued counters, the validation error per missing field, and a full message round-trip. A live broker round-trip now reports header intact: True.

messages.py is byte-identical between main and restructure, so this applies cleanly to both. The same commit also sits at the base of the restructure-side stack.

🤖 Generated with Claude Code

Header.parse passed four values positionally into a constructor whose
first parameter is increment_response_ID, so every field shifted by one:
sequence_id landed in increment_response_ID, response_id in sequence_id,
device_id in response_id, time_stamp in device_id, and time_stamp was
regenerated. Verified over a live broker round-trip.

This has been wrong since 26577b9 (2024-05-01), which added
increment_response_ID as a new first parameter without updating the call
site. The version before it read each field by name.

Pass the fields as keyword arguments and restore the validation that
commit dropped. Two related fixes:

- The constructor used `sequence_id or default`, discarding a legitimate
  counter value of 0. Use `is None` instead.
- parse() now accepts either the decoded wire dict or a Header instance.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@gonzalocasas

Copy link
Copy Markdown
Member Author

Superseded by #25, which carries this exact commit plus the project-envelope fix, as the base of the messaging stack.

Splitting this one out was only worth it if the header fix could reach users well ahead of the Model migration. With #23 now out of draft and mergeable, that gap has closed, so the duplicate review costs more than it buys.

The fix itself is unchanged — same commit, reviewed in #25.

@gonzalocasas
gonzalocasas deleted the pr/mqtt-header-field-mapping branch September 8, 2026 07:25
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.

1 participant