fix(state): never emit created_at on the wire - #146
Merged
Conversation
#144 removed the ts -> created_at carry but left created_at in the allowlist the serializer filters against, so a buffer record containing that key literally would still pass straight through. Nothing writes it today, so this was latent rather than live — but closing exactly this kind of door is what the allowlist is for, and the test only supplied ts so it could not catch it. Splits the two concepts: _INTERACTION_DATA_FIELDS stays the model-contract set (created_at IS a real InteractionData field, and the drift test pins the set against the model), while _WIRE_FIELDS is what the serializer filters on and excludes it. Why created_at must not be emitted: the extractor's bookmark is keyed on interaction created_at (last_processed_timestamp, compared with created_at >= ?), so a backdated batch — one recovered after the bookmark moved — is stored and then never extracted. That is permanent, silent loss of learning data on precisely the offline-recovery path this buffer exists for. Letting the server stamp its own time is the lesser evil until ingest ordering stops depending on caller-supplied event time. Caught by CodeRabbit on #144 after it merged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe outbound turn serializer now uses a wire-specific allowlist that excludes ChangesWire payload contract
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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.
Follow-up to #144, from a CodeRabbit Major on that PR after it merged.
#144 removed the
ts -> created_atcarry but leftcreated_atin the allowlist the serializer filters against, so a buffer record containing that key literally would still pass straight through. Verified on mergedmain:Nothing writes that key today, so it was latent rather than live — but closing exactly this kind of door is what the allowlist is for, and the existing test only supplied
ts, so it could not catch it.The fix
Splits two concepts that were conflated:
_INTERACTION_DATA_FIELDSstays the model-contract set.created_atgenuinely is anInteractionDatafield, and the drift test pins this set against the real model, so it must remain here._WIRE_FIELDSis what the serializer filters on, and excludes it.Why
created_atmust never be emittedThe extractor's bookmark is keyed on interaction
created_at(last_processed_timestamp, compared withcreated_at >= ?). A backdated batch — one recovered after the bookmark has moved — is stored and then never seen by the extractor. That was reproduced end to end as permanent, silent loss of learning data, on precisely the offline-recovery path this buffer exists to protect. Letting the server stamp its own time is the lesser evil until ingest ordering stops depending on caller-supplied event time.Tests: one asserting a literal
created_atin the buffer is not emitted, one pinning_WIRE_FIELDS < _INTERACTION_DATA_FIELDSso the two sets cannot silently converge again. 457 tests pass.Summary by CodeRabbit
Bug Fixes
Tests