feat(omp): batch primary watcher wake notifications - #86
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: f9df832beb
ℹ️ 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".
| const sessionFile = ctx.sessionManager.getSessionFile() || "unknown"; | ||
| notificationSession = createHash("sha256").update(sessionFile).digest("hex"); |
There was a problem hiding this comment.
Captain, when getSessionFile() returns undefined—which its API permits—every fileless conversation hashes the same literal "unknown". If /new or another session replacement occurs before a session file exists, replayWakeNotification() mistakes the replacement for the original session and suppresses replay; the old hidden nextTurn is tied to the abandoned conversation, while newly detected durable rows coalesce behind its pending claim, potentially leaving supervision notifications stranded until another process restart. Use a per-session identity that changes on every replacement when no file path is available.
AGENTS.md reference: AGENTS.md:L225-L225
Useful? React with 👍 / 👎.
1 participant
Intent
Implement Part B of issue kunchenguid#3123: OMP primary watcher wakes must use hidden custom nextTurn delivery with triggerTurn, batching one pending notification without changing Part A task-inbox doorbells. Preserve existing durable wake queue and generation-bound acknowledgement ownership. A same-session extension reload must not duplicate the continuation, while a replacement session or process re-notifies the exact unacknowledged durable batch and never retires its queue rows. Do not merge.
What Changed
nextTurnmessages withtriggerTurn, coalescing pending notifications through the shared watcher core.fm-wake-drain.shacknowledges the durable queue rows, and route supervision-branch fallback wakes through the same primary notification path.Risk Assessment
✅ Low: The change is tightly scoped and its claim, reload, replay, fallback, and mixed-actor acknowledgement paths preserve the required durable-wake invariants on source review.
Testing
Inspected the target change, then exercised the OMP adapter’s end-to-end hidden nextTurn/triggerTurn batching, reload and replacement replay, delivery-failure recovery, and acknowledgement lifecycle; also exercised durable queue generation-bound, actor-scoped acknowledgement/claim retirement. Both focused suites passed, evidence transcripts were captured outside the worktree, and testing left no worktree changes.
Evidence: OMP primary integration transcript
Focused OMP adapter lifecycle transcript: hidden nextTurn delivery, batching, reload/replacement replay, failure recovery, and acknowledgement behavior.Evidence: Durable queue integration transcript
Focused durable wake queue transcript: generation-bound acknowledgements and actor-scoped OMP claim retirement.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (6) ✅
.omp/extensions/fm-primary-omp.ts:398- This deletes the only retained nextTurn payload before binding the replacement session, and the switch path never replays it. If session A has a pending unacknowledged wake then/newor/resumeswitches to B before that continuation begins, B has durable queue rows but receives no exact-batch notification. This contradicts the required “replacement session or process re-notifies the exact unacknowledged durable batch.” Preserve the claim across the switch and replay after binding B..omp/extensions/fm-branch-supervision-omp.ts:866- The branch-failure fallback remains reachable after the primary offers an ordinary wake to the supervision branch: a failed branch sends the main watcher wake as visible-hiddensteer, not hidden customnextTurn. This leaves a concrete OMP primary-wake path contradicting the required “OMP primary watcher wakes must use hidden custom nextTurn delivery with triggerTurn.”🔧 Fix: Preserve OMP wake claims across switches and fallback
3 issues (2 errors, 1 warning) still open:
.omp/extensions/fm-primary-omp.ts:299- The claim deduplicates by notification key, so fallback bypasses the core latch. A main-ownedcheck:wake first queueswake-queue; before its turn begins, an ordinary wake can be accepted by the branch and then fail, emittingbranch-fallback. That overwrites the retained claim and submits a secondnextTurn, violating the required “batching one pending notification” and losing the first payload for later replay..omp/extensions/fm-primary-omp.ts:320- Replay identifies a process only by PID and session hash. A replacement process that reuses the former PID and resumes the same session returns here without re-notifying the unacknowledged batch, contradicting the required “replacement session or process re-notifies the exact unacknowledged durable batch.” Persist and compare a per-process boot nonce..omp/extensions/fm-primary-omp.ts:356- The fallback event is marked accepted before claim publication and notification submission. If either throws, the branch suppresses the error and skips its direct fallback becauseacceptedwas already set, leaving durable rows without a notification until another wake occurs. Accept only after the primary path is safely established, or let submission failure leave the offer unaccepted.🔧 Fix: Harden OMP wake claim ownership and batching
1 error still open:
bin/fm-primary-watch-core.ts:721-notificationTurnStarted()retires the persisted claim atbefore_agent_start, before the agent has runfm-wake-drain.shand its generation-bound acknowledgement. Concrete path: a queued hiddennextTurnstarts, this unlink succeeds, then the agent switches sessions or the process exits before acknowledging; the durable queue rows remain, but the replacement has no claim forreplayWakeNotification()to re-notify. This contradicts the required criterion, “a replacement session or process re-notifies the exact unacknowledged durable batch.” Keep or reconstruct the claim until durable acknowledgement establishes handling.🔧 Fix: Retain OMP wake claims through handling
2 errors still open:
.omp/extensions/fm-primary-omp.ts:377- The claim is only changed toinflight; no durable-acknowledgement path removes or advances it. Afterfm-wake-drain.sh --ack-throughsuccessfully retires all rows,/newor/resumereplays the stale claim atreplayWakeNotification(), creating a continuation for an already-acknowledged batch. This contradicts the required “replacement session or process re-notifies the exact unacknowledged durable batch.” Reconcile the claim at the generation-bound acknowledgement boundary..omp/extensions/fm-primary-omp.ts:469- Everybefore_agent_startclears the pending latch and marks the claim inflight, although it is not scoped to consumption of the custom watchernextTurn. If another agent turn starts while that continuation remains queued, a later wake overwrites the first claim and schedules a second continuation, violating required “batching one pending notification” and losing the first exact replay payload. Correlate this transition with actual watcher-notification consumption.🔧 Fix: Reconcile OMP claims with acknowledged watcher delivery
2 issues (1 error, 1 warning) still open:
bin/fm-wake-drain.sh:43- This contradicts the required “replacement session or process re-notifies the exact unacknowledged durable batch.” If notification A has begun (inflight), a new wake B arrives during that handling turn, the adapter replaces A's claim with pending B and queues one next-turn continuation. The handler can then drain and acknowledge all A+B rows in its exact--ack-throughcommand, but this condition refuses to publish an acknowledgement for pending B. B is therefore replayed after/new,/resume, or process replacement even though its durable rows were acknowledged. Bind claims to the drain presentation/cutoff and reconcile every claim whose represented rows were actually acknowledged..omp/extensions/fm-primary-omp.ts:430- IfqueueWakeNotification()throws while handling a branch fallback (for example, its claim-file publication fails),wake.accept()is never reached and the exception propagates into the branch's catch, which suppresses it. The direct no-handler fallback is skipped, leaving durable rows without a wake until a later event. Catch the primary-handshake failure locally and attempt the existing direct hiddennextTurnfallback before allowing the branch path to finish.🔧 Fix: Bind OMP claim retirement to acknowledged queue cutoffs
3 errors still open:
.omp/extensions/fm-branch-supervision-omp.ts:859- If the primary handshake’ssendWakeNotification()throws,queueWakeNotification()discards its claim before rethrowing; this direct fallback then schedulesnextTurnwithout restoring a claim. A replacement before acknowledgement finds no claim although durable rows remain, contradicting the required “replacement session or process re-notifies the exact unacknowledged durable batch.” Route fallback retry through claim-preserving delivery.bin/fm-wake-drain.sh:40- Claim retirement only publishes acknowledgement for the main actor. With main-ownedcheckseq 1 and branch-ownedsignalseq 2, the claim recordsthrough=2; main acknowledges seq 1, branch later consumes seq 2 but cannot reconcile the claim, and a replacement replays an already-acknowledged batch. This violates “exact unacknowledged durable batch.”.omp/extensions/fm-primary-omp.ts:532- A same-session extension reload leaves the supersededmessage_starthandler registered. It marks a pending claim inflight first; the active handler then returns here without clearing its coalescing latch. After that active core submits one later notification, its latch remains set, so a wake arriving after that turn’s drain cutoff is silently coalesced without any continuation. This breaks the required one-pending batching/continuity behavior on reload.🔧 Fix: Harden OMP wake claim continuity
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
git diff --stat ef8ee4955c343fb46dc7d3ebf474bd252c220ac0 bf823ad9c7a1608cdad2d9d106c3c50ff66ce097and focused test discoverybash tests/fm-omp-primary.test.shbash tests/fm-wake-queue.test.shgit status --shortand focused-path diff check after testing✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.