fix(omp): use native worker steering for task messages - #98
Merged
Merged
Conversation
Ordinary text sent to an OMP task used to fall back to typing the doorbell into the terminal and pressing Enter when the task-bound native adapter was unavailable. On an already-streaming OMP target the backend can only report `queued-unconfirmed` there: Enter transported, but no native session event proved the worker received anything. fm-send accepted that and exited 0, so a steer could sit unreceived behind a success report. OMP steering now uses the task-bound native receive adapter alone. Nothing is typed, no Enter is pressed, and fm-send reports exactly one bounded outcome, each naming the bound task, endpoint, session process, native queue entry and durable record: omp-native-received the session acknowledged the request (exit 0) omp-native-refused the adapter was unavailable or refused (exit 6) omp-native-queued the named request is queued without a receipt (exit 7) Both failures are durable and say not to resend; the watcher's re-ring ladder owns redelivery. On the typed plane a busy-OMP `queued-unconfirmed` is no longer a zero-exit success either: it falls into the existing `delivered-no-turn` verdict (exit 4) which queues supervised recovery. `/exit` stays an independent typed operation. Backends, non-OMP harnesses, remote, secondmate and --resolve-key routes are unchanged. FM_TASK_INBOX_OMP_REQUIRE_PROGRAMMATIC is gone: it is now the only OMP behavior. Tests drive the public fm-send over tmux and Herdr against a real task-bound extension process and cover the received, refused, unproven-binding and unacknowledged outcomes plus the absence of any composer transport. Claude-Session: https://claude.ai/code/session_013JMBTfuZSx3n2zoDXYUTuV
…ource The turn-start fixture reran on today's source, which changed the busy-OMP outcome it proves. Rebind its revision and patch hash to the manifest it actually covers now. Claude-Session: https://claude.ai/code/session_013JMBTfuZSx3n2zoDXYUTuV
bin/fm-lint.sh flagged both new outcome variables as unused. They are the library's published bindings, read by bin/fm-send.sh after sourcing, so state that where each is assigned. Claude-Session: https://claude.ai/code/session_013JMBTfuZSx3n2zoDXYUTuV
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 issue kunchenguid#3123 Part A ("explicit Firstmate-to-worker OMP steering") into this fork (dnth/firstmate). This is firstmate's own shared, tracked supervision plumbing, so the firstmate-coding-guidelines skill (one-owner rule, one sentence per line in tracked Markdown, plain dash never an em dash, shellcheck-clean bin scripts via bin/fm-lint.sh, colocated tests in tests/ extending an existing runner, no agent commit co-author, behavior-only tests) governs the change.
The defect, reproduced before fixing it: when a supervisor sends ordinary text with fm-send to an OMP task, the task-bound path typed the text into the terminal composer and pressed Enter. On an active OMP target the backend can only return
queued-unconfirmedthere - Enter transported while the pane was working, but no native session event proved the worker received or acted on the text - and fm-send accepted that verdict and exited 0. That false success is what let steers pile silently on a mate; a later /exit would get appended. Reproduced in this worktree: fm-send to an OMP task with no live receive adapter exited 0 aftersend-keys -t sess:fm-t1 -l <doorbell line>plussend-keys -t sess:fm-t1 Enter.Required change (authoritative source: kunchenguid#3123 Part A acceptance criteria): route ordinary text for an OMP task selector through an exact task-bound native OMP worker-receive adapter - no terminal text injection, no Enter - and report exactly one bounded, inspectable outcome (native ack, a named durable native queue identity, or an explicit refusal). Remove the generic zero-exit
queued-unconfirmedpublic outcome.Accepted acceptance criteria:
AC1 Ordinary text for an OMP task selector is delivered through an exact task-bound native OMP worker-receive adapter with no terminal/composer text injection and no Enter keypress on the OMP active-worker steer path.
AC2 fm-send reports exactly one of three bounded outcomes - native receive acknowledgement, a named durable native queue identity, or an explicit refusal with a reason - and the generic zero-exit
queued-unconfirmedpublic outcome is removed.AC3 Delivery binds and reports the exact task/session and exact message, proven by OMP session API/event identity rather than rendered composer text; a binding or receipt failure exits nonzero and is not resend-inviting.
AC4 /exit remains an independent operation and is never appended as a workaround for an unresolved steer.
AC5 OMP steering is covered over both tmux and Herdr, while non-OMP, remote, secondmate, lifecycle-key (--resolve-key), and harness-native routes are preserved unchanged.
AC6 A steer to a crewless idle / non-streaming OMP mate also yields exactly one bounded inspectable outcome - never a silent zero-exit false-success. Actually waking the idle mate to drain is Part B and out of scope; this AC only requires the delivery outcome be honest and inspectable.
Explicit scope exclusions, preserved unchanged on purpose: Part B (watcher->primary wake notifications, a separate sequenced follow-up) is untouched, including .omp/extensions/fm-primary-omp.ts and its
deliverAs: "steer", triggerTurn: truefollow-up mode; non-OMP harness routes keep the existing advisory composer pre-check and backend submit fallback; remote and secondmate routes keep their existing exit codes and messages; /exit stays an independent typed operation; zellij, Orca and cmux stay unsupported for OMP (fm-spawn.sh already refuses OMP outside tmux and herdr). Upstream kunchenguid#3123 is an OPEN issue with no upstream PR, so this was implemented from the acceptance criteria rather than by porting a merged change. This work was deliberately not run on OMP and adds no .omp/extensions behavior in the worktree.Decisions and tradeoffs made while doing the work, which a reviewer reading only the diff would not know:
The fork already had the native transport: bin/fm-task-inbox-lib.sh's fm_task_inbox_ring attempted an acknowledged programmatic request through fm_backend_omp_trigger_turn (SIGUSR2 to the task-bound OMP process, a PID-bound
<task>.omp-doorbell-readymarker plus a.requestsdirectory the extension drains and acknowledges). It was only preferred, gated behind FM_TASK_INBOX_OMP_REQUIRE_PROGRAMMATIC=1 which only bin/fm-remote-secondmate-control.sh set. So the fix is to make that native path OMP's ONLY transport rather than to build a new adapter, and the env flag is deleted because it is now the sole OMP behavior. This deliberately keeps one owner of the transport instead of adding a parallel one.fm-send's ordinary-text plane for a task selector already writes the durable sequenced state/.inbox/NNN.msg record before ringing, so the "named durable native queue identity" of AC2/AC3 is the existing pair of the inbox record and the native request entry
<marker>.requests/request.<NNN.msg>. Nothing new was invented to carry identity, and the request base name (without its .pending/.delivered/.ambiguous state suffix) is what gets reported because that is the stable identity.Outcome vocabulary and exit codes: I deliberately reused the existing exit codes 6 and 7 rather than minting new ones, because bin/fm-send.sh's remote branch maps a remote fm-send's numeric exit 6/7/8/9 into its own remote verdicts. Renaming the child's stderr text is safe; changing the numbers would have silently broken the remote secondmate route that AC5 requires preserved. The three outcomes are
omp-native-received(exit 0, printed on stdout),omp-native-refused(exit 6),omp-native-queued(exit 7), each carrying the identical binding stringtask=<id> endpoint=<backend>:<target> session-pid=<bound OMP process> request=<native queue entry> record=<durable record> message-bytes=<n>. The two remote-only follow-up gates (turn-start and handled-ack, exit 8) adopted the same vocabulary and binding.Typed plane: a busy-OMP
queued-unconfirmedis no longer a zero-exit success either. Rather than mint a fourth outcome I mapped it into the pre-existingdelivered-no-turnverdict (exit 4), which already means "submitted, no proof a turn started, do not resend, supervised recovery required" and already persists a status event plus a durable watcher wake. It gets its own message naming the busy-with-no-native-event cause. This is why tests/fm-send-turn-start.test.sh's former "already-busy OMP queued-Enter exception should remain accepted / expected exit 0" assertion now requires exit 4: that misleading contract was named in the task as one that must change.Backend verdicts were deliberately NOT changed. bin/fm-tmux-lib.sh and bin/backends/herdr.sh still emit
queued-unconfirmed, because it carries real information (Enter transported, target busy, no native proof) and because tests/fm-tmux-submit-busy.test.sh, tests/fm-backend-herdr.test.sh and the away-mode daemon's shared submit core depend on it. Only fm-send's public acceptance of it was removed. AC2 is about fm-send's public outcome, not the backend's internal verdict.Consequence I accepted on purpose: a local OMP steer now fails loudly (exit 6) when the task-bound extension is not live, where it previously fell back to typing and exited 0. Callers such as bin/fm-bootstrap.sh's secondmate nudge and bin/fm-pending-reply-lib.sh's recovery resend will now see that nonzero result. That is the point of AC6 - the durable record still exists and the watcher's re-ring ladder still owns redelivery, so nothing is lost, only the false success. bin/fm-watch.sh's re-ring path needed no change: it already logs any ring result and its escalation ladder still surfaces an unhandled record as a stale wake.
Test placement follows the colocated-test rule: the new cases extend tests/fm-omp-task-inbox-doorbell.test.sh, the existing owner of OMP doorbell routing and extension behavior, rather than adding a new runner. They drive the public bin/fm-send.sh executable over BOTH tmux and Herdr against a real task-bound Node extension process (the actual .omp/extensions/lib/fm-task-inbox-doorbell.ts helper, with a fake sendMessage recorder), asserting observable behavior: the reported binding fields, an empty terminal transport log, the byte-exact durable record, the recorded native session event with triggerTurn:true, and nonzero exits for a refused adapter, an unproven session binding, and an unacknowledged request. No test asserts implementation source bytes.
Three existing tests were rewritten because they pinned the old contract, not because they broke by accident: tests/fm-send-turn-start.test.sh (busy queued path now exits 4), tests/fm-send-resolve-key.test.sh (a busy OMP send with no native receipt still leaves its decision open, now via exit 4), and tests/fm-send-secondmate-marker.test.sh (its OMP secondmate case now asserts the bounded refusal keeps the marked record and leaves the pending-reply expectation undelivered rather than confirming delivery through a composer fallback that no longer exists; its bun-dependent busy-composer fixture became unnecessary and was dropped).
Documentation was patched at its existing owners rather than duplicated: bin/fm-send.sh's and bin/fm-task-inbox-lib.sh's headers own the mechanics, docs/architecture.md owns the mechanism boundary, docs/tmux-backend.md and docs/herdr-backend.md own the backend verdicts, and .agents/skills/harness-adapters/SKILL.md got a one-line cross-reference because it is the skill loaded before steering. AGENTS.md was deliberately left alone under the size-discipline rule: the new facts are situational mechanics owned by the script headers, and its existing steering sentence stays accurate. docs/verification/runtime-backends.md's two affected records were refreshed with today's date, the current observed output, and a rebound revision plus patch hash over the same file manifest, so no stale guarantee is left asserting the removed behavior.
Validation already run locally before this pipeline: bin/fm-lint.sh rc=0; bin/fm-doc-audience-check.sh ok surfaces=71 local_links=280; bin/fm-test-run.sh --changed --base origin/main ran all 91 selected suites. Seven failed inside this treehouse worktree and every one is an environment artifact rather than a regression - fm-kimi-harness (needs python3 with tomllib), fm-pi-primary-types and fm-on fail identically on untouched main here; fm-backend-herdr fails identically on untouched main here because the live Herdr session leaks HERDR_ENV/HERDR_PANE_ID into a fixture that assumes neither, and passes 188 ok from a clean clone of this branch; fm-secondmate-harness (53 ok), fm-remote-secondmate-lifecycle-e2e (25 ok) and fm-spawn-batch (5 ok) all pass rc=0 from that same clean clone.
Firstmate-Validation-Generation: 17d3ad616c36bdbe48e9670b457ea90c
What Changed
queued-unconfirmedacceptance for OMP paths while preserving existing backend, remote, secondmate, lifecycle-key, and non-OMP routing behavior.Risk Assessment
Testing
Ran the dedicated OMP doorbell suite plus all three rewritten fm-send contract suites. Behavioral coverage exercised native tmux and Herdr delivery, exact task/session/message binding, single bounded outcomes, refusal and unacknowledged paths, no composer or Enter transport, handled/reconciliation behavior, turn-start semantics, resolve-key preservation, and secondmate marker behavior. All targeted commands exited successfully; no transient files were left in the worktree.
Evidence: OMP doorbell behavioral evidence
Evidence: fm-send turn-start evidence
Evidence: fm-send resolve-key evidence
Evidence: fm-send secondmate marker evidence
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (5) ✅
bin/fm-send.sh:950- AC2 requires exactly one bounded public outcome, but the native inbox path printsomp-native-receivedat [bin/fm-send.sh:950-953] before the optional turn-start/handled-ack gates, then prints a secondomp-native-queuedoutcome at [bin/fm-send.sh:966-974] and exits 8 when either gate fails (the remote OMP path enables both gates). A caller can therefore observe two contradictory outcomes for one steer; the output contract needs an explicit decision on which single final outcome to emit.bin/fm-task-inbox-lib.sh:224- The reported session PID is read intoFM_TASK_INBOX_RING_OMP_PIDbefore the backend performs its own marker read and foreground-PID validation at [bin/fm-task-inbox-lib.sh:224-229]. If the OMP process restarts and rewrites the ready marker between those operations, the request can be delivered successfully to the new PID while fm-send reports the stale old PID, violating AC3's exact session binding. Export the PID from the successful backend/request operation (or re-read it after the operation) so the reported binding is the one actually signaled.🔧 Fix: Fix OMP outcome ordering and exact session PID binding
1 error still open:
bin/fm-send.sh:938- The handled-record fast path can violate AC2's “exactly one bounded outcome” contract. A reconciliation send whose delivery id already exists under<task>.inbox/handled/makesINBOX_RECORDpoint to that handled file, then [bin/fm-send.sh:938] skipsfm_task_inbox_ring; because no native request identity is populated, [bin/fm-send.sh:972-974] emits noomp-native-*outcome yet the command exits 0. This leaves an OMP selector steer with neither acknowledgement, queue identity, nor explicit refusal. Please decide the intended bounded outcome for idempotent already-handled reconciliation (the remedy changes that deliberate replay behavior).🔧 Fix: Report handled OMP replays as native acknowledgements
1 warning still open:
bin/fm-task-inbox-lib.sh:238- When the native request already exists asrequest.<id>.pending.delivered,fm_backend_omp_trigger_turnreturns success before reading the readiness marker.fm_task_inbox_ringthen copies the emptyFM_OMP_TASK_DOORBELL_PID, so fm-send emitsomp-native-receivedwithsession-pid=unreadableeven though this is a successful native acknowledgement. A replay of an unhandled durable record can reach this path, violating AC3's exact task/session binding. Preserve or re-read the marker PID for this already-delivered fast path, and refuse nonzero if the exact binding cannot be established.🔧 Fix: Bind already-delivered OMP acknowledgements to session PID
1 warning still open:
bin/fm-backend.sh:752- A replay of an already-delivered native request can report the current readiness-marker PID as the acknowledging session even when the request was acknowledged by a previous process: the fast path removesrequest.<id>.pending.delivered, reads whatever PID is currently in the marker, and returns success without validating the target or retaining the PID that produced the acknowledgement. If the OMP worker restarts between acknowledgement and reconciliation,omp-native-receivedcan therefore name a new session that never received this message, violating AC3's exact task/session binding. Making this durable requires retaining the acknowledging PID (or refusing when historical binding is unavailable), which extends the current state contract and needs authorization.🔧 Fix: Enforce proven OMP session bindings or honest refusal
1 error still open:
bin/fm-send.sh:951- The handled OMP reconciliation path still reportssession-pid=unreadablethrough the shared binding at [bin/fm-send.sh:951], and its test explicitly requires that value at [tests/fm-omp-task-inbox-doorbell.test.sh:622]. The authoritative decision for this path says it must not name a session and must report the session field as explicitly not-applicable/not-a-session-receipt (request=noneremains required);unreadableis an unknown-session claim and contradicts that requirement. Please decide the intended binding token and update the source/test together.🔧 Fix: Mark handled OMP replays without naming sessions
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-omp-task-inbox-doorbell.test.shbash tests/fm-send-turn-start.test.shbash tests/fm-send-resolve-key.test.shbash tests/fm-send-secondmate-marker.test.sh✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.