fix: verify intended steer delivery - #2260
Open
4mb1t10n wants to merge 17 commits into
Open
Conversation
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
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
Closes #1474
Risk Assessment
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
Evidence: daemon behavioral test transcript
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 observeemptyhere, 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_LABELdiffers between valid aliases for the same endpoint. A task selector sets it, while an explicit recorded window/terminal resolving to the same backend andTleaves 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; retainEXPECTED_LABELonly 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 differentFM_HOMEvalues 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-controlledTMPDIRorFM_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 offm-send.sh; the away supervisor still performs its own empty-composer check and callsfm_backend_send_text_submitwithout this lock. A daemon injection can therefore race a text send to the supervisor endpoint: both observe empty, both type, andfm-sendcan 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 callfm_backend_send_text_submitdirectly. For example,fm-control exitcan 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 concurrentfm-send --key C-corfm-controlinterrupt 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 assession:windowandsession:window.0can address the same pane but produce different locks; concurrent text/key operations through those aliases can therefore mutate one composer simultaneously and letfm-sendfalsely 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 andfm_backend_cmux_target_ready(target, expected_label)deliberately refreshes it by task label. This identity check drops the expected label and callsfm_backend_target_existswith only the stale target, so the new lock acquisition fails before the existing refresh path can run;fm-sendand 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.shcalls adapter-specificsend_text_line,send_literal, andsend_keyfunctions directly (lines 2063–2093), so an explicit-endpointfm-sendcan pass its empty-composer preflight and then race one of these operations; the spawn operation can alter or submit the composer whilefm-sendstill 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 assession:windowcan 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 allowingfm-sendto 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. Ifsession:windowis 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 tmuxsession:windowselector 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 throughfm_send_clear_after_interruptso 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 receiveexpected_label;fm_backend_cmux_target_readymay 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.shbash tests/fm-backend-cmux.test.shbash tests/fm-daemon.test.shbehavioral cases; stopped when the script entered its embedded shellcheck phase, which this test phase forbidstimeout 30 bash tests/fm-trace-context-spawn.test.sh(seven affected spawn/trace cases passed before the imposed timeout)Updatedtests/wake-helpers.shfake tmux#{pane_id}responses from impossiblefakepanevalues to%1, then reran the affected behaviorgit diff --check✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: Fix ShellCheck source and unused-loop warnings
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.