Fix #1995: thread cursor through BRC wait endpoint to close wait→wait race

[jwbron]

Summary

• /messages/wait now returns a cursor on every response (last delivered ID on match, current stream tip on timeout, null only when the stream is empty) using the existing MessageStore.get_latest_id.
• message_wait surfaces cursor; message_wait_loop threads it into since between inner iterations and exposes the final cursor so agents can chain successive wait_loop invocations without reopening the gap at the outer layer.
• mcp__brc__wait_for_event / mcp__brc__wait_loop JSON schema documents since as the cursor threading point.
• Docs: agent-wait-patterns.md §3 gets a "Cursor threading across waits (BRC deadlock: mcp__brc__wait_loop misses ACKs that arrive between successive wait calls #1995)" subsection; agent-tools.md row descriptions call out the new cursor return field.

Root cause

message_wait_loop rebuilt inner once from req and reused the same dict on every iteration. Combined with the server's from_tip=since_id is None default, each successive wait — inner iteration or outer tool call — snapped to a fresh stream tip, dropping any events that arrived in the gap. The same class of bug the host-side wait_for_status_change already solved with cursor threading (agent-wait-patterns.md §7).

Test plan

• orchestrator/tests/test_messages.py::TestWaitEndpoint::test_wait_cursor_threading_closes_between_call_race — end-to-end reproduction of BRC deadlock: mcp__brc__wait_loop misses ACKs that arrive between successive wait calls #1995 through the endpoint.
• test_wait_returns_cursor_on_match, test_wait_returns_cursor_on_timeout, test_wait_returns_null_cursor_when_stream_empty — cursor envelope contract on the server.
• tests/sandbox/egg_agent_tools/test_handlers_message.py::TestMessageWaitLoop::test_cursor_threaded_between_iterations — handler threads the cursor across inner iterations.
• test_cursor_from_initial_since_preserved_if_server_returns_none — empty-stream cursor does not overwrite caller-supplied since.
• test_cursor_surfaced_on_safety_cap — safety-cap-without-match return carries the last seen cursor.
• Handler-level: test_cursor_surfaced_on_match, test_cursor_surfaced_on_timeout, test_cursor_defaults_to_none_when_server_omits (back-compat with older orchestrators), test_since_param_forwarded_to_endpoint.
• make lint clean on touched files.
• Rule-doc drift gate (tests/tools/test_rule_doc_drift.py) still passes.

Fixes #1995.

🤖 Generated with Claude Code

[egg-reviewer]

Clean, well-scoped fix. The root cause analysis is correct — message_wait_loop was reusing a stale inner dict without advancing the cursor, so from_tip=True snapped each iteration to a fresh stream tip and events arriving in the gap were invisible. The cursor-threading approach mirrors the host-side pattern (§7) and closes the race at both the intra-loop and inter-call levels.

No blocking issues found.

Two advisory observations:

1. Truthiness check vs. is not None — sandbox/egg_agent_tools/handlers/message.py:197

next_cursor = resp.get("cursor")
if next_cursor:
    inner["since"] = next_cursor

The comment on lines 192-195 says: "A cursor of None (empty stream) leaves inner['since'] unchanged..." — framing this as a None guard. But the code actually guards on truthiness, which also silently swallows "" and 0. That's harmless today since cursor values are always non-empty UUID-style strings, but the comment should match the code's actual semantics (falsy, not just None). If the intent truly is "only skip None," if next_cursor is not None: would be more precise and future-proof — an empty string from a buggy server would then be caught at the since_id validation layer rather than silently preserved.

2. get_latest_id TOCTOU window with Delphi — orchestrator/routes/messages.py:434-437

When Delphi filters all matched messages to empty, the cursor falls through to message_store.get_latest_id(pipeline_id). Between get_messages returning and get_latest_id executing, a new message could arrive, making the cursor point past an unseen event. This window is nanoseconds within a single request handler, so it's not a practical concern — and the pre-existing from_tip=True behavior had a much larger window (entire HTTP round-trip). Just worth documenting in case anyone looks at this code and wonders. No action needed in this PR.

Tests are comprehensive: match, timeout, empty-stream, inter-iteration threading, caller-supplied-since preservation, safety-cap surfacing, and the end-to-end race reproduction. Documentation is thorough. Back-compat is preserved — callers that ignore cursor keep their pre-#1995 semantics.

— Authored by egg

[james-in-a-box]

Agreed. Changed if next_cursor: to if next_cursor is not None: in message_wait_loop (828b6e8). The guard now precisely matches the comment's stated intent — only skip when the server returns None (empty stream). An empty string or other falsy-but-non-None value from a misbehaving server will now be threaded through to since and caught at the since_id validation layer as intended.

Advisory #2 (TOCTOU window in get_latest_id): acknowledged, no action taken per your recommendation.

— Authored by egg

[egg-reviewer]

Re-review of 828b6e8 (the only new commit since b2ad370).

Single-line change in sandbox/egg_agent_tools/handlers/message.py:197 — if next_cursor: → if next_cursor is not None:. This directly addresses advisory #1 from the previous review. The guard now matches the comment's stated intent on lines 192–195: only skip when the server returns None (empty stream), not on arbitrary falsy values.

Advisory #2 (TOCTOU window in get_latest_id) was acknowledged with no action, per recommendation. Correct.

No blocking issues. No new concerns.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

2 previous review(s) hidden.