feat(logstream): background watcher agents can be woken by, plus the monitoring protocol - #2315
Conversation
… rule Coordination stalls are usually listening failures, not protocol failures: a task sits open because the agent it was addressed to was never watching, and the requester cannot tell "working on it" from "nobody is home". The load-bearing correction is the cursor. Events are ordered by append order (ORDER BY rowid), not wall clock, so a peer's event is appended whenever it syncs and can already be older than a timestamp high-water mark. Resuming with since_created_at therefore drops late-arriving cross-replica events permanently. Measured one such inversion in a single 50-event window: a windows-origin event created 09:10:48Z ingested after a mac-origin event created 09:13:21Z. list_events' docstring already said since_event_id is "the precise cursor ... regardless of timestamp ties" — that just never reached an agent. - coordination-protocol.md: new "Monitoring the stream" section — the cursor rule, four modes (inbox sweep / long-poll / SSE / declared-idle), the announce-your-watch convention, and declaring when you are NOT watching. Two new hard rules. - coordination-protocol.md: system-prompt snippet updated, since that block is copied verbatim into every agent's instructions. - agent-logstream.md: matching "Monitoring" concepts section. - mcp_server.py: event_list and event_wait descriptions now state the cursor rule at the point of use, and both since_created_at params are marked "NOT a resume cursor". The announce-your-watch shape is generalized from windows:grok:mempalace's issue-354-live-layer announcement, which named its filter, its cursor, and the work not to duplicate. Docs and tool descriptions only; no behavior change.
…an be woken by `logstream wait` is a primitive, not a watcher. It caps at MAX_WAIT_TIMEOUT_MS and reports a timeout, so every caller writes the same re-arm loop and each one has to remember to carry the cursor forward. Agents mostly did not, so coordinated tasks stalled on nobody listening rather than on the work. Two filters a watcher needs cannot be expressed by list_events, whose SQL is single-valued and positive-only: * "wake me for task.request OR patch.ready" * "everything addressed to me EXCEPT my own events" The second is not a nicety. to_agent=<me> deliberately also matches '*' broadcasts, and an agent's own broadcasts are broadcasts — so a hand-rolled watcher wakes itself every time it posts a status. Found by running one: mac-claude woke on its own fleet announcement within seconds. - logstream.watch_events(): owns the re-arm loop and the cursor. Yields ([], cursor) on idle polls so callers can time out, heartbeat, or checkpoint without a second clock. The cursor advances past every event examined, not only matches, so a restart never re-judges what it rejected. - event_matches_watch() / normalize_watch_values() / pushdown_watch_filters(): single-valued filters push down to SQL to keep the query selective; multi-valued and negative parts are re-checked client-side. Pushdown is an optimization, never a correctness dependency. - read/write_watch_cursor(): atomic temp+rename. A corrupt state file costs a replay; refusing to start costs every event after it, so reads degrade to None rather than raising. - CLI `logstream watch`: --agent (shorthand for --to-agent X --exclude-from-agent X), repeatable filters meaning "or", --state-file, --follow, --idle-exit-ms. Exit 0 on a match, 2 on idle — the same convention `wait` uses, so a harness can background the process and treat its exit as "you have mail". Docs updated to lead with it: coordination-protocol.md (including the system-prompt snippet every agent copies), the agent-logstream concepts page, and the CLI reference. 20 tests: self-exclusion at both the matcher and CLI level, multi-valued ORs, cursor advance-past-rejected, resume-without-replay, --since-event-id overriding the state file, and the exit-code contract.
![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
mempalace/mempalace/mcp_server.py
Lines 5534 to 5536 in 581efdf
Stop recommending the matched event as a cursor
The preview guidance tells MCP clients to retrieve an event's full body using that event as since_event_id, but the schema immediately above defines this cursor as strictly after the supplied event. An agent following the instruction will exclude the very event whose verbatim body it needs; recommend repeating the original targeted filters with preview=false (or add an exact event-id lookup) instead.
AGENTS.md reference: AGENTS.md:L22-L22
ℹ️ 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".
| for matched, cursor in ls.watch_events( | ||
| cursor=cursor, | ||
| poll_timeout_ms=args.poll_timeout_ms, | ||
| limit=args.limit, |
There was a problem hiding this comment.
When --idle-exit-ms is shorter than --poll-timeout-ms, the deadline is not checked until the current long poll finishes. With the CLI defaults, --idle-exit-ms 1000 can therefore take roughly 300 seconds to exit instead of one second, defeating harnesses that depend on the documented idle wake signal. Pass the remaining idle duration as the effective poll timeout, or otherwise arrange for the deadline to interrupt the poll.
Useful? React with 👍 / 👎.
| # Checkpoint every advance, including idle ones: the cursor moves | ||
| # past events that were examined and rejected, and re-judging them | ||
| # after a restart is pure waste. | ||
| write_watch_cursor(args.state_file, cursor, agent=args.agent) | ||
| if matched: |
There was a problem hiding this comment.
When a match arrives, the state cursor is persisted before the event is written to stdout. If the watcher is killed, encounters a broken pipe, or crashes between these operations, the harness never receives the match, but the next watcher resumes after it and silently skips that coordination event. Checkpoint matched batches only after successful output so failures may cause a safe replay rather than lost work.
Useful? React with 👍 / 👎.
…iew cursor docs Codex P2s on the watch PR, confirmed on Windows: - Cap each wait_events timeout to remaining --idle-exit-ms so idle 400ms does not wait out a 300s long-poll (measured 2687ms -> 572ms). - Persist the watch cursor after successful stdout on a match; unmatched advances still checkpoint immediately. A broken pipe no longer skips the event on resume. - preview=true docs told clients to re-fetch via since_event_id of the truncated event, but that cursor is strictly after it. Repeat the original filters with preview=false instead.
|
Windows-side check of the Codex P2s, now on the branch as
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d3f21678b
ℹ️ 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".
| with open(path, "r", encoding="utf-8") as handle: | ||
| cursor = json.load(handle).get("cursor") |
There was a problem hiding this comment.
When the state file contains valid JSON that is not an object (for example null, [], or a JSON string), json.load(handle).get(...) raises AttributeError, so the watcher fails to start instead of degrading to a replay as the function's corruption-recovery contract promises. Validate the decoded value before calling .get(), or include AttributeError in the recovery path.
Useful? React with 👍 / 👎.
| except KeyboardInterrupt: | ||
| if not as_json: | ||
| print("Stopped.", file=sys.stderr) |
There was a problem hiding this comment.
When the operator presses Ctrl-C or a supervisor sends SIGINT before any event is printed, this handler consumes KeyboardInterrupt and returns normally, producing exit code 0. Since the documented contract tells harnesses that exit 0 means a match was printed, an interrupted watcher is falsely reported as having mail; re-raise the interrupt or exit with the conventional nonzero status.
Useful? React with 👍 / 👎.
| timeout = poll_timeout_ms() if callable(poll_timeout_ms) else poll_timeout_ms | ||
| if timeout is not None and int(timeout) <= 0: | ||
| # Idle deadline already elapsed: yield immediately so the | ||
| # caller can exit without waiting out the next long-poll. | ||
| yield [], cursor | ||
| continue |
There was a problem hiding this comment.
When logstream watch is invoked with --poll-timeout-ms 0 and the default unlimited idle duration, the callable returns zero on every iteration, this branch yields immediately, and the caller re-enters it forever without sleeping, consuming a CPU core while never performing a database poll. Reject nonpositive configured poll timeouts or distinguish an expired idle deadline from a user-supplied zero.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
… the log windows:grok:mempalace flagged the first-run replay on PR #2315. Measured on the real shared brain: a fresh `logstream watch --agent mac-claude` with no cursor woke holding 41 events, the oldest 49 days old. Nothing in the payload marks them stale, so an agent reads July's task.requests as new work — the opposite of the problem this command exists to solve. `latest_event_id()` already documents the right behaviour for the SSE live-tail: capture the tip at connect time "so they receive only post-connect events". The CLI watcher not doing the same made one product with two first-run semantics. - default: a watch with no --since-event-id and no stored cursor starts at the tip. Backlog belongs to the inbox sweep (`logstream list`), which can page it deliberately; a watcher is for what arrives from now on. - never silent: it prints what it skipped and how to get it, on stderr, in --json mode too, so the note cannot corrupt a parsed payload. - --from-start opts back into the replay. Six existing tests turned out to depend on the replay, which is the evidence this mattered. Their intent is filtering, not first-run behaviour, so the shared args helper opts them into --from-start and three new tests cover the tip default explicitly — including that an explicit --since-event-id or a state-file cursor still wins over it. Note for review: test_agent_shorthand_does_not_wake_on_your_own_broadcast asserts exit 2, which the tip default would satisfy for the wrong reason. It opts into --from-start so it keeps testing the exclusion. 4395 passed, 31 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8663ecdc5d
ℹ️ 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".
| # stale — measured 41 events, the oldest 49 days old, on a real | ||
| # shared brain. Backlog is the inbox sweep's job; a watcher is for | ||
| # what arrives from now on. Never silent: say what was skipped. | ||
| cursor = ls.latest_event_id() |
There was a problem hiding this comment.
When --state-file is unreadable, empty, or malformed, read_watch_cursor() returns None, and this assignment treats that recovery case exactly like a first run by jumping to the latest event. Any matching events appended after the last good checkpoint but before restart are then silently skipped, and the state file is subsequently checkpointed at the new tip. Distinguish an absent first-run state from a failed cursor read, or recover from a safe earlier position instead of applying the live-tail default.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| print( | ||
| json.dumps( |
There was a problem hiding this comment.
With --follow --json, two matching polls cause this branch to print two indented JSON documents back-to-back, and an idle exit may append a third document. The resulting stdout is not valid JSON—standard consumers such as json.load or jq fail with trailing/extra data—even though --json is advertised as machine-readable and --follow is intended for daemons. Emit NDJSON/JSON-seq records or another explicitly framed stream format.
Useful? React with 👍 / 👎.
Seven inline P2s on 8663ecd. Two were stale — Codex anchored them to lines 0d3f216 had already fixed. The other five were real, and all five are mine. (a) read_watch_cursor: a state file holding valid JSON that is not an object (null, [], a bare string) made .get() raise AttributeError, which the except clause did not catch — so the watcher refused to start, the exact opposite of the recovery contract in its own docstring. (b) KeyboardInterrupt returned normally, exiting 0. Exit 0 is the documented "a match was printed" signal, so SIGINT told a supervisor it had mail that never arrived. Now 130. (c) --poll-timeout-ms 0 with no idle deadline made the callable return zero forever, so watch_events' expired-deadline branch yielded without ever polling and burned a core. Rejected at the CLI, so that branch can now only mean a deadline actually elapsed. I saw this shape while reviewing 0d3f216 and wrongly filed it as unreachable in-tree. (d) A regression I introduced in 8663ecd. read_watch_cursor returns None for both "no state file yet" and "state file present but unreadable"; the tip-default treated the second like the first, skipping everything since the last good checkpoint and then re-checkpointing at the tip, making the loss permanent. A failed read now replays and says so, which is what read_watch_cursor promises: "a corrupt state file costs a replay". (e) --follow --json printed repeated indented documents, which json.load and jq reject as trailing data — a machine-readable flag that was not, on the mode built for daemons. Follow now emits NDJSON, one compact record per line; single-shot keeps the one pretty document. Five regression tests, each verified to fail against 8663ecd. The --poll-timeout-ms one initially passed pre-fix for the wrong reason — an idle timeout also exits nonzero — so it now pins the validation error itself rather than "some nonzero exit". 4400 passed, 31 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57d8d24493
ℹ️ 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".
| cursor = ls.latest_event_id() | ||
| skipped_from = cursor |
There was a problem hiding this comment.
When the first stateful watch starts against an empty log, latest_event_id() returns None, and write_watch_cursor() later ignores that falsey cursor, so no state file is created. If the watcher then exits on idle and an event arrives while it is stopped, the next launch again appears to be a fresh watch and this line jumps to that event as the new tip, silently skipping it forever; persist an initialized empty-log sentinel or otherwise distinguish this restart from a true first run.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
…behind Third Codex round, one P2 on 57d8d24, and a third variant of the same family as the last two: "no cursor" is not one fact. Sequence: a stateful watch starts against an empty log, so latest_event_id() is None and write_watch_cursor ignored the falsey cursor, leaving no file. The watcher idles out. An event arrives while it is stopped. The next launch sees no state file, concludes first run, jumps to that event as the new tip, and skips it — then checkpoints past it, making the loss permanent. The root cause both this and the previous round shared is that a missing cursor was being read as a single condition when it is four: absent no state file — a genuine first run, may start at the tip ok a usable cursor, resume strictly after it empty file exists recording cursor: null — started against an empty log, has not seen an event yet. NOT a first run. corrupt unreadable, truncated, or valid JSON that is not an object Only `absent` may start at the tip. `empty` and `corrupt` replay. read_watch_state returns (cursor, condition) and the CLI branches on the condition; read_watch_cursor stays as a wrapper for callers that do not care. write_watch_cursor now persists a null cursor rather than skipping the write, and the starting position is recorded at startup instead of waiting for the first idle yield, so a kill in between cannot reproduce the same hole. Three regression tests, each verified to fail against 57d8d24, including the full arrives-while-stopped sequence end to end. 4403 passed, 31 skipped. CI green on 57d8d24 across all nine checks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b35bfc113a
ℹ️ 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".
| if not path or not os.path.exists(path): | ||
| return None, WATCH_STATE_ABSENT |
There was a problem hiding this comment.
When an existing checkpoint's parent directory temporarily lacks traversal permission, os.path.exists(path) can return False, so the watcher classifies the checkpoint as absent and starts at the current tip instead of taking the corruption-recovery replay path. This can permanently skip events since the stored cursor; fresh evidence in this revision is this preflight existence check, which bypasses the subsequent OSError handler. Attempt the open directly and distinguish FileNotFoundError from other access failures.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| except OSError: | ||
| logger.debug("could not persist watch cursor to %s", path, exc_info=True) |
There was a problem hiding this comment.
When the configured state path is unwritable or the disk is full, this handler silently discards the initial tip checkpoint and lets the watcher continue. If it subsequently idles out, an event arrives while it is stopped, and it restarts, the still-absent state file makes the restart capture that event as the new tip and skip it permanently. The initial checkpoint must report failure or abort rather than using the best-effort behavior that is safe only for later checkpoints.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
…one must not fail quietly Fourth Codex round, two P2s on b35bfc1. Same family as rounds two and three — both are ways a *present* checkpoint gets mistaken for a first run — so both are checked against the invariant rather than patched individually: a restart may cost a duplicate, never a missed delegation. read_watch_state no longer preflights with os.path.exists(). That call answers False both for "no such file" and for "cannot traverse the parent directory", so a checkpoint that exists but is momentarily unreachable was classified absent, started at the tip, and skipped every event since the stored cursor. Opening directly lets FileNotFoundError mean absent and every other OSError — permissions, a directory in the way, I/O error — mean corrupt, which replays. It also closes the exists()/open() race. write_watch_cursor gains required=. Best effort is the right default because a lost checkpoint costs a replay while a crashed watcher costs every event after it. That trade-off inverts for the first checkpoint of a fresh watch: if it never lands there is no state file, the next launch calls itself a first run, and it starts at the tip — so a swallowed failure there guarantees the exact skip the checkpoint existed to prevent. The CLI now refuses to start rather than continuing into that. Three regression tests, each verified to fail against b35bfc1. The unreachable-file case is exercised twice, via a directory standing where a file is expected (OSError on every platform) and via a simulated PermissionError, so it holds on the Windows runner too. 4406 passed, 31 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9b123743
ℹ️ 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".
| cursor = args.since_event_id or stored_cursor | ||
| skipped_from = None | ||
| if cursor is None and not args.from_start and state_condition == WATCH_STATE_ABSENT: |
There was a problem hiding this comment.
When the first stateful invocation supplies --since-event-id and the state file is absent, this bypasses the only immediate checkpoint below, so the cursor is not persisted until watch_events() yields—which can take the default five minutes. If the watcher is interrupted before then, an event arrives while it is stopped, and the next invocation relies on the state file, that file still appears to be a fresh watch and the restart jumps to the new tip, permanently skipping the event. Persist the explicit initial cursor with the same required-write behavior used for the tip cursor.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
…ced it Fifth Codex round, one new P2 on dd9b123, and the sixth instance of one bug: a starting cursor that never reaches disk, so the next launch calls itself a first run and jumps to the tip. This time --since-event-id with no state file skipped the immediate checkpoint entirely, because that write lived inside the "resolve the tip" branch and an explicit cursor never enters it. Nothing was persisted until watch_events yielded, which by default is up to five minutes later; an interrupt inside that window leaves no file at all. Rather than add a third call site I hoisted it: one place resolves the starting position, one place records it. The position can arrive three ways — an explicit --since-event-id, the tip, or None from an empty log or --from-start — and all three now take the same required write whenever no state file exists yet. That removes the shape the last four rounds kept finding new instances of, instead of removing one more instance. Two test-quality notes, both the same lesson: The first version of this test passed against the unfixed code. The helper uses a 60ms poll timeout, so watch_events yielded immediately and the loop's own checkpoint wrote the file — the startup window the bug lives in never opened. It now stubs watch_events to interrupt before yielding, which is what actually pins the startup write, and covers all three entry paths. The hoist also broke test_match_does_not_checkpoint_if_output_fails, which asserted that no state file existed after a failed match write. That was pinning the symptom: a startup checkpoint now legitimately exists. The invariant is that its cursor must still be the pre-match position so the undelivered event replays, so it asserts the cursor value instead. 4407 passed, 31 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5c4af4da4
ℹ️ 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".
| if args.state_file and state_condition == WATCH_STATE_ABSENT: | ||
| try: | ||
| write_watch_cursor(args.state_file, cursor, agent=args.agent, required=True) |
There was a problem hiding this comment.
When a fresh state file is combined with a typo or stale --since-event-id, this write persists the cursor before list_events() verifies that the anchor exists. The first poll then raises an uncaught ValueError, while the invalid ID remains in the state file; every subsequent invocation without the flag reloads that ID and fails again until the user manually deletes the checkpoint. Validate the cursor before the required startup write and report an ordinary CLI error without poisoning persistent state.
Useful? React with 👍 / 👎.
| idle_s = args.idle_exit_ms / 1000.0 if args.idle_exit_ms and args.idle_exit_ms > 0 else None | ||
| deadline = time.monotonic() + idle_s if idle_s else None |
There was a problem hiding this comment.
When --idle-exit-ms is negative, this conditional maps it to None, the same behavior as the documented 0 = wait forever. A negative value produced by configuration or timeout arithmetic therefore silently disables the idle deadline and can leave a harness waiting indefinitely instead of exiting; reject negative values explicitly.
Useful? React with 👍 / 👎.
…tive idle as forever Sixth Codex round, two P2s on b5c4af4. A typo'd or stale --since-event-id was persisted by the required startup write before anything checked it. list_events raises on an unknown anchor and that ValueError was uncaught, so the run died with a traceback while the bad id sat in the state file — and every later run without the flag reloaded it and died the same way until someone deleted the file by hand. The cursor is now verified before it is written anywhere. That check distinguishes where the cursor came from, because the right answer differs. An explicit --since-event-id that does not resolve is user error: refuse, and leave nothing behind to reload. A *stored* cursor whose event has gone — log rebuilt, replica reset — is corrupt state, and refusing would strand the watcher exactly as an unreadable file would, so it replays with a notice instead. Same invariant as the rest of this branch: a duplicate, never a missed delegation. --idle-exit-ms now rejects negatives. Only 0 documents "wait forever", but a negative took that same branch silently, so a value arriving from config or from timeout arithmetic would leave a harness waiting on a watcher it believed would time out. Three regression tests. The negative-idle one cannot simply be run against the old code to prove it: the pre-fix behaviour is an infinite wait, so the tripwire check hangs rather than fails, and confirming it needed an external timeout. Worth knowing before someone "fixes" that test by shortening it. 4410 passed, 31 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af9b7e2596
ℹ️ 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".
| "streams": normalize_watch_values(args.stream), | ||
| "rooms": normalize_watch_values(args.room), | ||
| "types": normalize_watch_values(args.type), | ||
| "statuses": normalize_watch_values(args.status), |
There was a problem hiding this comment.
When a repeatable filter contains multiple values, pushdown_watch_filters() omits it from list_events(), so the normal routing, event-type, and status sanitizers never run and event_matches_watch() compares the raw strings. For example, --type Task.Request --type Patch.Ready is silently accepted and can leave the watcher waiting forever instead of reporting the invalid event types; whitespace in a repeated routing value similarly prevents intended matches. Validate each filter member before constructing the watch specification.
Useful? React with 👍 / 👎.
| for matched, cursor in ls.watch_events( | ||
| cursor=cursor, | ||
| poll_timeout_ms=_poll_timeout_ms, | ||
| limit=args.limit, |
There was a problem hiding this comment.
When --limit is zero or negative, argparse accepts it and the first generator iteration raises ValueError from list_events(). This loop catches only KeyboardInterrupt, so the command emits a traceback rather than using _logstream_fail(); under --json, machine consumers receive no JSON error document. Validate the limit before starting or translate polling ValueErrors into the normal CLI error response.
Useful? React with 👍 / 👎.
… polling Seventh Codex round, two P2s on af9b7e2, and the same shape twice: input that was only rejected once the loop had already started. Filter validation depended on arity. list_events sanitizes what it is given, so a single-valued watch filter was checked for free by being pushed down, while a repeated one was not pushed down and got compared raw. `--type Task.Request` alone was an error; `--type Task.Request --type patch.ready` was accepted and then matched nothing, so the watcher waited forever for an event type that cannot exist. sanitize_watch_spec now runs every value through the same sanitizers list_events uses, whatever the arity, and normalizes them so a padded routing value still matches. --limit 0 was accepted by argparse and raised inside the first poll, where the only handler was for KeyboardInterrupt: a traceback, and under --json no error document at all. Validated up front. Both are argument checks, so they belong together and ahead of everything else: _watch_spec now validates and builds the whole spec before any polling, cursor resolution, or checkpoint write. That ordering is the point — round six's bug was a bad cursor being persisted before it was validated, and keeping validation in one place ahead of side effects is what stops the next one. Added a ValueError backstop around the loop as well. Every known bad input is rejected earlier now, but an escape would otherwise surface as a traceback, and under --json as nothing a machine consumer can parse. Four regression tests, each verified to fail against af9b7e2. 4414 passed, 31 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3ad155f38
ℹ️ 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".
| since_event_id=cursor, | ||
| limit=limit, | ||
| poll_interval_s=poll_interval_s, | ||
| **pushdown, |
There was a problem hiding this comment.
When replaying from a null/old cursor with a single-valued filter, pushing that filter into wait_events() prevents rejected rows from ever reaching the code that advances cursor. For example, --from-start --status ready against a large backlog containing no ready events repeatedly scans the same post-cursor rows throughout every long poll (and again after every re-arm), especially badly because status, room, and from_agent lack indexes. Preserve a scanned high-water mark or process bounded rowid batches client-side so rejected backlog is checkpointed once rather than rescanned indefinitely.
Useful? React with 👍 / 👎.
…lop merge Fallout from merging develop, not a new feature: `_HTTP_LOCK_FREE_TOOLS` (MemPalace#2315) narrowed the dispatch lock the handoff watchdog relies on, and `_dispatch_stdio_request` (MemPalace#2312) became the single stdio entry point. The stdio loop used to take `_REQUEST_DISPATCH_LOCK` around the whole request. Keeping that would have put a five-minute `mempalace_event_wait` long-poll in front of every handoff: the watchdog would find the lock held for minutes by a call that touches no storage at all. Wrapping the hub forward would have been worse — it would hold the barrier across a network round trip. So the policy moves into `_dispatch_locally`, shared by both transports: lock-free for the logstream set (every handler goes through `_call_logstream`, which is also why they are exempt from the writer lease), dispatch lock for everything that can reach Chroma or the KG. Local handling inside `_dispatch_stdio_request` — both the no-hub path and the unreachable-hub fallback — now goes through it, so a proxied session keeps the barrier for the requests it answers itself. Two tests pin both directions, probing the handoff from the main thread while a request runs on another: the dispatch lock is reentrant, so probing from the dispatching thread would take its own credit and prove nothing. docs/writer-lease-handoff.md records the exemption and the rule it implies for MemPalace#1984, and gains the section this thread asked for: where you control the deployment, one `mempalace serve` process is the better answer than several direct writers trading a baton.
Coordination friction on the shared brain is usually a listening failure, not a protocol failure: a task sits
openbecause the agent it was addressed to was never watching, and the requester cannot tell "working on it" from "nobody is home".Two commits — the protocol, then the tool that makes it practical.
logstream watchlogstream waitis a primitive, not a watcher. It caps atMAX_WAIT_TIMEOUT_MSand reports a timeout, so every caller writes the same re-arm loop and each one has to remember to carry the cursor forward. Most did not.Two filters a watcher needs cannot be expressed by
list_events, whose SQL is single-valued and positive-only:task.requestorpatch.ready"The second is not a nicety.
to_agent=<me>deliberately also matches*broadcasts, and an agent's own broadcasts are broadcasts — so a hand-rolled watcher wakes itself every time it posts a status. This was found by running one: it woke on its own fleet announcement within seconds.mempalace logstream watch \ --agent mac-claude \ --type task.request --type patch.ready \ --state-file ~/.mempalace/watch/mac-claude.json --json--agent IDis shorthand for--to-agent ID --exclude-from-agent ID--state-fileresumes exactly; the cursor advances past events examined and rejected, not only matches0on a match,2on--idle-exit-ms— same convention aswait, so a harness can background the process and treat its exit as "you have mail"--followstays alive past the first match, for daemonsSingle-valued filters push down to SQL to keep the query selective; the multi-valued and negative parts are re-checked client-side. Pushdown is an optimization, never a correctness dependency.
The cursor rule
Events are ordered by append order (
ORDER BY rowid), not wall clock. Across replicas those diverge: a peer's event is appended whenever it syncs, so it can already be older than a timestamp high-water mark. Resuming withsince_created_attherefore drops late-arriving cross-replica events permanently.Measured one such inversion in a single 50-event window: a windows-origin event created
09:10:48Zwas ingested after a mac-origin event created09:13:21Z.list_events' docstring already saidsince_event_idis "the precise cursor … regardless of timestamp ties" — that just never reached an agent. It now appears in the tool descriptions at the point of use, and bothsince_created_atparams are marked "NOT a resume cursor".Docs
coordination-protocol.md— new Monitoring the stream section: cursor rule, the modes, the announce-your-watch convention, and declaring when you are not watching. The system-prompt snippet is updated, since that block is copied verbatim into every agent's instructions.website/concepts/agent-logstream.mdandwebsite/reference/cli.md— matching sections.The announce-your-watch shape is generalized from an existing fleet announcement that named its filter, its cursor, and the work not to duplicate.
Testing
20 new tests: self-exclusion at both matcher and CLI level, multi-valued ORs, cursor advance-past-rejected, resume-without-replay,
--since-event-idoverriding the state file, corrupt state files degrading rather than raising, and the exit-code contract.Full suite 4390 passed, 31 skipped; ruff clean. Also exercised end-to-end against a live 165k-drawer palace and a real multi-machine fleet — the watcher is currently running as this repo's own coordination watcher.