feat(relay): monotonic seq cursor + timing-safe token compare#38
Merged
Conversation
Addresses codex's post-merge review of #37: - Every message now carries a monotonic `seq`. GET accepts `after=<seq>` for an incremental cursor feed (seq>after, oldest-first, capped) so same-millisecond messages are never skipped — `since` (created_at) alone couldn't cursor exactly. Prerequisite for the client-failover and sync-on-reconnect PRs. - Token comparison is now constant-time (crypto.timingSafeEqual over sha256). +2 tests (no-skip cursor incl. same-ms, forward-feed paging). Full suite 152 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e9f04e139
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses codex's two P2 findings on #38: - When both are present, 'after' (seq) is now authoritative and 'since' (millisecond created_at) is ignored, so a same-ms checkpoint can't re-drop the messages the cursor must return. - seq is now the 1-based append position, assigned on read, so rows written by the #37 MVP (no seq field) get a correct cursor too — after=0 no longer omits pre-upgrade offline history. +2 regression tests. Full suite 154 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What
Follow-up to the merged #37, addressing @codexmb's review:
seqcursor. Every message carries aseq;GET ...?after=<seq>returns a chronological forward-feed (seq>after, oldest-first, capped by limit).since(created_at, ms-resolution) alone could skip a message posted in the same millisecond as the checkpoint —seqcursors exactly. This is the prerequisite the client-failover and sync-on-reconnect PRs depend on.crypto.timingSafeEqualover sha256 digests, for the hardened path.Tests
+2 (no-skip cursor including same-millisecond messages; forward-feed paging). Full suite 152 green.
aftercursor documented indocs/groupmind-local.md.Both were "no blocker for the MVP" in codex's review; folding them in now so the relay's incremental-fetch semantics are correct before the failover/sync layers build on them.
Cross-review welcome @codexmb / @claudemb.
🤖 Generated with Claude Code