Repository navigation
fix: batch supervisor wake echoes and drop redundant notifications - #292
Merged
Merged
Conversation
Supervisor turns were dominated by notification echoes: the watcher woke firstmate once per turn boundary for state it had already delivered, and climbed the wedge ladder for lanes parked at a validation gate nobody had answered yet. This lands both upstream ports and our own batching: - Port kunchenguid#2877 (a56a78a): an opt-in config/turnend-churn-absorb lets a bare turn-end use bounded pane churn since the previous poll as a third form of positive work evidence, with a per-window .churn-since-* bound. Adapted to this fork's per-generation .turn-ended.<spawn_gen> markers so the proof reaches production writers, and to its existing marker/window helpers. - Port kunchenguid#4974 (9aabe3b): an opt-in config/wedge-defer-parked-gate turns a wedge escalation into the bounded recheck cadence for a lane whose own no-mistakes gate awaits the supervisor's still-open decision for that run. The gate's human-owed verdict is now minted from the findings table's action column by position (never a payload-wide substring), and the deferral needs both that verdict and an open nm-<run>-<step> decision. Adapted to this fork's wait-record and away-mode ownership (the daemon owns afk triage). - Add deterministic echo batching: status-only and bare turn-ended signal wakes are deferred inside a 60-300s window (default 120) and delivered as one wake. check/stale/heartbeat/needs-decision/blocked/done/failed and decision-owned rows stay immediate and flush a pending batch; the batch persists in state/.echo-batch across restarts. Two drop rules - a wholly turn-ended batch whose gates already have a recorded decision, or whose crew-state lines are byte-identical to the last delivered wake with an active blocking pane and no steer since - remove echoes that carry nothing. Every deferral, flush, and drop is logged with its rule name. No model or network call runs in the wake path. Tests: tests/fm-watch-triage.test.sh (churn absorb, batching, immediate classes, drop near misses, parked-gate ladder) and tests/fm-crew-state.test.sh (action-column derivation counterexamples). (cherry picked from commit a56a78a) (cherry picked from commit 9aabe3b)
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.
Summary
Supervisor notification path after this change:
Upstream attribution:
Evidence
After: three echoes in one window produce exactly one delivered wake with three durable rows; a restart inherits and flushes the same batch.
ask-usersubstring minted the human-owed verdict from free text.After: the gate's
actioncolumn is read by position, the verdict is bound to its currentnm-<run>-<step>, and only an open decision for that run takes the bounded recheck. Crewmate-owned, answered, reopened, wrong-run, and unrelated-key gates keep the ladder.tests/fm-watch-triage.test.sh117 ok / 0 not ok, including batching, restart persistence, immediate classes, both drop rules with near misses, and the parked-gate ladder;tests/fm-crew-state.test.shcovers the real producer and the action-column counterexamples.fm_watch_arm_ompprimary with two spawned OMP workers deferred two real turn-end echoes and delivered them in one wake (entries=2, one primary watcher-wake message); a Discord-shaped ext-request was delivered in 0s while the batch was pending (window 120s).Merge Danger
Door: two-way
Blast Radius: supervision-only
Watcher-only behavior; no project, model, or network path changes. Both upstream ports remain opt-in (
config/turnend-churn-absorb,config/wedge-defer-parked-gate); batching is default-on at 120s and can be disabled withconfig/wake-echo-batch=off. Drops are limited to wholly turn-ended batches with positive evidence and are logged with their rule name.Pipeline
Updates from git push no-mistakes
✅ **Intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 6 issues found → auto-fixed (5) ✅
bin/fm-watch.sh:2338- R2 does not bind answer evidence to the current gate. After run A's review is answered, a turn-end at run B's unanswered gate can reuse A's latest status key and historical resolution, causing the batch to be dropped. Reopening the same key also retains the earlier resolution as proof. Validate the current run/step and latest decision occurrence before dropping. Related sites: bin/fm-watch.sh:2262 selects an unbound historical key; :2287 accepts any historical resolution; :2291-2293 accepts substring matches in handled messages; :2336-2340 combines these proofs.bin/fm-watch.sh:2372- R3 misses steers sent within the delivery's second. A delivery at 1000.1 followed by fm-send at 1000.5 gives both records epoch 1000; the strict greater-than comparison allows an unchanged blocked echo to be dropped despite that steer. Treat equal timestamps as an ambiguous near miss and deliver. Related sites: bin/fm-watch.sh:2233 and :2446 create second-resolution baselines; :2369-2372 checks pending and handled inbox records.bin/fm-watch.sh:2412- A persisted ordinary-mode batch can survive into away mode, then be dropped by R2/R3 before the daemon receives it. New signals correctly bypass batching under .afk, but inherited batches do not. Apply the away-mode ownership guard at the shared flush boundary and promptly deliver inherited batches without watcher-side drops. Related sites: bin/fm-watch.sh:2199-2201 retains the normal deadline; :2412-2427 applies both drops; :3015 flushes before away-mode classification; :3294-3298 guards only new signals.bin/fm-crew-state.sh:455- The findings parser still mistakes commas inside file paths for column separators. With the normal header, an auto-fix finding for "src/a,b,ask-user,c.ts" places ask-user at the raw fifth comma field and incorrectly mints the human-decision marker. Parse quoted fields correctly or reject ambiguous rows. Related sites: bin/fm-crew-state.sh:442 incorrectly treats file values as comma-free; :459-460 accepts the shifted field; :677 emits the incorrect marker consumed by bin/fm-classify-lib.sh:2171.bin/fm-watch.sh:2091- Numeric configuration is validated as decimal but retained for shell arithmetic: 060 passes the range check and becomes a 48-second window, while 090 causes an invalid-octal arithmetic error. Oversized digit strings can also survive failed range comparisons. Normalize bounded decimal input or reject noncanonical values in the resolver. Related sites: bin/fm-watch.sh:2087 validates; :2091 retains the raw value; :2106 checks enablement; :2196 performs arithmetic.bin/fm-watch.sh:2078- Simplification: the additional OFF/false/FALSE/no/NO disable aliases are unnecessary for the requested configurable batching window and exceed the documented off spelling. Remove these aliases and retain the narrower documented configuration surface unless broader spellings are explicitly desired.🔧 Fix applied.
10 issues (5 errors, 5 warnings) still open:
bin/fm-watch.sh:2338- R2 does not bind answer evidence to the current gate. After run A's review is answered, a turn-end at run B's unanswered gate can reuse A's latest status key and historical resolution, causing the batch to be dropped. Reopening the same key also retains the earlier resolution as proof. Validate the current run/step and latest decision occurrence before dropping. Related sites: bin/fm-watch.sh:2262 selects an unbound historical key; :2287 accepts any historical resolution; :2291-2293 accepts substring matches in handled messages; :2336-2340 combines these proofs.bin/fm-watch.sh:2372- R3 misses steers sent within the delivery's second. A delivery at 1000.1 followed by fm-send at 1000.5 gives both records epoch 1000; the strict greater-than comparison allows an unchanged blocked echo to be dropped despite that steer. Treat equal timestamps as an ambiguous near miss and deliver. Related sites: bin/fm-watch.sh:2233 and :2446 create second-resolution baselines; :2369-2372 checks pending and handled inbox records.bin/fm-watch.sh:2412- A persisted ordinary-mode batch can survive into away mode, then be dropped by R2/R3 before the daemon receives it. New signals correctly bypass batching under .afk, but inherited batches do not. Apply the away-mode ownership guard at the shared flush boundary and promptly deliver inherited batches without watcher-side drops. Related sites: bin/fm-watch.sh:2199-2201 retains the normal deadline; :2412-2427 applies both drops; :3015 flushes before away-mode classification; :3294-3298 guards only new signals.bin/fm-crew-state.sh:455- The findings parser still mistakes commas inside file paths for column separators. With the normal header, an auto-fix finding for "src/a,b,ask-user,c.ts" places ask-user at the raw fifth comma field and incorrectly mints the human-decision marker. Parse quoted fields correctly or reject ambiguous rows. Related sites: bin/fm-crew-state.sh:442 incorrectly treats file values as comma-free; :459-460 accepts the shifted field; :677 emits the incorrect marker consumed by bin/fm-classify-lib.sh:2171.bin/fm-watch.sh:2091- Numeric configuration is validated as decimal but retained for shell arithmetic: 060 passes the range check and becomes a 48-second window, while 090 causes an invalid-octal arithmetic error. Oversized digit strings can also survive failed range comparisons. Normalize bounded decimal input or reject noncanonical values in the resolver. Related sites: bin/fm-watch.sh:2087 validates; :2091 retains the raw value; :2106 checks enablement; :2196 performs arithmetic.bin/fm-watch.sh:2078- Simplification: the additional OFF/false/FALSE/no/NO disable aliases are unnecessary for the requested configurable batching window and exceed the documented off spelling. Remove these aliases and retain the narrower documented configuration surface unless broader spellings are explicitly desired.bin/fm-watch.sh:2429- A pending echo batch deadlocks process-event delivery. procevent_surface_queued holds FM_WAKE_QUEUE_LOCK until its post-output callback, but the new pre-output hook calls echo_batch_flush, which calls fm_wake_append and waits indefinitely for that same non-reentrant lock. Neither the immediate check nor subsequent wakes can print. Flush before acquiring the process-event lock, or append under its existing ownership. Related sites: bin/fm-push-transition-lib.sh:102 invokes the hook; bin/fm-watch.sh:2461 flushes; :2537 acquires the lock; :2549 installs its deferred release.bin/fm-crew-state.sh:677- The required parked-gate deferral and answered-gate drop are unreachable with the real producer. This hunk adds only the human-decision marker; fm-crew-state never emits a 'run: <id>' component. Both bin/fm-classify-lib.sh:2167-2169 and bin/fm-watch.sh:2320-2324 require it and therefore always reject real parked gates. This omits the required 'defer the wedge escalation ... supervisor-owed gate' behavior and R2's 'drop the echo' behavior. Round 1's R1 fix moved the drop onto this nonexistent field and left the producer and original deferral sibling inconsistent. Supply the attributed run identity through the shared producer boundary for both consumers; the watcher tests currently manufacture that component themselves.bin/fm-watch.sh:2068- Simplification: FM_ECHO_BATCH_SECS introduces a second configuration surface and precedence rule beyond the requested config-file window ('Configuration lives in config/'). Remove the environment override and use config/wake-echo-batch, including in test fixtures. Related definitions: docs/configuration.md:197 and :1400.bin/fm-watch.sh:645- Simplification: remove the unused signal_statuses array and status-specific bookkeeping (:623, :645-647), plus the uncalled echo_batch_window wrapper (:2100-2103). No intent requirement needs either component: status files already return at :630, and all window consumers call echo_batch_resolve directly.🔧 Fix applied.
9 issues (5 errors, 4 warnings) still open:
bin/fm-watch.sh:2338- R2 does not bind answer evidence to the current gate. After run A's review is answered, a turn-end at run B's unanswered gate can reuse A's latest status key and historical resolution, causing the batch to be dropped. Reopening the same key also retains the earlier resolution as proof. Validate the current run/step and latest decision occurrence before dropping. Related sites: bin/fm-watch.sh:2262 selects an unbound historical key; :2287 accepts any historical resolution; :2291-2293 accepts substring matches in handled messages; :2336-2340 combines these proofs.bin/fm-watch.sh:2372- R3 misses steers sent within the delivery's second. A delivery at 1000.1 followed by fm-send at 1000.5 gives both records epoch 1000; the strict greater-than comparison allows an unchanged blocked echo to be dropped despite that steer. Treat equal timestamps as an ambiguous near miss and deliver. Related sites: bin/fm-watch.sh:2233 and :2446 create second-resolution baselines; :2369-2372 checks pending and handled inbox records.bin/fm-watch.sh:2412- A persisted ordinary-mode batch can survive into away mode, then be dropped by R2/R3 before the daemon receives it. New signals correctly bypass batching under .afk, but inherited batches do not. Apply the away-mode ownership guard at the shared flush boundary and promptly deliver inherited batches without watcher-side drops. Related sites: bin/fm-watch.sh:2199-2201 retains the normal deadline; :2412-2427 applies both drops; :3015 flushes before away-mode classification; :3294-3298 guards only new signals.bin/fm-watch.sh:2091- Numeric configuration is validated as decimal but retained for shell arithmetic: 060 passes the range check and becomes a 48-second window, while 090 causes an invalid-octal arithmetic error. Oversized digit strings can also survive failed range comparisons. Normalize bounded decimal input or reject noncanonical values in the resolver. Related sites: bin/fm-watch.sh:2087 validates; :2091 retains the raw value; :2106 checks enablement; :2196 performs arithmetic.bin/fm-watch.sh:2078- Simplification: the additional OFF/false/FALSE/no/NO disable aliases are unnecessary for the requested configurable batching window and exceed the documented off spelling. Remove these aliases and retain the narrower documented configuration surface unless broader spellings are explicitly desired.bin/fm-watch.sh:2429- A pending echo batch deadlocks process-event delivery. procevent_surface_queued holds FM_WAKE_QUEUE_LOCK until its post-output callback, but the new pre-output hook calls echo_batch_flush, which calls fm_wake_append and waits indefinitely for that same non-reentrant lock. Neither the immediate check nor subsequent wakes can print. Flush before acquiring the process-event lock, or append under its existing ownership. Related sites: bin/fm-push-transition-lib.sh:102 invokes the hook; bin/fm-watch.sh:2461 flushes; :2537 acquires the lock; :2549 installs its deferred release.bin/fm-watch.sh:2068- Simplification: FM_ECHO_BATCH_SECS introduces a second configuration surface and precedence rule beyond the requested config-file window ('Configuration lives in config/'). Remove the environment override and use config/wake-echo-batch, including in test fixtures. Related definitions: docs/configuration.md:197 and :1400.bin/fm-watch.sh:645- Simplification: remove the unused signal_statuses array and status-specific bookkeeping (:623, :645-647), plus the uncalled echo_batch_window wrapper (:2100-2103). No intent requirement needs either component: status files already return at :630, and all window consumers call echo_batch_resolve directly.bin/fm-watch.sh:2348- Round 1's R2 timestamp fix leaves an acknowledgment race. Both globs expand before iteration; if a worker then moves a newer steer from the inbox into handled/, the expanded pending path disappears and is skipped, while its handled path was never enumerated. With unchanged crew state and a blocking pane, R3 incorrectly drops the echo despite that steer. Treat disappearing enumerated records and failed timestamp reads as ambiguous evidence requiring delivery. Related sites: bin/fm-watch.sh:2347 expands both inbox locations; :2348 skips disappeared records; :2349-2350 ignores timestamp-read failure.🔧 Fix applied.
11 issues (7 errors, 4 warnings) still open:
bin/fm-watch.sh:2338- R2 does not bind answer evidence to the current gate. After run A's review is answered, a turn-end at run B's unanswered gate can reuse A's latest status key and historical resolution, causing the batch to be dropped. Reopening the same key also retains the earlier resolution as proof. Validate the current run/step and latest decision occurrence before dropping. Related sites: bin/fm-watch.sh:2262 selects an unbound historical key; :2287 accepts any historical resolution; :2291-2293 accepts substring matches in handled messages; :2336-2340 combines these proofs.bin/fm-watch.sh:2372- R3 misses steers sent within the delivery's second. A delivery at 1000.1 followed by fm-send at 1000.5 gives both records epoch 1000; the strict greater-than comparison allows an unchanged blocked echo to be dropped despite that steer. Treat equal timestamps as an ambiguous near miss and deliver. Related sites: bin/fm-watch.sh:2233 and :2446 create second-resolution baselines; :2369-2372 checks pending and handled inbox records.bin/fm-watch.sh:2412- A persisted ordinary-mode batch can survive into away mode, then be dropped by R2/R3 before the daemon receives it. New signals correctly bypass batching under .afk, but inherited batches do not. Apply the away-mode ownership guard at the shared flush boundary and promptly deliver inherited batches without watcher-side drops. Related sites: bin/fm-watch.sh:2199-2201 retains the normal deadline; :2412-2427 applies both drops; :3015 flushes before away-mode classification; :3294-3298 guards only new signals.bin/fm-watch.sh:2091- Numeric configuration is validated as decimal but retained for shell arithmetic: 060 passes the range check and becomes a 48-second window, while 090 causes an invalid-octal arithmetic error. Oversized digit strings can also survive failed range comparisons. Normalize bounded decimal input or reject noncanonical values in the resolver. Related sites: bin/fm-watch.sh:2087 validates; :2091 retains the raw value; :2106 checks enablement; :2196 performs arithmetic.bin/fm-watch.sh:2078- Simplification: the additional OFF/false/FALSE/no/NO disable aliases are unnecessary for the requested configurable batching window and exceed the documented off spelling. Remove these aliases and retain the narrower documented configuration surface unless broader spellings are explicitly desired.bin/fm-watch.sh:2429- A pending echo batch deadlocks process-event delivery. procevent_surface_queued holds FM_WAKE_QUEUE_LOCK until its post-output callback, but the new pre-output hook calls echo_batch_flush, which calls fm_wake_append and waits indefinitely for that same non-reentrant lock. Neither the immediate check nor subsequent wakes can print. Flush before acquiring the process-event lock, or append under its existing ownership. Related sites: bin/fm-push-transition-lib.sh:102 invokes the hook; bin/fm-watch.sh:2461 flushes; :2537 acquires the lock; :2549 installs its deferred release.bin/fm-watch.sh:2068- Simplification: FM_ECHO_BATCH_SECS introduces a second configuration surface and precedence rule beyond the requested config-file window ('Configuration lives in config/'). Remove the environment override and use config/wake-echo-batch, including in test fixtures. Related definitions: docs/configuration.md:197 and :1400.bin/fm-watch.sh:645- Simplification: remove the unused signal_statuses array and status-specific bookkeeping (:623, :645-647), plus the uncalled echo_batch_window wrapper (:2100-2103). No intent requirement needs either component: status files already return at :630, and all window consumers call echo_batch_resolve directly.bin/fm-watch.sh:2348- Round 1's R2 timestamp fix leaves an acknowledgment race. Both globs expand before iteration; if a worker then moves a newer steer from the inbox into handled/, the expanded pending path disappears and is skipped, while its handled path was never enumerated. With unchanged crew state and a blocking pane, R3 incorrectly drops the echo despite that steer. Treat disappearing enumerated records and failed timestamp reads as ambiguous evidence requiring delivery. Related sites: bin/fm-watch.sh:2347 expands both inbox locations; :2348 skips disappeared records; :2349-2350 ignores timestamp-read failure.bin/fm-watch.sh:2347- R3 treats inbox history as complete steer history, but fm-send explicitly routes slash commands, Codex dollar invocations, explicit endpoint messages, and keys through its typed plane without creating inbox records (bin/fm-send.sh:1020-1031, :1069-1078). After a delivered baseline, a typed steer can start another turn that leaves the crew-state line unchanged and blocks again; its echo is then incorrectly dropped. The prior rounds' inbox timestamp fixes leave these sibling delivery paths uncovered. Invalidate the existing delivered baseline at the shared task-bound send boundary for every steering plane, so typed steering cannot authorize a no-change drop.bin/fm-watch.sh:634- The new marker parsers truncate valid task IDs at the first '.turn-ended' occurrence. For tasks alpha and alpha.turn-ended.beta, the live marker alpha.turn-ended.beta.turn-ended.g1 passes incarnation filtering correctly, but this parser resolves it to alpha. With churn absorption enabled, alpha's working verdict can swallow the stopped task's wake; batching can likewise use alpha's answered-gate or unchanged-state evidence to drop it. The same invariant is violated at bin/fm-watch.sh:2232 (delivery baselines) and :2385 (both batch drop rules). Strip only the final marker suffix, matching the existing parsers at :2471 and bin/fm-classify-lib.sh:2294.🔧 Fix applied.
1 error still open:
bin/fm-send.sh:1004- Round 4's typed-steer fix leaves concurrent baseline access unsynchronized. The watcher can read a baseline, fm-send can delete it and deliver a typed steer, and the watcher can still drop the batch using its cached values because typed steering creates no inbox record. Conversely, fm-send deletes the baseline before actual transport; a concurrent wake can recreate it before the steer lands, allowing subsequent echoes to drop despite that steer. Coordinate invalidation, baseline publication, and the final drop decision at a shared task boundary. Related sites: bin/fm-watch.sh:2333-2355 reads and accepts invalidated evidence; bin/fm-watch.sh:2203-2211 publishes replacement evidence; bin/fm-watch.sh:2403-2408 commits the drop; bin/fm-send.sh:1004 invalidates before transport.🔧 Fix applied.
✅ Re-checked - no issues remain.
TMPDIR="$PWD/.validation/tmp" FM_WATCH_TRIAGE_TEST=test_echo_review_regressions bash tests/fm-watch-triage.test.shTMPDIR="$PWD/.validation/tmp" FM_WATCH_TRIAGE_TEST=test_wedge_threshold_defers_to_a_parked_gate_awaiting_a_human bash tests/fm-watch-triage.test.shTMPDIR="$PWD/.validation/tmp" bash tests/fm-send-inbox.test.shTMPDIR="$PWD/.validation/tmp" bash tests/fm-crew-state.test.shpython3 .validation/watcher-live.pypython3 .validation/churn-live.pypython3 .validation/stale-live.pypython3 .validation/block-live.pypython3 .validation/omp-live.pypython3 .validation/captain-live.pyVerified native OMP session records contain exactly one combined task notification and a separate ext-request notification.Reviewed other supported primary integration paths and backend capture interfaces; details retained in validation-notes.txt.Exported the OMP session and attempted Chrome rendering; closed the temporary tab after the export viewer's Highlight dependency failed.Collected child exit statuses, stopped disposable processes, removed temporary homes, and confirmed a clean worktree.✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.