Repository navigation
Compare delivery cursor timestamps chronologically and fail closed on lost precision - #22
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🔵 Needs a closer look
The change alters fail-closed acknowledgement semantics in the delivery-critical cursor path, so despite correct and well-tested logic it warrants final human review.
0 open findings
What changed in this PR
This PR fixes a correctness bug in the debate delivery pump's cursor-reached check (_cursor_reached in hooks/debate_pump.py). Previously the function compared delivery cursor timestamps as raw strings, so a whole-second trigger timestamp (e.g. ...T00:00:00Z) sorted after a cursor carrying fractional seconds (e.g. ...T00:00:00.123Z), which could leave an acknowledged delivery pending or let an older whole-second cursor satisfy a fractional trigger. The fix parses both sides as timezone-aware UTC instants, compares them chronologically before the existing message-ID tiebreak, and fails closed on missing/invalid/timezone-free timestamps as well as on nonzero sub-microsecond precision that datetime.fromisoformat would otherwise silently truncate into a false tie.
Changes:
- Rewrote
_cursor_reachedto compare UTC instants (then message IDs), fail closed on invalid/naive timestamps, and reject nonzero digits beyond microseconds (while tolerating extra zero digits). - Added regression tests covering precision, UTC offsets, equal instants, invalid/missing inputs, and both real (signal + watermark) temporary-DB acknowledgement paths.
| File | Description |
|---|---|
hooks/debate_pump.py |
Replaces lexicographic cursor/trigger timestamp comparison with UTC-instant comparison plus fail-closed handling for invalid/naive timestamps and sub-microsecond precision loss. |
tests/test_debate_pump_delivery_ack.py |
Adds parametrized and integration regressions for instant ordering, timezone offsets, equal-instant tiebreaks, invalid/missing timestamps, precision loss, and signal/watermark ack paths. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
When a trigger timestamp omits fractional seconds, a later cursor with fractional seconds sorts earlier as text. The pump can therefore leave an acknowledged delivery pending, or accept an older whole-second cursor for a fractional trigger.
Compare timezone-aware timestamps as UTC instants before applying the existing message-ID tie ordering. Missing, invalid or timezone-free timestamps fail closed. Nonzero precision beyond microseconds also fails closed so the datetime parser cannot silently discard time and create a false tie; extra zero digits normalize safely. Add regressions for precision, UTC offsets, equal instants and the real temporary-DB signal/watermark acknowledgement paths.
Addresses the timestamp comparison finding on PR #20. Only
hooks/debate_pump.pyandtests/test_debate_pump_delivery_ack.pychange.Validation: baseline focused suites 105 passed; initial regressions on the old source 17 failed / 11 passed. An independent review found a submicrosecond false tie in the first parser candidate; targeted regressions including both real delivery paths then failed 9 cases. Corrected pump/wake focused suites pass all 145 tests. Repository Ruff checks and
git diff --checkpass. Tests use the existing isolation and synthetic DB fixtures. Full Python 3.11 validation will run in GitHub CI.