fix(omp): persist and deliver watcher wake batches - #119
Merged
Conversation
…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
…n-failure regression test
… durable wake notifications
…and claim terminology
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 afterfm-wake-drainacknowledges 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:
Acceptance criteria:
Deliberate decisions a reviewer reading only the diff would not know:
Why nextTurn at all. Reading OMP's own sendCustomMessage: a
steerdelivered 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+triggerTurninstead 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.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.
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.
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.
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.
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,
replayhands the body over first and rebinds LAST, so any failure leaves the claim with its previous owner and the next session event simply retries.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.
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.
tests/fm-omp-primary.test.sh's whitespace-identity case left
set -eon 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.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.
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
nextTurn/triggerTurncontinuation delivery, including handoff coordination with pending core actions.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
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-publishWakeClaimswallows every publication failure (including a missing/unexecutable claim script, permission failure, or exhausted queue-lock retry), thensendWakeproceeds 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 checksfm-omp-wake-claim.sh showwithout settingFM_STATE_OVERRIDEto 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 toFM_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.sendFollowUpcaptureswakeClaimSession, awaitspublishWakeClaim, then callssendWakeNotificationusing the live OMP session. Asession_switchcan 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 callsendWakeNotificationuntil aftertakeOverWakeClaimreturns (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:sendMessageimmediately callsclearTimeout(timer), so the only scheduledsession_switchat 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, butnotifyQueuedWake()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.shbash tests/fm-wake-queue.test.shFM_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.