feat(watch): opt-in catch-up drain of unread inbox on fresh attach (#229)

[Masashi-Ono0611]

Implements the proposal in #229: an OPT-IN, flag/config-gated catch-up drain — on a fresh attach, emit the current unread inbox once before entering the live stream, with the watermark model preventing any replay on reconnect. Default behavior stays "start from now".

Motivation — a live occurrence of the race

Beyond the spawn-then-send case in #229, the same miss happens in the "send, then arm the monitor" order. Observed on a production team on 2026-07-23:

• 20:14:27Z — leader sends a consultation request (message id 163)
• 20:16:16Z — the peer replies (message id 164)
• 20:16:17Z — the leader's own watcher attaches, one second after the reply, and seeds its fresh watermark at MAX(id) = 164

The reply was never pushed to the leader (it sat at the just-seeded watermark) and was only discovered by manually running inbox.sh. With delivery.monitor.catchup enabled, the attach would have drained it immediately.

What counts as "unread at attach" (defined against the watermark model)

As #229 asks, the definition is fixed first and drives the implementation:

Rows addressed to the watcher's subscription (team, to_agent) with read_at IS NULL and id <= the just-seeded watermark, drained only on a fresh attach (no persisted watermark existed at startup).

Consequences:

• Reconnect never re-drains. A restart with a persisted watermark takes the existing watch.sh: Monitor restart silently drops unread messages (id>MAX(id) watermark skips the gap) #107 gap-fill path (resume strictly after the last delivered id) and skips the drain entirely — so history is not replayed on every reconnect, the failure mode Opt-in catch-up drain of unread inbox on watcher attach #229 warns about.
• Drain and live stream are mutually exclusive. The drain covers id <= LAST; the live loop covers id > LAST. A message that lands between watermark seeding and the drain query belongs to the live loop. Nothing is delivered twice.
• Control messages (ctrl:*) are excluded. They are live teardown directives; replaying a stale ctrl:despawn at attach could tear down a freshly spawned successor for the same role.
• The drain does not advance read_at. Streaming has never marked rows read (that is inbox.sh's job on display); keeping that invariant makes this change purely additive on the delivery path.

Gating and cap

• Opt-in via AGMSG_WATCH_CATCHUP=1 (env wins) or config.sh set delivery.monitor.catchup true. Default OFF — the pinned "fresh session starts from now and does not replay history" test passes unchanged.
• Drained rows are capped (default 50, AGMSG_WATCH_CATCHUP_CAP as a tuning/test seam). When the backlog exceeds the cap, the newest rows are kept — recent context is what an attaching agent needs — and a single notice line reports how many older rows to fetch via inbox.sh.

Relationship to #439 (live delivery marking read_at)

Independent but complementary. This PR does not depend on #439 and stands alone: it defines "unread at attach" purely against read_at IS NULL + the watermark, and deliberately does not mark drained rows read. If #439 lands, live-streamed messages start advancing read_at, which makes "unread" a more precise proxy for "never delivered" — the combination moves attach-time delivery close to exactly-once. Until then, the only overlap cost is that a drained-but-unread row may also appear in a later manual inbox.sh check, which is the current behavior for live-streamed rows too.

Notes for review

• config.sh's YAML helpers read one nesting level, so dotted keys round-trip as a flat monitor.catchup: entry under delivery: (same pre-existing behavior as delivery.monitor.poll_interval). The default template therefore documents the key with a comment only — no nested value line that yaml_get would never read. A parser extension felt out of scope for this PR.
• Bash 3.2 compatible (macOS default): no bash-4 expansions; the cap value is 10#-normalized so a leading-zero override cannot kill the watcher via octal arithmetic.

Tests

Six new bats tests in tests/test_watch.bats (all pass; existing watch-watermark suite 23/23 green locally):

1. catch-up ON drains queued unread on fresh attach, exactly once (live stream still works, no double delivery)
2. catch-up ON does not re-drain on reconnect with a persisted watermark
3. drain skips already-read messages
4. drain caps at the newest N with a notice
5. drain never replays control messages (stale ctrl:despawn does not tear down a fresh actas watcher)
6. enablement via the delivery.monitor.catchup config key

🤖 Generated with Claude Code

[fujibee]

@Masashi-Ono0611

Apologies for the long silence on this one — the delay was on our side, not a reflection of the PR.

Two mechanical things first, because they explain why this cannot merge as it stands and neither is about your code.

The required check names changed after this PR last ran. The bats suite is now split into parallel shards, and the required context is a summary job named bats rather than the per-OS jobs. Your last run has the old names, so the required check has never reported on this head and GitHub holds the PR as blocked regardless of content. Any push to the branch fixes that; a rebase onto current main is the cleanest since the branch is a few weeks behind. We rebase external PRs in-house rather than making contributors chase our churn, so say the word if you would rather we did it.

The macOS job on that run was cancelled, not failed. GitHub renders a cancelled check the same as a failing one, which is misleading here: it was our own macOS bats instability at the time, not anything your change did. Please don't spend time debugging it.

On the change itself, two points worth your judgement.

The rationale for not marking rows read no longer holds. The drain's comment says that watch.sh streaming has never advanced read_at, and that keeping that invariant leaves the change orthogonal to any future live-delivery read-marking. On current main that invariant is gone: watch.sh has a mark_read helper and the live loop calls it. So read-marking is present rather than future, and the drain now sits next to a live path that does mark. We are not asking you to make the drain mark — that may still be the right call — but the reasoning needs redoing against what main does now, and the consequence should be stated deliberately: drained rows stay unread, so a later inbox.sh re-surfaces exactly what was just shown.

The broad-watcher path needs the same exclusion the live path has, and a test. mark_read on main deliberately does nothing when a broad (non-actas) watcher handles a row for a role that owns its own exclusive ready sentinel — a broad watcher must not act on behalf of a role that has its own watcher. The drain selects on the subscription pairs with no equivalent check, so a broad watcher on a fresh attach would emit the backlog belonging to roles that each have their own exclusive watcher. That is the case we would want a test for specifically, since it is the one that does not show up in single-role manual testing.

Last, a scope note so the expectation is clear. AGMSG_WATCH_CATCHUP, AGMSG_WATCH_CATCHUP_CAP, and the delivery.monitor.catchup config key are new public interface. Interface additions here need fujibee to look at the design directly rather than being merged on review alone, so even once the above is settled there is a step after review that is not about code quality. Flagging it now rather than at the end.

[Masashi-Ono0611]

Thanks — and no need to apologise for the delay; the two mechanical notes saved me from debugging the wrong thing entirely.

I took the rebase myself rather than passing it back: current main is in, and the required bats summary check now reports on this head.

The read-marking rationale

You were right that the invariant it leaned on is gone — main's live loop calls mark_read, so the drain now sits beside a path that marks. Not marking is still the call, but as a choice rather than a consequence, and the comment says so with the trade named: drained rows stay unread, so a later inbox.sh re-surfaces exactly what was just shown. I kept it that way because catch-up is an additive convenience at attach, and having it silently consume the backlog would make the one command an operator uses to audit unread mail lie about what is outstanding.

The reconnect test now asserts that read state directly rather than leaving it to prose — the watermark suppresses replay whether or not a row was marked, so the absence of a drained row proved nothing about the no-mark choice on its own.

The broad-watcher exclusion

Fixed, and it was real. A broad watcher now builds its drain clause from only the roles with no exclusive ready sentinel. The clause is built at drain time so it sees the sentinels as they stand, and before this watcher writes its own, so an exclusive watcher never excludes itself.

Three tests, mutation-checked rather than trusted green:

• the exclusion itself — removing it fails only that test;
• a companion pinning that the rule applies to broad watchers only, without which an implementation that skipped every sentinel-owning role — breaking catch-up for the spawned agents that need it most — would also pass;
• the all-roles-owned no-op.

One honest gap: that third test does not pin the empty-clause guard. Dropping the guard interpolates AND (), and the drain's 2>/dev/null || echo 0 swallows the error into the same no-drain, no-notice outcome the test asserts. I verified by mutation that removing the guard leaves the suite green, and the comment says that rather than implying coverage it lacks.

Things my own review pass turned up

All the same description-versus-reality kind B3 was about on #507:

• FRESH_ATTACH keyed on the parsed watermark being empty, not on the file being absent. A watermark truncated by a kill mid-write also empties it, so a damaged reconnect took the fresh-attach path and replayed a backlog the session had already seen — "a reconnect never drains" only held for an intact watermark. Now keyed on the file, with a test that fails against the old behaviour.

• The reconnect test killed the first watcher as soon as its log line appeared, but the live loop prints before persisting the watermark, so it was measuring kill timing rather than the contract it named. Flaky 1-in-3; now syncs on the watermark advancing, 5 consecutive clean runs.

• AGMSG_WATCH_CATCHUP_CAP had no absolute ceiling. The drained rows are materialized into one shell variable, so a large enough cap traded "floods the Monitor stream" for "exhausts memory building the string". Bounded at 10k — which is a bound chosen to keep that string manageable, not a claim that no backlog reaches it.

• The "drained the newest N of M" notice was printed off the COUNT query, before the fetch. The fetch's failure is swallowed, so a failed fetch announced a drain and then emitted nothing. The notice now waits until rows are in hand. Not separately tested — forcing the fetch to fail while the count succeeds needs fault injection this suite has no seam for.

• The config-key test never cleared AGMSG_WATCH_CATCHUP. bats inherits the caller's environment, so an exported 1 would have let it pass with the config lookup broken. Cleared explicitly.

Full suite: 840 passing, 0 failing.

Known gap I did not close

Nothing exercises a team or agent name containing a quote through the generated clause. It reuses the same sql_escape as the existing WHERE_PAIRS builder, which has no such test either, so I left it rather than widen scope here — happy to add it if you would rather it landed with this.

Understood on the interface-addition step for AGMSG_WATCH_CATCHUP, AGMSG_WATCH_CATCHUP_CAP and delivery.monitor.catchup — thanks for flagging it up front rather than at the end.