fix(engine): anchor content search to obj_start in parse_messages_json#764
Open
somtri wants to merge 1 commit into
Open
fix(engine): anchor content search to obj_start in parse_messages_json#764somtri wants to merge 1 commit into
somtri wants to merge 1 commit into
Conversation
parse_messages_json searched "content" from role_end instead of obj_start, so a message object serializing content before role (legal JSON; key order isn't semantically significant) silently dropped its content. role_pos and the images/audio path arrays already search from obj_start; content_pos now matches. Fixes cactus-compute#757 Signed-off-by: Som Tripathi <somtri@iastate.edu>
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
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.
Fixes #757.
parse_messages_jsonsearches for"role"starting at the message object's opening brace, so it finds the role key regardless of where it appears in the object. The search for"content", however, started from the end of the parsed role value instead of the same starting point. When a message object serializescontentbeforerole(legal JSON, since key order isn't semantically significant, and something an unordered-map serializer such as Swift'sJSONSerializationcan produce), the content key sits earlier in the string than the search start, so it's never found within the object's bounds andmsg.contentis silently left empty.This anchors the
contentsearch toobj_start, matching howroleand theimages/audiopath arrays already search. The existingcontent_pos < obj_endbound is unchanged, so the search still can't cross into a later message object.Added
cactus-engine/tests/test_parse_messages.cpp, a standalone suite (no model weights needed) covering role-first order, content-first order, escaped content, mixed ordering across a multi-message array, and content-first with image/audio paths attached. Confirmed the content-first case fails on the unpatched function and passes after the fix.Built and ran the full
cactus-enginetest component (all 15 targets) on an ARM64 Linux environment. No new failures. Two pre-existing, unrelated issues were investigated and ruled out as caused by this change:test_indexneedsCACTUS_INDEX_PATHset when run directly (passes once set);test_telemetry's concurrency race test fails identically on the unpatched tree (confirmed by reverting the fix and rerunning), most likely a timing margin issue under emulation, not related to this diff.