Skip to content

fix: verify intended steer delivery - #2260

Open
4mb1t10n wants to merge 17 commits into
kunchenguid:mainfrom
4mb1t10n:fm/fm-1474
Open

fix: verify intended steer delivery#2260
4mb1t10n wants to merge 17 commits into
kunchenguid:mainfrom
4mb1t10n:fm/fm-1474

Conversation

@4mb1t10n

Copy link
Copy Markdown

Intent

Implement #1474 exactly as specified by the full GitHub issue. Make fm-send success mean the intended steer was observably accepted, including parked Codex lanes containing stale composer text, while preserving busy-queue handling and all verified adapter behavior. Exclude generalized activity monitoring. Target main and fully close #1474 with a standalone Closes #1474 PR body line. Follow firstmate-coding-guidelines and add focused stale-composer and successful-delivery regressions. Use no database or browser stack and never run local Cypress. Complete No Mistakes plus exact-head CI and mergeability verification, and never merge the PR.

What Changed

  • Require an affirmatively empty composer before typing, preventing stale parked-lane drafts from being combined with a new steer while preserving verified busy-queue delivery.
  • Serialize sends, interrupts, launches, and supervisor injections by revalidated endpoint identity so composer mutations cannot interleave.
  • Add focused regressions for stale-composer refusal, successful delivery, busy queues, endpoint recovery, and concurrent submissions.

Closes #1474

Risk Assessment

⚠️ Medium: No material source defect was substantiated, but the fix now changes shared submission, key, control, daemon, spawn, and adapter paths, creating a broader regression surface than the original stale-composer fix.

Testing

The public fm-send interface rejected stale Codex composer text without typing, accepted an empty composer exactly once, preserved busy-queue Enter retries, serialized concurrent endpoint delivery, and retained cmux/daemon adapter behavior; reviewer-visible CLI transcripts were captured, while no visual artifact applies to this CLI-only change.

Evidence: fm-send delivery transcript
ok - fm-send strict: exact task/lane ids resolve through home metadata
ok - fm-send --key: exit status follows delivery, and an undelivered key never reports success
ok - fm-send delivery: stale parked-lane composer refuses before typing or submitting
ok - fm-send delivery: an empty composer accepts the intended steer exactly once
ok - fm-send delivery: empty preflight preserves proven busy-queue acceptance
ok - fm-send delivery: send and shared injection submit serialize per resolved endpoint
ok - fm-send strict: unset FM_HOME fails before target resolution
ok - fm-send strict: unresolvable selectors do not fall back to tmux
ok - fm-send strict: prefixless herdr pane ids are rejected before tmux fallback
ok - fm-send strict: unmatched single-colon explicit targets must verify live before sending
ok - fm-send strict: fm-prefixed Herdr sessions remain explicit backend targets
ok - fm-send strict: healthy fm-<id> sends still type once and submit
Evidence: daemon behavioral test transcript
ok - fm-afk-start.sh fails before daemon startup when the afk flag cannot be written
ok - fm-afk-start.sh ignores stale pidfile-only live pids
ok - fm-afk-start.sh reclaims stale daemon locks whose live pid identity no longer matches
ok - supervise daemon state root is scoped by FM_HOME
ok - routine signal self-handles
ok - captain-relevant status verbs escalate
ok - check + unknown escalate; heartbeat self-handles
ok - transient stale self-handles and records a persistence marker
ok - enriched stale wedges bypass status absorption without disturbing busy workers
ok - stale + terminal status escalates immediately
ok - paused reasons with captain phrases remain pause-classified
ok - handle_wake on a paused stale records a pause marker, drops the wedge marker, and does not escalate
ok - handle_wake records a declared pause from a routine signal for long-cadence rechecks
ok - a terminal signal clears pause and stale tracking across both supervisors
ok - housekeeping migrates a normal-watcher's declared pause into daemon tracking
ok - housekeeping clears an already-resumed watcher pause across both supervisors
ok - housekeeping seeds pause tracking from status without a watcher marker
ok - persistent stale escalates after threshold and clears its marker
ok - resumed (busy) stale clears its marker without escalating
ok - housekeeping re-surfaces a stale declared pause on the long cadence and resets its window
ok - housekeeping clears a paused marker whose pane became busy again, without escalating
ok - housekeeping clears a paused marker once the crew is no longer declaring the pause
ok - housekeeping moves an existing stale marker to pause before wedge escalation
ok - housekeeping clears tracking when a crew leaves pause
ok - persistent herdr stale resolves the target from metadata and escalates
ok - herdr idle busy-footer stale clears through capture corroboration
ok - resumed herdr stale clears through backend-aware busy state
ok - persistent Orca stale resolves the terminal from metadata
ok - multiple escalations flush as a single batched digest
ok - batch flush measures max-delay from the first append, not the last
ok - catch-all scan escalates a missed terminal once, not twice
ok - handle_wake routes routine->self and captain->escalate
ok - INJECT_SKIP forces self-handle, bypassing captain-relevant classification
ok - is_wake_reason distinguishes watcher wake reasons from singleton-status stdout
ok - terminal-stale escalate removes its marker so housekeeping does not re-escalate
ok - captain signal escalate marks seen so the catch-all scan does not re-fire
ok - _collapse_newlines replaces newlines with literal separator
ok - afk flag absent: daemon does not inject, buffer preserved
ok - busy-guard defers injection when supervisor pane is busy
ok - marker detection: marker -> stay afk, no marker -> exit afk
ok - /afk invocation is exempt from afk exit (no self-cancel)
ok - should_exit_afk returns false when afk is not active
ok - strip_injection_marker removes the sentinel marker cleanly
ok - pane_input_pending detects partial input on the cursor line
ok - pane_input_pending: a blank unidentified cursor row defers (strict container-proof rule)
ok - pane_input_pending: only proven empty agent prompts pass
ok - fm_tmux_composer_state: a bare shell prompt ($/%/#/>) reads unknown, never empty (dead-shell injection safety)
ok - fm_tmux_composer_state: a bordered composer box and bare agent glyphs (❯/›) still read empty
ok - fm_tmux_composer_state: only matching edge borders form a composer box
ok - pane_input_pending preserves bright placeholder-like drafts in styled captures
ok - classify_signal dedupes against the catch-all scan seen marker
ok - classify_stale dedupes against the signal path seen marker
ok - AFK nonterminal working:+merged keeps wedge aging and re-escalates at bound
ok - genuine done: and merge-check events still escalate
ok - pane_input_pending: an idle bordered composer is NOT pending (afk-invx-i5)
ok - pane_input_pending: text inside a bordered composer is still pending
ok - submit-ACK confirms a submit when the composer returns to a bordered-empty box
ok - submit-ACK reports pending on a persistently swallowed Enter (type-once)
ok - max-defer on an empty stuck pane types once, alarms, and preserves the buffer
ok - max-defer flushes and clears the buffer on an empty bordered pane
ok - max-defer on a pending composer alarms without typing
ok - normal flush clears a stale wedge marker
ok - below MAX_DEFER: no inject, no alarm, buffer preserved
ok - max-defer does not flush or alarm while afk is inactive
ok - library mode: sourcing the daemon defaults FM_WEDGE_ALARM_EXEC to discard (no test can fire a real notification)
ok - wake helpers replace inherited notifier overrides with the safe recorder
ok - the discard seam suppresses every notifier, including command: (fires nothing)
ok - direct notifier helpers honor the discard seam, including command:
ok - osascript channel routes through the notifier seam with the summary (never a real notification)
ok - herdr channel routes through the notifier seam with the summary (never a real notification)
ok - command channel runs the captain command with the summary on $1 and on stdin
ok - command channel failures redact configured commands while logging their exit status
ok - unknown channel directives are redacted while the alarm keeps running
ok - off disables every active alert regardless of directive position (marker and tmux flash are unaffected)
ok - auto resolves to the macOS osascript notifier on Darwin (default-on)
ok - auto on a non-macOS platform selects no built-in OS channel (the marker or a configured command carries it)
ok - config/wedge-alarm selects every configured channel and skips comment and blank lines
ok - a failing channel logs and falls back to the next channel, never crashing the alarm
ok - a hung notifier is bounded, logged, and falls through to the next channel
ok - a backgrounded command notifier remains bounded until its process group is reaped
ok - a hung notifier override is bounded, logged, and proceeds to the next channel
[1]+  Terminated              sh -c 'sleep 30 & printf "%s" "$!" > "$1"; wait' sh "$child_file"
ok - daemon shutdown stops and reaps the active notifier process group
ok - inject_wedge_alarm writes the marker AND emits the active alert even with no tmux status-line (herdr backend)
ok - in-process wedge throttle prevents alert spam when the marker cannot persist
ok - fm-send exits non-zero on a confirmed swallow, zero on a clean submit
ok - fm-send exits non-zero when initial text send fails
ok - fm-send exits non-zero unless delivery is proven empty
ok - discover_supervisor_backend: override > TMUX_PANE > HERDR_ENV+HERDR_PANE_ID > tmux fallback
ok - discover_supervisor_target: override > TMUX_PANE > herdr '<session>:<pane-id>' composition > firstmate:0 fallback
ok - pane_is_busy: herdr native busy_state='busy' short-circuits without a capture fallback
ok - primary busy guard isolates rendered signatures by detected harness
ok - pane_is_busy: omitted backend defaults to tmux for Grok's isolated fallback
ok - pane_input_pending: dispatches through fm_backend_composer_state for backend=herdr
ok - inject_msg: herdr busy-guard defers before ever attempting a submit
ok - inject_msg: herdr composer-guard defers before ever attempting a submit
ok - inject_msg: herdr pane-gone check defers before any busy/composer/submit call
ok - inject_msg: dispatches busy-guard/composer-guard/submit through the herdr backend and succeeds on a confirmed empty composer
ok - inject_msg: defers on a dead-shell/unreadable composer (unknown), never typing the escalation into a shell
ok - inject_msg: unrecognized composer states defer by default

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed (13) ✅
  • 🚨 bin/fm-send.sh:440 - The required guarantee that “fm-send success mean the intended steer was observably accepted” remains racy: two concurrent fm-send processes can both observe empty here, then type before either submits, producing one combined composer payload that can clear afterward and let both callers report success. The gap is larger for marked secondmate sends because pending-reply persistence occurs between this check and typing. Serialize sends per resolved endpoint and perform the empty check inside that critical section immediately adjacent to the shared type/submit boundary.

🔧 Fix: Serialize concurrent text sends per resolved endpoint
1 error still open:

  • 🚨 bin/fm-send.sh:432 - The lock is not actually keyed only by the resolved endpoint: EXPECTED_LABEL differs between valid aliases for the same endpoint. A task selector sets it, while an explicit recorded window/terminal resolving to the same backend and T leaves it empty. Concurrent sends through those two forms therefore acquire different locks, can both observe an empty composer, and can still combine while both report success. Key the lock solely by the canonical backend and resolved target; retain EXPECTED_LABEL only for adapter verification.

🔧 Fix: Canonicalize send locks across endpoint aliases
1 error still open:

  • 🚨 bin/fm-send.sh:436 - Serialization is still scoped to $STATE, not solely to the resolved endpoint. Two supported invocations using different FM_HOME values can target the same physical endpoint through the explicit endpoint escape hatch, acquire different lock files, both observe an empty composer, and combine their text while reporting success. Put the endpoint-keyed lock at the shared composer/type-submit boundary in a lock namespace common to all homes that can address that backend endpoint.

🔧 Fix: Share endpoint send locks across homes
1 error still open:

  • 🚨 bin/fm-send.sh:207 - Serialization can still be bypassed because the supposedly shared namespace varies with caller-controlled TMPDIR or FM_SEND_LOCK_NAMESPACE. Two supported processes targeting the same endpoint with different values acquire different locks, can both observe an empty composer, combine their text, and report success. Use one fixed per-user namespace independent of each invocation’s environment; keep any test isolation seam outside production lock identity.

🔧 Fix: Fix send lock namespace across environments
1 error still open:

  • 🚨 bin/fm-send.sh:471 - The endpoint lock only serializes callers of fm-send.sh; the away supervisor still performs its own empty-composer check and calls fm_backend_send_text_submit without this lock. A daemon injection can therefore race a text send to the supervisor endpoint: both observe empty, both type, and fm-send can report success after the combined composer clears. Move the canonical endpoint lock to the shared composer-check/type-submit boundary and have both paths use it.

🔧 Fix: Share checked submit locking across writers
1 error still open:

  • 🚨 bin/fm-checked-submit-lib.sh:51 - The shared lock can still be bypassed by supported writers that call fm_backend_send_text_submit directly. For example, fm-control exit can type its lifecycle command into the same endpoint after fm-send's empty-composer check but before fm-send types, combining both inputs while fm-send can report success. Route every competing writer through the endpoint serialization boundary, or move locking into the common submit boundary while retaining the checked composer's atomic preflight.

🔧 Fix: Serialize all backend text submit writers
1 error still open:

  • 🚨 bin/fm-checked-submit-lib.sh:66 - The serialization boundary still excludes composer-mutating key sends. A concurrent fm-send --key C-c or fm-control interrupt can issue its required composer-clear key after a text send's empty preflight and typing but before submission; the clear removes the intended steer, then the text sender can observe an empty composer and falsely report success. Serialize composer-mutating key operations on the same canonical endpoint lock, covering the entire interrupt/clear sequence where applicable.

🔧 Fix: Serialize composer-mutating keys with text submits
1 error still open:

  • 🚨 bin/fm-checked-submit-lib.sh:35 - Serialization still keys on the caller's target spelling, not the physical endpoint. For tmux, supported selectors such as session:window and session:window.0 can address the same pane but produce different locks; concurrent text/key operations through those aliases can therefore mutate one composer simultaneously and let fm-send falsely report acceptance. Resolve a stable adapter-owned endpoint identity (for tmux, the verified #{pane_id}) before acquiring the shared lock.

🔧 Fix: Canonicalize locks by physical endpoint identity
1 error still open:

  • 🚨 bin/fm-backend.sh:743 - This contradicts the required criterion to preserve “all verified adapter behavior.” For cmux, a recorded workspace/surface target may become stale and fm_backend_cmux_target_ready(target, expected_label) deliberately refreshes it by task label. This identity check drops the expected label and calls fm_backend_target_exists with only the stale target, so the new lock acquisition fails before the existing refresh path can run; fm-send and control operations now reject supported cmux tasks they previously recovered. Resolve the adapter-owned physical identity using the same expected-label-aware readiness path, then lock the refreshed workspace/surface IDs.

🔧 Fix: Preserve cmux stale-target recovery through locking
1 error still open:

  • 🚨 bin/fm-checked-submit-lib.sh:66 - The endpoint serialization boundary still excludes supported launch/readiness writers. fm-spawn.sh calls adapter-specific send_text_line, send_literal, and send_key functions directly (lines 2063–2093), so an explicit-endpoint fm-send can pass its empty-composer preflight and then race one of these operations; the spawn operation can alter or submit the composer while fm-send still reports success. Route applicable launch/readiness composer transactions through the same physical-endpoint lock, covering each complete multi-operation sequence without weakening the checked-submit preflight.

🔧 Fix: Serialize spawn composer transactions by endpoint
1 error still open:

  • 🚨 bin/fm-checked-submit-lib.sh:35 - The physical identity is resolved before waiting for its lock, but subsequent composer checks and writes still use the original logical target. For tmux, a supported selector such as session:window can be replaced or retargeted from pane A to pane B while this caller waits on pane A's lock; another caller can then lock pane B while this caller also writes to pane B, recreating the composer race and allowing fm-send to report success incorrectly. After acquiring the lock, re-resolve and compare the physical identity (retrying if it changed), or perform the transaction through the canonical adapter-owned physical target.

🔧 Fix: Revalidate endpoint identity after locking
1 error still open:

  • 🚨 bin/fm-checked-submit-lib.sh:59 - The retarget race remains reachable after the new revalidation: once the logical selector resolves to the locked pane, this assigns the original selector—not the verified physical target—to the transaction. If session:window is retargeted immediately afterward, another sender can lock the new pane while this transaction also writes there, allowing composer mutation and false delivery success. Have endpoint resolution return the canonical writable target (for tmux, the verified pane ID) and use that target for the entire locked transaction.

🔧 Fix: Use canonical targets throughout locked transactions
2 errors still open:

  • 🚨 bin/fm-send.sh:139 - The interrupt transaction still switches back to the mutable logical target for its required composer-clear key. If a tmux session:window selector is retargeted after the locked interrupt reaches pane A, this clear is sent to pane B while only pane A's lock is held; pane A retains the restored prompt and pane B's composer can race another writer. Pass the locked canonical target through fm_send_clear_after_interrupt so the interrupt and clear operate on the same endpoint under one lock.
  • 🚨 bin/fm-checked-submit-lib.sh:96 - The canonical cmux target can still migrate inside the locked transaction. After locking workspace/surface A, composer and submit operations receive expected_label; fm_backend_cmux_target_ready may therefore refresh stale A to replacement surface B. A concurrent caller can lock B's identity while this A-locked transaction also reads or writes B, recreating the composer race and false-success path. Add an adapter operation mode that uses the already-verified physical IDs without label-based retargeting for the duration of the lock, or revalidate/retry at the shared boundary before every migration-capable operation.

🔧 Fix: Pin locked operations to canonical endpoints
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • bash tests/fm-send-strict.test.sh
  • bash tests/fm-backend-cmux.test.sh
  • bash tests/fm-daemon.test.sh behavioral cases; stopped when the script entered its embedded shellcheck phase, which this test phase forbids
  • timeout 30 bash tests/fm-trace-context-spawn.test.sh (seven affected spawn/trace cases passed before the imposed timeout)
  • Updated tests/wake-helpers.sh fake tmux #{pane_id} responses from impossible fakepane values to %1, then reran the affected behavior
  • git diff --check
✅ **Document** - passed

✅ No issues found.

🔧 **Lint** - 1 issue found → auto-fixed ✅
  • ⚠️ linter found issues (exit code 1)

🔧 Fix: Fix ShellCheck source and unused-loop warnings
✅ Re-checked - no issues remain.

✅ **Push** - passed

✅ No issues found.

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.

fm-send exits 0 even when the steer never lands (stale composer in a parked lane)

1 participant