fix(protocol): chat-sync preservation — null reader floor, publisher-derived invariant, drop the dead clone seed - #1252
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (2)
Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 1 per hour. Summary by CodeRabbit
WalkthroughThe chat-sync head schema now defaults ChangesChat-sync compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adjusts chat-sync compatibility metadata and removes unused clone code; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…o the 1.1 floor The writer shape narrowed minReaderVersion to z.literal(1)/z.literal(1), which made two things true that should not have been: - a null floor was unpublishable, so the host had to stamp a floor it had no justification for. Per COMPATIBILITY.md the minimum is for a change an older reader cannot safely INTERPRET, and the 1.1 reshape is additive and read-safe - its cut-plan fields ride the residual bags for a reader that does not model them. Stamping the floor is what produced the dev-host refusal "requires a reader on 1.1; this reader is 1.0" for a chat every 1.0 reader could open; - the pin was a latent publish-breaker. It matched this contract's own version, so bumping CHAT_SYNC_SCHEMA_VERSION to 1.2 would have made a publisher's own minReaderVersion stamp fail its own .parse() - breaking every publication on the build that shipped the bump. minReaderVersion now inherits the shared shape's nullable default. CHAT_SYNC_1_1_READER_FLOOR stays as the documented mechanism for a future DELIBERATE raise (and as refineClaimedCutPlanCompleteness' 1.1 threshold); its doc records that no writer stamps it today, and why. Also codifies the publisher-derived-levels invariant that makes residual capture's coverage a deliberate boundary rather than an accident: the head's part entries, cdc and hostPrivateShard are re-derived from the owner's op log on every publish, so durable chat data must never be added there. Loss across a host downgrade costs at most one full recut, never data. Recorded in COMPATIBILITY.md 6, in the bump ritual table, and beside the captured-level manifest where the next person adding a level will read it. Fixtures now state what a correct publisher stamps (no floor), which is also what keeps the store's byte-for-byte republication comparison honest. Signed-off-by: Tanveer Gill <tanveer@traycer.ai>
…tually true Two overstatements from the previous commit, both found by cold review, both the kind a future reader would quote back: - "the cut-plan fields ride the residual bags" is false for the PER-PART fields. `chatHeadPartSchema` is a plain object with no capture site - by the same publisher-derived-levels rule this branch codifies - so an older republisher strips them. Only head-level `cdc` rides the head's bag. The null-floor conclusion is unchanged and now rests on what actually holds: a 1.0 reader takes each part entry as the address it is and re-derives its own cut plan from its op log, so it renders and re-publishes correctly without modelling any of it. - "re-derived from the owner's op log on every publish" is not how two of the three levels behave. The extend road re-emits unchanged cohort entries verbatim and reads `cdc` back to confirm the plan has not moved. They are re-DERIVABLE, and derived afresh on every full recut; the sentence that carries the rule is the next one - the only round-trip any of them makes is through the owner's own predecessor head. Signed-off-by: Tanveer Gill <tanveer@traycer.ai>
`buildChatCloneSeed` / `chatCloneResidualsOf` derived a lossless clone seed client-side and had exactly one consumer: their own test. The clone that ships goes through the HOST fork path (`epic.createChat`'s `forkSource`), which now carries preserved raw bodies itself rather than reparsing them through the live message schema. Left in place, the module reads as the lossless clone path while being neither reached nor maintained - the misleading half of dead code. The idea it existed to state (messages and events cross as the preserved pairs assembly produced, never rebuilt from their interpreted view) now lives where the copy actually happens. Signed-off-by: Tanveer Gill <anay@traycer.ai> Signed-off-by: Tanveer Gill <tanveer@traycer.ai>
`captured-levels.ts` pointed at `chatCloneResidualsOf` as "the client surface [that] states the gap explicitly" for the un-carryable `shard` bag. That function went with the dead cloud-chat clone seed builder, so the contract doc for the level manifest was naming a symbol that no longer exists. Rewritten to say the thing the symbol was standing in for: a shard bag never reaches a re-publishing reader at all, so "we chose not to carry it" and "it never reached us" are indistinguishable on the other side - which is why the rule has to be stated here rather than enforced downstream. The cross-boundary completeness guard is described by ROLE rather than by symbol. A consumer holding an exhaustive `Record<CapturedResidualLevelId, …>` still makes a new level a compile error across the package boundary (the Traycer host's chat-sync record adapter is that consumer today), but this file lives in a different repository from it and must not depend on its names. Signed-off-by: Tanveer Gill <anay@traycer.ai> Signed-off-by: Tanveer Gill <tanveer@traycer.ai>
facf7be to
e9657ed
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
Two semantic resolutions: - chat-sync `minReaderVersion`: #1252's null reader floor wins over our 1.1 pin. Our 1.2 bump adds `chat.imported` through the unknown-variant passthrough, which is exactly the additive class that must NOT raise the floor, so both fixtures now stamp what a correct publisher on this line stamps - `null` in `publishChat`, a deliberate floor only where the test is about a deliberate floor. - `chatSubscribeCommonServerFrameSchemasPreManagedCommand` takes BOTH sides: our `event: chatEventSchemaPreImported` freeze and main's `action: chatActionSchemaV15`. The freeze pin is unchanged - `chat.subscribe` still tops out at the unreleased @1.6, and the newest released baseline is still @1.5. Signed-off-by: Amiteshwar Randhawa <amiteshwar04@gmail.com>
Protocol half of the chat field-preservation train (companion host PR in the internal repo pins this branch).
What
Verification
🤖 Generated with Claude Code