Skip to content

fix(watcher): preserve PR-check wakes before suppression#52

Closed
tommy230 wants to merge 4 commits into
kunchenguid:mainfrom
tommy230:fix/check-suppress-enqueue-before-suppress
Closed

fix(watcher): preserve PR-check wakes before suppression#52
tommy230 wants to merge 4 commits into
kunchenguid:mainfrom
tommy230:fix/check-suppress-enqueue-before-suppress

Conversation

@tommy230

Copy link
Copy Markdown

Intent

The transcript is mostly omitted, but the visible context indicates the developer wanted work continued on PR or issue #34 by restoring its fixed branch and running it through a fork-aware no-mistakes validation flow. They expected the agent to operate under Firstmate rules: avoid direct project writes, use delegated work where appropriate, preserve unlanded work, and validate through the configured pipeline before reporting readiness.

What Changed

  • Move PR-check wake suppression into the watcher so merged-check notifications are enqueued before suppress markers are recorded, preserving lossless wake delivery while still preventing duplicates.
  • Clean stale check suppression markers during teardown so completed tasks do not leave behind watcher state.
  • Update watcher check documentation and add coverage for lossless PR-check wakes and teardown cleanup.

Risk Assessment

✅ Low: Captain, the change is well-bounded to watcher check wake dedup/catch-all cleanup behavior, with matching teardown cleanup and focused regression coverage; I did not find material merge-blocking risks.

Testing

Exercised the CLI-facing PR-check watcher flow end to end plus the targeted shell suites for crash recovery, enqueue-before-suppress, repeat deduping, catch-all escalation, and teardown marker cleanup; all checks passed and the final worktree status was clean.

Evidence: fm-wake-queue targeted test transcript
ok - supervise daemon state root is scoped by FM_HOME
ok - concurrent append plus drain preserves queue records
ok - signal written while no watcher runs is caught on next run
ok - stale wake is queued before suppressor state is advanced
ok - check output is queued before cadence suppression
ok - check wake is enqueued before suppression so a lost delivery is recovered
ok - watcher-side .seen-check dedup suppresses repeated identical check output
ok - catch-all force-escalates a swallowed terminal transition within one sweep
ok - simultaneous watcher starts leave exactly one live process
ok - two atomic drains cannot consume the same records twice
ok - drain collapses obvious duplicate heartbeat and signal records
ok - killed watcher stale lock is reclaimed
ok - live watcher lock with stale heartbeat is actionable
ok - guard warns when queued wakes are pending
ok - guard orders watcher re-arm after queued wake drain
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 - stale + terminal status escalates immediately
ok - persistent stale escalates after threshold and clears its marker
ok - resumed (busy) stale clears its marker without escalating
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 - afk flag present: daemon injects with sentinel marker prefix
ok - injected digest is single-line (no embedded newlines)
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: blank cursor line is not pending
ok - pane_input_pending: bare prompts are not pending (idle)
ok - composer guard defers injection when pane has pending input
ok - swallowed Enter: type-once + Enter-retry, no concatenation
ok - normal inject: exactly one digest, one Enter, no duplicates
ok - classify_signal dedupes against the catch-all scan seen marker
ok - classify_stale dedupes against the signal path seen marker
Evidence: fm-teardown targeted test transcript
ok - local-only worktree with HEAD on a fork remote is torn down (fix holds)
ok - local-only worktree with truly unpushed work is refused (safety preserved)
ok - local-only worktree with work merged into local main is torn down (no regression)
ok - no-mistakes worktree with HEAD on origin is torn down (no regression)
ok - no-mistakes worktree with truly unpushed work is refused (no regression)
ok - local-only worktree with unpushed work is torn down under --force (escape hatch)
ok - teardown removes watcher check dedup state
Evidence: manual PR-check/watch/drain transcript
$ FM_STATE_OVERRIDE=$MANUAL/state bin/fm-pr-check.sh task-pr https://example.test/pull/3095
armed: state/task-pr.check.sh polls https://example.test/pull/3095

$ sed -n 1,5p $MANUAL/state/task-pr.check.sh
state=$(gh pr view "https://example.test/pull/3095" --json state -q .state 2>/dev/null)
[ "$state" = "MERGED" ] && echo "merged"

$ PATH=$MANUAL/fakebin:$PATH FM_STATE_OVERRIDE=$MANUAL/state FM_POLL=1 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=0 FM_HEARTBEAT=999999 bin/fm-watch.sh
check: /var/folders/kg/vqcvwwlx3xs4wblm4wpvpkz00000gn/T/no-mistakes-evidence/01KVTN33M520MFG54DMN8E78EB/manual-pr-check-lossless/state/task-pr.check.sh: merged

$ FM_STATE_OVERRIDE=$MANUAL/state bin/fm-wake-drain.sh
1782232451	1	check	/var/folders/kg/vqcvwwlx3xs4wblm4wpvpkz00000gn/T/no-mistakes-evidence/01KVTN33M520MFG54DMN8E78EB/manual-pr-check-lossless/state/task-pr.check.sh	check: /var/folders/kg/vqcvwwlx3xs4wblm4wpvpkz00000gn/T/no-mistakes-evidence/01KVTN33M520MFG54DMN8E78EB/manual-pr-check-lossless/state/task-pr.check.sh: merged

$ repeat same terminal state: watcher should stay asleep and queue should stay empty
duplicate check output suppressed: watcher still sleeping after 2s

$ FM_STATE_OVERRIDE=$MANUAL/state bin/fm-wake-drain.sh (after duplicate)
queue empty after duplicate check output

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • bash tests/fm-wake-queue.test.sh 2>&1 | tee /var/folders/kg/vqcvwwlx3xs4wblm4wpvpkz00000gn/T/no-mistakes-evidence/01KVTN33M520MFG54DMN8E78EB/fm-wake-queue.test.log
  • bash tests/fm-teardown.test.sh 2>&1 | tee /var/folders/kg/vqcvwwlx3xs4wblm4wpvpkz00000gn/T/no-mistakes-evidence/01KVTN33M520MFG54DMN8E78EB/fm-teardown.test.log
  • Manual end-to-end smoke with fake gh pr view returning MERGED: ran bin/fm-pr-check.sh, bin/fm-watch.sh, bin/fm-wake-drain.sh, then reran the watcher on the same terminal state to verify no duplicate wake was queued; transcript at /var/folders/kg/vqcvwwlx3xs4wblm4wpvpkz00000gn/T/no-mistakes-evidence/01KVTN33M520MFG54DMN8E78EB/manual-pr-check-lossless.transcript.
  • git status --short
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

e-jung and others added 4 commits June 23, 2026 10:55
Checks were the only wake source whose suppression lived inside an opaque
script: an edge-triggered check advanced its own .babysit-*.seen marker
before the print could become a wake, so a lost stdout (timeout / concurrent
run / crash) permanently swallowed the transition. This is the root cause of
the missed PR #3095 merge (see data/fm-git-events-s8/report.md): the
.cli-printing-press-3095.seen sidecar advanced to MERGED but no check wake
was ever emitted.

Port the kunchenguid#29 enqueue-before-suppress invariant to checks:

1. Watcher-side suppression (bin/fm-watch.sh). The check always prints its
   current state (idempotent); the watcher dedups against .seen-check-<name>
   and calls fm_wake_append (durable queue) BEFORE advancing the marker. A
   crash between detect and suppress leaves the wake in the queue (recovered
   next turn) and the marker un-advanced (re-detected next cycle). A lost
   check wake is now impossible - exactly the guarantee signals enjoy.

2. Backward-compatible with old edge-triggered checks: empty stdout never
   produces a wake, so they keep their quiet behavior.

3. Catch-all backstop: force-escalate any .babysit-*.seen sidecar showing a
   terminal state (MERGED/CLOSED) the watcher never delivered a wake for.
   Catches a swallowed transition within one sweep; deduped via
   .escalated-<sidecar> so each terminal state fires at most once.

Tests (tests/fm-wake-queue.test.sh):
- test_check_wake_survives_lost_delivery: the #3095 regression - a check wake
  survives a simulated crash between enqueue and suppress (queue recovery +
  re-detection).
- test_check_dedup_suppresses_repeats: identical repeated output wakes once.
- test_catch_all_escalates_swallowed_transition: a self-suppressed check whose
  sidecar shows MERGED is force-escalated within one sweep.

AGENTS.md: the check contract now documents the stateless "always print
current state" form as preferred, and .seen-check-*/.escalated-* join the
enqueue-before-suppress marker list.

shellcheck clean; all 14 wake-queue tests + 5 spawn-batch tests pass.
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.

2 participants