fix(state): build the wire payload from an allowlist - #144
Merged
Conversation
📝 WalkthroughWalkthroughThe interaction serializer now uses an explicit wire-field allowlist. Unpublished turns exclude bookkeeping fields and ChangesWire payload contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_state.py`:
- Around line 503-518: Update test_bookkeeping_keys_never_reach_the_wire to
include id and kind in the input fixture, then explicitly assert that the
forbidden bookkeeping-key set is disjoint from turns[0]. Retain the existing
nonempty and content assertions while ensuring the test directly verifies those
keys are absent from emitted wire data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9072f9b0-e094-4515-8e6f-df0d2936af71
📒 Files selected for processing (2)
plugin/src/claude_smart/state.pytests/test_state.py
Comment on lines
+503
to
+518
| def test_bookkeeping_keys_never_reach_the_wire(self): | ||
| _, turns = state.unpublished_slice( | ||
| [ | ||
| { | ||
| "ts": 1, | ||
| "role": "User", | ||
| "content": "x", | ||
| "user_id": "p", | ||
| "host": "h", | ||
| "synthesised_by": "s", | ||
| } | ||
| ] | ||
| ) | ||
| assert turns, "expected one wire turn" | ||
| assert set(turns[0]) <= state._INTERACTION_DATA_FIELDS | ||
| assert turns[0]["content"] == "x" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert bookkeeping keys are absent directly.
This assertion only checks that output keys belong to _INTERACTION_DATA_FIELDS; if a bookkeeping key is accidentally added to that allowlist, the test still passes. Add id and kind to the fixture and assert that the forbidden-key set is disjoint from the emitted turn.
Suggested test adjustment
{
"ts": 1,
"role": "User",
"content": "x",
+ "id": "i",
+ "kind": "turn",
"user_id": "p",
"host": "h",
"synthesised_by": "s",
}
]
)
assert turns, "expected one wire turn"
+ bookkeeping = {"id", "kind", "user_id", "host", "synthesised_by"}
+ assert not bookkeeping.intersection(turns[0])
assert set(turns[0]) <= state._INTERACTION_DATA_FIELDS📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_bookkeeping_keys_never_reach_the_wire(self): | |
| _, turns = state.unpublished_slice( | |
| [ | |
| { | |
| "ts": 1, | |
| "role": "User", | |
| "content": "x", | |
| "user_id": "p", | |
| "host": "h", | |
| "synthesised_by": "s", | |
| } | |
| ] | |
| ) | |
| assert turns, "expected one wire turn" | |
| assert set(turns[0]) <= state._INTERACTION_DATA_FIELDS | |
| assert turns[0]["content"] == "x" | |
| def test_bookkeeping_keys_never_reach_the_wire(self): | |
| _, turns = state.unpublished_slice( | |
| [ | |
| { | |
| "ts": 1, | |
| "role": "User", | |
| "content": "x", | |
| "id": "i", | |
| "kind": "turn", | |
| "user_id": "p", | |
| "host": "h", | |
| "synthesised_by": "s", | |
| } | |
| ] | |
| ) | |
| assert turns, "expected one wire turn" | |
| bookkeeping = {"id", "kind", "user_id", "host", "synthesised_by"} | |
| assert not bookkeeping.intersection(turns[0]) | |
| assert set(turns[0]) <= state._INTERACTION_DATA_FIELDS | |
| assert turns[0]["content"] == "x" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_state.py` around lines 503 - 518, Update
test_bookkeeping_keys_never_reach_the_wire to include id and kind in the input
fixture, then explicitly assert that the forbidden bookkeeping-key set is
disjoint from turns[0]. Retain the existing nonempty and content assertions
while ensuring the test directly verifies those keys are absent from emitted
wire data.
guangyu-reflexio
force-pushed
the
fix/wire-payload-allowlist
branch
2 times, most recently
from
July 27, 2026 16:56
4176d7d to
d8c783c
Compare
unpublished_slice dropped four known keys and passed everything else through, so buffer-internal bookkeeping rode onto the wire on every turn. A denylist rots every time a hook adds a record key, and a server that rejected rather than reported unknown fields would turn that rot into a publish failure this plugin's adapter swallows without advancing its watermark -- so the same batch would retry forever. Scoped honestly: reflexio treats `user_id` as a benign request-level key and never warned about it, so the warning this actually removes is `synthesised_by`, emitted once per session by the SessionEnd anchor. The larger noise source was `tools_used[*].status`, fixed on the server by declaring the field (ReflexioAI/reflexio#383) rather than here. Deliberately does NOT send `created_at`. Carrying the buffer's own `ts` across was implemented and reverted: the extractor's bookmark is keyed on interaction `created_at` (`last_processed_timestamp`, compared with `created_at >= ?`), so a batch recovered after the bookmark had moved was stored and then never seen by the extractor. Reproduced end to end as permanent, silent loss of learning data -- on exactly the offline-recovery path this buffer exists to protect. The server stamping drain time is the lesser evil until ingest ordering stops depending on caller-supplied event time. Contract tests pin the allowlist against the real InteractionData model and assert a literal wire key set. Both details matter: asserting `set(turn) <= _INTERACTION_DATA_FIELDS` compares against the same constant the slicer filters by and could never fail (verified tautological by mutation), and the model check is a subset assertion because the plugin is deliberately forward-compatible -- it may know fields the pinned `reflexio-ai` release has not caught up to, which is why equality broke CI.
guangyu-reflexio
force-pushed
the
fix/wire-payload-allowlist
branch
from
July 27, 2026 17:39
d8c783c to
521daca
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/src/claude_smart/state.py`:
- Around line 438-444: The turn serializer in state.py must exclude buffered
created_at from emitted wire data; introduce or use a separate wire-field
allowlist excluding created_at, while preserving other interaction fields. In
tests/test_state.py, add created_at to the input fixture and assert the
serialized turn omits it.
In `@tests/test_state.py`:
- Around line 547-560: The test_buffer_timestamp_is_not_sent fixture must verify
removal of an input record’s existing created_at, not just omission of
ts-derived output. Add a created_at value to the unpublished_slice input record
and retain the assertion that the emitted turn lacks created_at.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a1a9610d-9fd0-4685-ba74-ec99db40ad1a
📒 Files selected for processing (3)
plugin/src/claude_smart/state.pytests/test_publish.pytests/test_state.py
Comment on lines
438
to
+444
| turn = { | ||
| key: value | ||
| for key, value in record.items() | ||
| if key not in {"role", "ts", "cited_items", "host"} | ||
| if key in _INTERACTION_DATA_FIELDS | ||
| } | ||
| turn["role"] = role | ||
| # NOTE: deliberately does NOT send `created_at`. Carrying the buffer's |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep buffered created_at off the wire and test that exact case.
The allowlist contains created_at, and the serializer filters against it without an exclusion. The current test only provides ts, so this regression remains undetected.
plugin/src/claude_smart/state.py#L438-L444: excludecreated_atfrom emitted turns, preferably via a separate wire-field set.tests/test_state.py#L547-L560: include"created_at"in the input fixture and assert it is omitted.
📍 Affects 2 files
plugin/src/claude_smart/state.py#L438-L444(this comment)tests/test_state.py#L547-L560
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/src/claude_smart/state.py` around lines 438 - 444, The turn serializer
in state.py must exclude buffered created_at from emitted wire data; introduce
or use a separate wire-field allowlist excluding created_at, while preserving
other interaction fields. In tests/test_state.py, add created_at to the input
fixture and assert the serialized turn omits it.
Comment on lines
+547
to
+560
| def test_buffer_timestamp_is_not_sent(self): | ||
| """`created_at` must stay off the wire. | ||
|
|
||
| Carrying the buffer's `ts` across was implemented and reverted: the | ||
| extractor bookmark is keyed on interaction `created_at`, so a batch | ||
| recovered after the bookmark moved was stored and then never extracted | ||
| — permanent silent loss of learning data on the offline-recovery path | ||
| this buffer exists for. The server stamping drain time is the lesser | ||
| evil until ingest ordering stops depending on caller-supplied time. | ||
| """ | ||
| _, turns = state.unpublished_slice( | ||
| [{"ts": 1700000000, "role": "User", "content": "x", "user_id": "p"}] | ||
| ) | ||
| assert "created_at" not in turns[0], turns[0] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test an existing created_at value, not only ts.
This fixture passes even if unpublished_slice forwards a pre-existing created_at. Include that key in the input record and assert it is absent from the emitted turn.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_state.py` around lines 547 - 560, The
test_buffer_timestamp_is_not_sent fixture must verify removal of an input
record’s existing created_at, not just omission of ts-derived output. Add a
created_at value to the unpublished_slice input record and retain the assertion
that the emitted turn lacks created_at.
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 27, 2026
#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.
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
1 participant
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.
Companion to ReflexioAI/reflexio#383.
Correction to this PR's original framing
The first version of this description claimed a correct 50-turn batch emitted 50 warnings because every turn carries
user_id. That was wrong — reflexio treatsuser_idandsession_idas benign request-level keys and never warns about them (_BENIGN_UNKNOWN_KEYS). I had added that suppression after writing the description. Measured against the real server:maintools_used[0].status,synthesised_bytools_used[0].statusSo the warning this branch actually removes is
synthesised_by, emitted once per session by the SessionEnd anchor. The dominant source wastools_used[*].status— fixed on the server in #383 by declaring the field, which both recovers the signal (it is"success"/"error"derived from the tool response, previously discarded) and removes the noise. No plugin change was needed for it.Why this is still worth landing
The denylist was the real problem regardless of how many warnings it produced:
It ships whatever a hook happens to write, and rots every time a hook adds a record key. A server that rejected rather than reported unknown fields would turn that rot into a publish failure
reflexio_adapter.pyswallows without advancing the watermark — the same batch then retries forever and nothing publishes. #383 tried exactly that and had to revert it precisely because of this pattern.Also fixes a real data-quality bug
The buffer records
tsat turn time but never sent it, so the server defaultedcreated_atto parse time. A buffer drained hours later — the offline-resilience case this buffer exists for — stamped every turn with the drain time. Now carried across ascreated_at.Tests
Contract tests pin the allowlist against the real
InteractionDataand assert a literal wire key set. The literal matters: a review proved that assertingset(turn) <= _INTERACTION_DATA_FIELDScompares against the same constant the slicer filters by, so it could never fail. Verified by runtime mutation that both tests now bite.444 tests pass. (Three test files fail on
maintoo — missingnpm cideps and the gitignored vendor bundle — so they are excluded, not caused here.)Verified no legitimately-needed field is lost: every
state.appendcall site was enumerated and each key classified as a realInteractionDatafield, bookkeeping, or converted (cited_items→citationsafter the comprehension). Round-tripped through the real model:unknown_field_names() == []for every emitted turn.Summary by CodeRabbit
created_atand other bookkeeping details in wire-ready interaction turns (server timestamps are used instead).user_id/created_atare not included in the wire payload.