Skip to content

fix(omp): persist and deliver watcher wake batches - #119

Merged
dnth merged 7 commits into
mainfrom
fm/port-3123b-omp-watcher-notify
Sep 6, 2026
Merged

fix(omp): persist and deliver watcher wake batches#119
dnth merged 7 commits into
mainfrom
fm/port-3123b-omp-watcher-notify

Conversation

@dnth

@dnth dnth commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Intent

Port upstream kunchenguid/firstmate kunchenguid#3123 Part B: deliver the durable watcher wake batch to the OMP primary via a hidden nextTurn + triggerTurn, with claim persistence for same-session reload and replacement/process replay, retiring claims ONLY after fm-wake-drain acknowledges the durable rows.

Reference, NOT a merge candidate: contributor PR #86 (branch fm/fm-omp-primary-nextturn-batching) is CONFLICTING, author-marked do-not-merge, and carries ~13 auto-fixed subtle claim-continuity/replay/actor-ack findings that were never really validated. It was read ONLY for the shape of the change and re-implemented cleanly on current origin/main. Nothing was cherry-picked from it and none of its auto-fixes were trusted.

Requirements:

  • Build on current origin/main. Part A (port-3123a-omp-native-steering) is already landed (omp-native delivery is live in bin/fm-send.sh); this is Part B.
  • Deliver the durable watcher wake batch to the OMP primary through the hidden nextTurn + triggerTurn path (not only the status/queue file).
  • Claim persistence: a claimed batch must survive same-session reload AND primary replacement/process replay - never lost, never double-delivered across a primary restart/replacement.
  • Retire claims ONLY after fm-wake-drain acknowledges the durable rows; an interruption before ack must leave the batch durable for idempotent re-handling.
  • This edits firstmate shared tracked material (the OMP primary extension, bin/fm-watch.sh, wake-drain/claim persistence): keep changes shellcheck-clean with colocated tests.

Acceptance criteria:

  • AC1: The watcher delivers its durable wake batch to the OMP primary via the hidden nextTurn + triggerTurn path. Evidence: a test/repro shows an idle OMP primary receiving and processing a watcher wake batch with no manual intervention.
  • AC2: Claim persistence survives same-session reload AND primary replacement/process replay - a claimed-but-unacknowledged batch is re-presented exactly once, never silently dropped, never double-delivered. Evidence: a test simulating reload/replacement asserts exactly-once delivery.
  • AC3: Claims are retired ONLY after fm-wake-drain acknowledges the durable rows; a pre-ack interruption preserves the durable rows for idempotent re-handling. Evidence: a test asserts pre-ack interruption leaves the rows durable.
  • AC4: Concurrency-continuity holds under concurrent watcher appends and primary turns - no lost or duplicated wake. Evidence: a concurrency test or reproduction.
  • AC5: Verified against the idle-secondmate wake reproduction - an idle secondmate now receives the watcher wake. Evidence: the reproduction passes.
  • AC6: The implementation is re-derived on current main; no cherry-pick or unvalidated auto-fix from PR feat(omp): batch primary watcher wake notifications #86 is carried forward. Evidence: diff review plus a short rationale in the PR description.

Deliberate decisions a reviewer reading only the diff would not know:

  1. Why nextTurn at all. Reading OMP's own sendCustomMessage: a steer delivered while a turn is still unwinding lands in agent.steer() plus #scheduleIdleQueueDrain(), whose #canAutoContinueForFollowUp() gate can decline (advisorAutoResumeSuppressed, a blocked drain, a non-conversational transcript tail). The session then settles idle holding a wake it never handled - the reported quiet-secondmate symptom. nextTurn + triggerTurn instead calls #queueHiddenNextTurnMessage, which schedules a post-prompt continuation bound to that prompt generation, and #promptQueuedHiddenNextTurnMessages consumes every wake queued during that turn in one continuation. That batching-into-one-continuation is the "batch" in the task. The hidden mode also keeps the message out of the editable pending-message UI, so the existing draft-preservation guarantee is retained deliberately, not by accident.

  2. Where the claim lives. PR feat(omp): batch primary watcher wake notifications #86 kept a claim inside the extension with a v2..v6 line format plus a separate acknowledgement sidecar file that the extension had to reconcile; most of its ~13 review findings were races in that reconciliation. This re-derivation instead makes bash the single owner: bin/fm-omp-wake-claim-lib.sh owns the format and every mutation, and bin/fm-omp-wake-claim.sh holds the EXISTING durable wake-queue lock around one mutation. Publication and retirement therefore serialize on the one lock the queue already has instead of racing across a second one, and no ack sidecar exists to go stale. The OMP adapter spawns that script rather than linking the lock into the OMP process.

  3. Owner identity is deliberately (per-process boot UUID, per-session hash), not PID. The instance UUID lives on globalThis so a same-process extension RELOAD keeps it and re-presents nothing, while a replacement process never inherits it - which is what makes the distinction survive operating-system PID reuse. This was a named finding on PR feat(omp): batch primary watcher wake notifications #86 and is fixed structurally here.

  4. Retirement is queue-content-based on purpose: a claim is retired only when no queued row remains at or below its cutoff. That makes it actor-agnostic, so a mixed queue where main acknowledges some covered rows and the supervision branch acknowledges the rest keeps the claim until whichever acknowledgement clears the last one. PR feat(omp): batch primary watcher wake notifications #86's actor-scoped retirement was another of its open findings. The cutoff is read from the queue's own sequence counter under the lock and only ever moves forward, so replacing an outstanding claim can never shorten the row span its retirement waits for.

  5. Two mechanisms could otherwise re-present the same wake after a replacement: the shared core's own handoff for a close it never delivered, and this new claim. bin/fm-primary-watch-core.ts therefore exposes hasPendingActionableHandoff(), and the adapter takes the claim over SILENTLY while the core still owes an undelivered close. The handover still happens exactly once; only one of the two speaks.

  6. Ordering and failure policy: the claim is published BEFORE the notification, so an interruption between the two leaves a replayable claim rather than a notification no successor can re-present. publishWakeClaim deliberately never rejects - the durable wake queue remains the authority for the batch, so a claim that cannot be published costs one re-presentation after a replacement, whereas throwing would either cancel the wake or make the core redeliver it (a duplicate). Symmetrically, replay hands the body over first and rebinds LAST, so any failure leaves the claim with its previous owner and the next session event simply retries.

  7. The drain retires the claim on both the ordinary and the acknowledgement paths but never fails the drain over it: a claim that cannot be retired is a stale re-presentation at worst, never a lost wake.

  8. Explicitly out of scope and unchanged: Part A's task-inbox doorbell still uses deliverAs steer with triggerTurn (tests/fm-omp-task-inbox-doorbell.test.sh and the remote-secondmate lifecycle e2e still pin that), and the session-start nudge still uses plain nextTurn. Only the watcher wake moved.

  9. tests/fm-omp-primary.test.sh's whitespace-identity case left set -e on for the rest of the file, so every later failure aborted the suite silently with no "not ok" line - it hid the counterfactual runs used to validate this change. That is restored to the suite's own mode in the same pass, which is why an unrelated-looking one-line test change is present.

  10. tests/fm-omp-primary-live-e2e.test.sh sampled the composer once immediately after send-keys, which raced the TUI repaint and flaked in 2 of 3 runs. It now waits for the exact draft. The assertion is unchanged; only the sampling is bounded.

  11. A live leg firing the wake MID-TURN was attempted and deliberately left out of the opt-in guard: driving a reliably long real turn from the composer was not dependable enough for a guard (two attempts failed at "never started the busy turn" before reaching the assertion). The mid-turn continuation guarantee rests on OMP's own sendCustomMessage contract plus the deterministic delivery-mode assertions in tests/fm-omp-primary.test.sh. This is recorded honestly as a skipped receipt and in docs/verification/supervision.md rather than being claimed as proven.

Verification already performed on this branch: tests/fm-omp-primary.test.sh 15/15 and tests/fm-wake-queue.test.sh 27/27 green; 17 related suites (pi-watch-extension, watch-arm, watch-recovery-loop, watcher-lock, watch-triage, watch-checkpoint, the three wake-drain suites, omp-branch-supervision, omp-fleet-hooks, omp-harness, omp-secondmate, omp-task-inbox-doorbell, session-start, turnend-guard, omp-branch-types) all rc=0; bin/fm-lint.sh and bin/fm-doc-audience-check.sh clean; bin/fm-test-run.sh --check-coverage ok. Four counterfactuals prove the new tests are not vacuous (reverting to steer, retiring without the covered-row guard, dropping the per-process identity, and removing the core interlock each fail the test that guards them). The opt-in live guard passed against real OMP 18.1.5: an idle session with an unsent draft received the wake and ran the handling turn itself with the draft intact.

Firstmate-Validation-Generation: df11b4338aa343a5f86f990552ce247e

What Changed

  • Deliver durable watcher wake batches to the OMP primary through hidden nextTurn/triggerTurn continuation delivery, including handoff coordination with pending core actions.
  • Add durable, queue-locked claim persistence and replay across reloads and process/primary replacement; retire claims only after acknowledged queue rows are drained.
  • Expand watcher/OMP integration tests and verification documentation for continuity, concurrency, replay, and live idle-session behavior.

Risk Assessment

✅ Low: The changed adapter and interlock logic are consistent with the durable-queue notification contract; no concrete source-level correctness or behavioral regression was substantiated.

Testing

The focused OMP-primary and durable wake-queue suites passed, and the opt-in live OMP 18.1.5 lifecycle verified that an idle session automatically handled the watcher wake while preserving its editable draft. The guarded live test also passed its broader lifecycle checks; no worktree artifacts were left behind.

Evidence: Live OMP watcher wake evidence
ok - OMP omp/18.1.5 primary E2E proved an idle session runs the watcher wake turn itself with an intact editable draft

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed (5) ✅
  • 🚨 .omp/extensions/fm-primary-omp.ts:168 - publishWakeClaim swallows every publication failure (including a missing/unexecutable claim script, permission failure, or exhausted queue-lock retry), then sendWake proceeds and eventually marks the core wake delivered. The durable row remains queued, but neither a claim nor core replacement handoff remains, so a primary replacement has no way to re-present it—contradicting AC2's never-dropped replay requirement. The comment says this only costs a replay, but no replayable state exists when publication fails. Please decide whether failure must retain core delivery ownership/retry, or explicitly authorize this loss behavior; the remedy changes failure semantics and needs intent approval.

🔧 Fix: Documented durable fallback and added publication-failure regression test
1 warning still open:

  • ⚠️ tests/fm-omp-primary.test.sh:1561 - The new publication-failure regression checks fm-omp-wake-claim.sh show without setting FM_STATE_OVERRIDE to the fixture state. The adapter itself uses the fixture state, but this assertion reads the command's default state instead, so it can pass even when a claim remains outstanding in $fixture/state; bind the check to FM_STATE_OVERRIDE="$fixture/state" so the test proves the required no-claim outcome.

🔧 Fix: Scoped claim assertion to fixture state
1 error still open:

  • 🚨 .omp/extensions/fm-primary-omp.ts:346 - The claim owner can become inconsistent with the session that receives the notification. sendFollowUp captures wakeClaimSession, awaits publishWakeClaim, then calls sendWakeNotification using the live OMP session. A session_switch can run between those steps: the switch rebinds/replays the claim for the new session, after which the original continuation sends the same wake again (duplicate), or it sends into the new session while the claim remains bound to the old session and that wake is replayed again on the next reload. This is a reachable AC2 violation across concurrent watcher delivery and session replacement; the remedy requires an explicit ownership/serialization decision rather than a local mechanical edit.

🔧 Fix: Synchronized wake claims and covered session-switch ownership
2 issues (1 error, 1 warning) still open:

  • 🚨 bin/fm-omp-wake-claim.sh:116 - The replay protocol rebinds the claim at line 116 after writing the body to stdout, but the adapter does not call sendWakeNotification until after takeOverWakeClaim returns (lines 302-304). If the host send throws or the extension reloads in the same process between those operations, the claim is already owned by the current process/session and will be suppressed on reload even though no notification was delivered, contradicting AC2's never-lost same-session replay guarantee. The stated remedy requires an explicit acknowledgement/rebind protocol, so it needs intent approval.
  • ⚠️ tests/fm-omp-primary.test.sh:1649 - The purported session-switch interleaving regression does not actually attempt a concurrent handler: sendMessage immediately calls clearTimeout(timer), so the only scheduled session_switch at lines 1649-1653 is cancelled before it can run. The test therefore always asserts the non-interleaved path and cannot provide the required real concurrency evidence for AC4 or the claimed race fix. Please authorize a test redesign that invokes the actual session_switch handler from a genuinely concurrent execution context.

🔧 Fix: Durable queue now owns OMP wake replay
1 error still open:

  • 🚨 bin/fm-primary-watch-core.ts:1143 - hasPendingActionableHandoff() treats any existing handoff path as an owned actionable wake (existsSync(actionableHandoff)), even when the file is malformed, empty, or contains only already-delivered entries. activateOwnedWatch() rejects malformed handoffs and surfaces the error, but notifyQueuedWake() has already been suppressed, so durable queue rows can remain indefinitely without any wake notification. Validate the handoff contents (or only report an actually undelivered entry) before suppressing the durable-queue notification.

🔧 Fix: Validated actionable handoffs before suppressing durable wake notifications
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • bash tests/fm-omp-primary.test.sh
  • bash tests/fm-wake-queue.test.sh
  • FM_OMP_PRIMARY_LIVE_E2E=1 bash tests/fm-omp-primary-live-e2e.test.sh
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

dnth added 7 commits September 6, 2026 18:48
…ions

The OMP primary delivered its watcher wake as a custom steer. A steer that
lands while the current turn is unwinding depends on OMP's queued-message
auto-continue gate, which can decline, and the session then settles idle
holding a wake it never handled. Deliver the wake as a hidden nextTurn message
with triggerTurn instead: OMP schedules a continuation bound to that prompt
generation and consumes every wake queued during the turn in it, while the
message still stays out of the editable pending-message UI.

Delivery is not durable on its own, so the adapter now also keeps a claim
naming the exact notification body and the durable wake sequence it covers. A
replacement session or a replacement OMP process re-presents that batch exactly
once; a same-session extension reload re-presents nothing. Retirement is bound
to acknowledgement, not delivery: only bin/fm-wake-drain.sh retires a claim, and
only once no durable row at or below its cutoff is left queued, so an
interruption before acknowledgement leaves the rows and the claim durable for
idempotent re-handling. Because retirement reads the queue rather than an actor,
a mixed queue keeps the claim until whichever acknowledgement clears its last
covered row.

The shared core exposes hasPendingActionableHandoff so the adapter takes the
claim over silently while the core still owes an undelivered close, and exactly
one of the two mechanisms speaks after a replacement.

bin/fm-omp-wake-claim-lib.sh owns the claim format and every mutation;
bin/fm-omp-wake-claim.sh holds the durable wake-queue lock around one of them so
the adapter drives the claim without linking that lock into the OMP process.

Also restores the OMP primary suite's own shell mode after the whitespace-
identity case, which had left errexit on and turned every later failure into a
silent exit 1 with no "not ok" line.

Claude-Session: https://claude.ai/code/session_01HfeoA87NpVLfXdajotU1EB
@dnth
dnth merged commit 2b6b3d8 into main Sep 6, 2026
15 checks passed
@dnth
dnth deleted the fm/port-3123b-omp-watcher-notify branch September 6, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant