Skip to content

self-merge wake re-presented on a later drain after the merging turn already reported it #1

Open

Description

@NewAiCoder

Summary

Investigated as candidate "shape 5" for kunchenguid#3285 (fewer no-op model turns for the Claude primary). The observed symptom - a check: merge landed: <id> <url> wake surfacing again after the same turn already reported the merge (seen tonight for one PR in another project and two fork PRs) - is not a duplicate-detection bug in bin/fm-merge-outcome-lib.sh/bin/fm-watch.sh's merge-poll dedup, which already works correctly. It is the ONE intended durable wake from a main home's own self-merge surviving, unacknowledged, into a later wake-drain, where it gets presented again as if new even though the merging turn already told the captain the outcome directly.

Evidence this is not a duplicate-detection bug

fm_merge_outcome_report (bin/fm-merge-outcome-lib.sh) is idempotent per PR identity via the pr-poll-merge-notified marker (bin/fm-pr-lib.sh): a second call with the same task/provider/host/path/number returns FM_MERGE_OUTCOME_ALREADY_RECORDED=true and queues nothing. bin/fm-watch.sh's poll loop (is_pr_poll=1 && out=merged branch) checks that flag and continues past the generic wake-append when it is true, after retiring the poll.

Verified two ways, unmodified code:

  1. A direct unit-level repro calling fm_merge_outcome_report twice (origin=self, then origin=poll) for the same PR identity in a main home (no parent-channel destination): the wake queue holds exactly one row for that PR both before and after the second call.
  2. The full existing tests/fm-pr-check-security.test.sh suite (56 tests, unmodified) passes, including test_self_merge_and_poll_publish_one_outcome, which exercises exactly this self-then-poll interleaving (in a secondmate home).

I also tried gating the local wake to origin = poll only (dropping the || [ -z "$destination" ] clause that lets a main-home self-merge queue its own local wake) - this is the fix that would make "no wake row survives a self-merge" literally true, but it broke tests/fm-pr-merge.test.sh's test_main_home_merge_leaves_a_durable_wake, which explicitly requires a main home's own self-merge to leave exactly one durable wake naming the PR. That test's rationale (inferred, not stated in this pass): crash-durability - if the session dies between the merge call and reporting to the captain in chat, the wake queue is the only trace that survives. Reverted; no production code changed in this pass.

The actual mechanism (best-supported hypothesis, not fixed here)

bin/fm-pr-merge.sh is only ever invoked by the main session itself, synchronously (bin/fm-lease-lib.sh refuses a branch actor's attempt to call it), so a self-merge is always known to its caller in the same turn - AGENTS.md section 7 already has that caller report the outcome to the captain directly ("After an autonomous merge, give the captain a one-line full-URL or local-main outcome"). But fm_merge_outcome_report's wake-queue row for that self-merge is never acknowledged/dequeued by that same turn. It sits in .wake-queue until some later drain reads it - which could be the very next turn - and gets presented again as a check: wake, which the supervision protocol (AGENTS.md section 8, item 3) always escalates, forcing a turn that has nothing new to do.

Candidate designs (neither implemented)

  1. Ack-at-report: bin/fm-pr-merge.sh dequeues its own just-queued wake row immediately after a successful self-merge, on the theory that the calling session's direct chat report (mandated by AGENTS.md section 7) is the durable-enough record, backed by the pr-poll-merge-notified marker and the task's own meta/status. Simplest, but removes the crash-recovery trace test_main_home_merge_leaves_a_durable_wake currently locks in; that test's expectation would need to change too, and whether the marker + task status are actually sufficient crash-recovery replacements needs a real answer, not an assumption (does anything durable point back to "this task's PR merged" without the wake row, for a session that crashed before reporting?).
  2. Durable "reported" marker the drain reads: leave the wake queued (keeping the crash-durability trace as-is), but have the same turn that performs the self-merge also write a marker (or reuse notice_emitted/a new field on the terminal-outcome-style record) that a later wake-drain checks before re-presenting a merged-<id>-<url> row it finds still queued - i.e., "this exact row was already delivered synchronously in the turn that created it; drain it silently instead of escalating." Keeps the safety net for the crash case (nothing to acknowledge => still surfaces) at the cost of a second bookkeeping mechanism the drain path has to consult.

Repro scripts

Both ad hoc scripts used for the evidence above (not part of the tracked test suite) are described here for whoever picks this up; happy to hand over the exact scripts on request rather than pasting throwaway paths from a session scratch directory.

Not in scope for this issue

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions