Repository navigation
fix(routing): exact delivery proof and atomic queue acknowledgement - #20
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.
Copilot review overview
🔵 Needs a closer look
The change reworks concurrency-sensitive durable-delivery acknowledgement and transaction ownership, and the new test suite unconditionally imports the undeclared optional psutil, which would error the referenced CI gate—warranting human review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR hardens the debate "pump" routing path so a durable targeted-delivery queue row is only marked complete when there is genuine proof the recipient was reached. It fixes an incident where an implementation-tagged notify-only PING was treated as terminal and the queue was cleared with zero workers launched and no recipient acknowledgement. It separates cursor-terminal eligibility from durable delivery acknowledgement, introduces a versioned "successful-launch receipt" bound to the exact recipient/binding/worker identity, and revalidates acknowledgement inside the same owned BEGIN IMMEDIATE write transaction that completes the queue (closing the rebind-between-ACK-and-write gap).
Changes:
debate_pump.py: adds_delivery_acknowledged[_on_connection],_confirmed_delivery_receipt,_cursor_reached, and bounded withheld-delivery logging;_complete_pending_deliveriesnow runs under an owned write transaction and re-checks acknowledgement before writing.debate_wake.py: captures an authoritative pre-launch recipient/binding snapshot, rechecks it in the spawn-audit transaction, and records arouting_receipt_version:1/launch_confirmedreceipt plusbinding_generationonly when attribution is still current.- Adds
tests/test_debate_pump_delivery_ack.pywith extensive regression coverage (public rebind, exact receipt attribution, diagnostic isolation, mixed targets, bounded diagnostics, writer serialization, rollback/lock ownership).
| File | Description |
|---|---|
| hooks/debate_pump.py | Separates queue ACK from cursor-terminal; adds versioned-receipt/cursor acknowledgement checks and atomic guarded completion under BEGIN IMMEDIATE. |
| hooks/debate_wake.py | Captures and re-audits prelaunch binding identity; emits versioned successful-launch receipts with binding_generation. |
| tests/test_debate_pump_delivery_ack.py | New in-process temp-DB regression suite pinning the delivery-ACK contract; one unguarded optional-dependency import flagged. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @pytest.mark.parametrize("spawn_ok,rebind_race", [(True,False), (False,False), (True,True)]) | ||
| def test_real_launcher_receipt_records_prelaunch_parent_and_actual_worker(pump_db, monkeypatch, tmp_path, spawn_ok, rebind_race): | ||
| import db_utils | ||
| import psutil |
![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: d5458b1fdb
ℹ️ 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".
| @pytest.mark.parametrize("spawn_ok,rebind_race", [(True,False), (False,False), (True,True)]) | ||
| def test_real_launcher_receipt_records_prelaunch_parent_and_actual_worker(pump_db, monkeypatch, tmp_path, spawn_ok, rebind_race): | ||
| import db_utils | ||
| import psutil |
There was a problem hiding this comment.
On a clean Linux environment using the repository's declared .[dev,gui] dependencies, this unconditional import fails with ModuleNotFoundError before all three parameterized cases run; psutil is not listed in pyproject.toml, and unlike the existing psutil-dependent tests this test is not guarded by the Windows-only marker. This makes the full pytest CI job depend on an undeclared ambient package, so either mock the optional module without importing it or declare it as a test dependency.
Useful? React with 👍 / 👎.
| return bool(cursor and cursor["last_processed_ts"] and | ||
| (str(cursor["last_processed_ts"]), str(cursor["last_processed_msg_id"] or "")) >= (trigger_ts, trigger_id)) |
There was a problem hiding this comment.
When the trigger and cursor use different valid ISO-8601 precisions, this string comparison can order them incorrectly. For example, a trigger at 2026-10-03T00:00:00Z compares greater than a later cursor at 2026-10-03T00:00:00.500000Z because Z sorts after ., so an acknowledged delivery remains pending; the accepted timestamp format permits both representations, and Python's isoformat() can omit the fraction when microseconds are zero. Parse or normalize the timestamps before applying the (timestamp, msg_id) comparison.
Useful? React with 👍 / 👎.
Summary
Verification
Integration gate
The existing full CI (lint, manifest completeness, full pytest and core-only registry smoke) must succeed for this exact feature head before main integration. No runtime/live database/bridge deployment, schema/governance change, workflow modification or optional-package/native-Windows verification is claimed. Main integration requires separate ratification and postmerge CI.