Repository navigation
fix: improve Pi watcher recovery and processing retry visibility - #179
Merged
Merged
Conversation
…tension log opt-in (kunchenguid#5489) * Fix Pi watcher successor-gap confirmations and add extension log Accept an already-acknowledged handling confirmation as a no-op when the generation matches, confirm the restoration's own recovery token with a superseded (not rejected) outcome on generation mismatch, retire an arm on confirm failure only when the failed token names that exact pid, and record restore attempts, readiness timeouts, and confirm results in the bounded state/.watch-extension.log. Regression tests: already-acked no-op plus mismatch/dead-pid/lock-mismatch rejections and the manual-restart churn contract in fm-watch-arm.test.sh, and a mid-restore marker advance with no rejection appendix in fm-pi-watch-extension.test.sh. * Treat a dead arm child as an empty slot so repair and retry recover startArm and scheduleRetry answered unchanged while holding a ChildProcess whose OS process was already gone but whose close had not fired, so neither the repair tool nor the retry timer started anything until that close fired. Gate slot occupancy on a liveness check (exit/signal codes plus pid probe) and start a fresh arm instead, with a regression test driving the repair tool against a dead-but-unclosed child. * no-mistakes(document): Document new Pi extension log knob * no-mistakes(review): Fix confirm-failure retire token match, add distinct-pid test * no-mistakes(document): Clarify retire guard needs pid and generation * Make the Pi extension diagnostic log opt-in and default-off Only a positive FM_WATCH_EXTENSION_LOG_KEEP_LINES enables state/.watch-extension.log. Unset, empty, non-numeric, zero, and negative values disable logging entirely, so the default run writes nothing and never creates the file. The shared positiveInteger fallback semantics stay untouched for the retry and timeout knobs. docs/configuration.md owns the knob contract and docs/watcher-continuity.md points at it. Tests: the superseded-delivery case runs opted in, and a new case proves unset, zero, and non-numeric values create no log file while delivery still succeeds. * no-mistakes(document): Qualify extension-log coverage bullet as opt-in * no-mistakes(ci): The two reported checks (CI run 36372002913, Require no-mistakes run 36372069208) show conclusion action_required with 0 jobs and no logs because this is a fork PR (RibatTRW/firstmate) and GitHub is holding the workflow runs awaiting maintainer approval; that gate is external to the code and needs a maintainer to approve the runs. While verifying the change locally I found a real defect the approved CI run would hit: the PR's new test test_handling_delivered_rejects_a_superseded_generation failed deterministically. Invariant violated: reopen-announced (a non-successor/manual arm start) mints a fresh recovery generation only when the durable wake queue holds unrecovered work; an announced episode with an empty queue must be left untouched so idle arm starts never churn generations (the [ -s queue ] guard from kunchenguid#4819, relied on by bin/fm-watch.sh:2413 and covered by the append-reopens and announcement-bound sibling tests). The test called reopen with an empty queue and expected churn, so the fix establishes the queued-work precondition (append one wake, re-announce, re-read the generation) before asserting the reopen mints and the old confirmation mismatches. Test-only change, 14 lines in tests/fm-watch-arm.test.sh. Verified: fm-watch-arm.test.sh 25/25 ok on two consecutive runs, fm-pi-watch-extension.test.sh 55/55 ok, and shellcheck reports only one pre-existing warning outside the edited region * no-mistakes(document): Restore blank line in watcher-continuity docs * Route superseded Pi deliveries like confirmed ones and cover the retire guard A superseded handling confirmation now falls through to the normal delivery path, so an accepting supervision branch owns the wake instead of main. Scope the watcher-continuity token-pinned confirmation and narrowed retire rule to Pi, since omp and OpenCode still confirm against the current successor. Add tests that fail when the retire guard, the scheduled-retry gate, or the deferred-close gate is reverted, relabel the churned-generation characterization test, and use a reaped pid for the dead-pid rejection.
…es (kunchenguid#5863) * fix(pi): silence unacknowledged processing retry replies Suppress autonomous processing prose before persistence and during streaming while retaining tool calls, signed reasoning, and retryable outcomes. Restore ordinary output after acknowledgement or a user message. Fixes kunchenguid#4954 * no-mistakes(review): Silence only processing retries, keep first presentation visible * fix(pi): preserve differing processing retry replies
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
3 participants
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.
Merge upstream main into the fork's house line, preserving upstream commit ancestry with a merge commit.
The pinned upstream tip is
1f3e769616fdf9f31f85f4c3e6a9f71606634238, unchanged from intake.Fork main was advanced to the same tip through GitHub's non-force ref update after checking fast-forward ancestry; house remains unchanged pending this pull request.
Upstream commits
e31bc6e6fix(pi): restore watcher continuity across successor gaps and make extension log opt-in (fix(pi): restore watcher continuity across successor gaps and make extension log opt-in kunchenguid/firstmate#5489)1f3e7696fix(pi): hide duplicate assistant finals from hidden processing retries (fix(pi): hide duplicate assistant finals from hidden processing retries kunchenguid/firstmate#5863)The first change pins confirmation to its recovery token, handles superseded and already-acknowledged episodes, protects newer watchers, starts repairs over dead-but-unclosed children, and makes bounded extension diagnostics opt-in.
The second change hides empty or exact-repeat assistant finals from processing retries while retaining first and differing replies, tool calls, user responses, and retryable outcomes.
Conflicts and house behavior dropped
.pi/extensions/fm-primary-pi-watch.ts: house'srecoverRejectedHandlingDeliveryloop retired the current arm and restored additional successors after rejected confirmations, while upstream pins the original token and retires only its matching dead watcher.Prefer upstream's guarded handling and remove the house retry helper, which also retained the obsolete confirmation call signature after the automatic merge.
tests/fm-pi-watch-extension.test.sh: house's in-place recovery regression occupied the insertion point for upstream's successor protection, superseded delivery, logging, and dead-child repair regressions.Keep all upstream regressions; remove the house-only recovery test and permanent-rejection assertions that required extra arm restorations.
Dropped house behavior: automatic in-place successor replacement after a rejected handling confirmation, introduced by house commit
3bd6ac3b.A genuine rejection now surfaces one typed failure after a token-pinned confirmation retry; a superseded delivery follows normal routing without retiring another arm.
The independent house behavior hiding routine supervision outcomes remains, including its tests.
No other house behavior was dropped.
Validation
Six distinct targeted suites passed: Pi watcher extension, Pi branch extension, watcher arm, wake queue, Pi SDK branch guard, and strict Pi extension typecheck.
The SDK guard ran against installed Pi 0.87.1 with intercepted synthetic/offline provider streams and no external provider calls or account validation.
The initial npm-global package lookup failed/skipped because Pi is installed under the user's local prefix; the SDK guard and typecheck both passed after setting
FM_PI_PACKAGE_DIRto that installed package.All touched shell files passed canonical pinned ShellCheck lint, all five workflows passed pinned actionlint, documentation audience checks passed, and the staged diff passed whitespace checks.
The broader default lint invocation found two pre-existing SC2031 findings at lines 287 and 355 of
tests/fm-remote-job.test.sh.The file is byte-identical to origin/house; the same pinned ShellCheck invocation reproduced both findings on the file extracted from origin/house.
They are unrelated to this merge and remain unchanged.
The initial missing actionlint dependency was resolved with the repository's checksum-pinned installer into task scratch space.
No-mistakes Test returned an inconclusive live-validation verdict, and Firstmate explicitly approved proceeding for this upstream sync with the existing targeted tests.
This is a Test exception, not complete live Pi validation.
The following four scenarios were not driven live:
Synthetic/offline extension tests cover these behaviors; installed-SDK intercepted streams provide no external-provider evidence.
The disposable Pi attempt was limited by a pre-existing lab guard conflict and empty isolated provider credentials; it does not establish that the operator's normal login is unavailable.
House CI: House checks passed for published head
a7bf8f105640302291d4f7ed70311a378d3552fb; no-mistakes returnedchecks-passed.Target house and land as a merge commit; do not squash or rebase.
No-mistakes review completed with no findings.
The pipeline added documentation commit
a7bf8f10, clarifying matching watcher identity after acknowledgement, partial acknowledgement and retry suppression, retained message envelopes and comparison reset, and finite positive logging values; duplicated summaries now point to their authoritative owners.The canonical lint gate was accepted with the documented unchanged baseline warnings and its daemon-PATH actionlint limitation; this is not an all-clean canonical lint result.
evidence-artifact: /tmp/fm-hf-firstmate-upstream-sync-1004/sync-evidence.txt
evidence-command: python3 /tmp/fm-hf-firstmate-upstream-sync-1004/capture-evidence.py
evidence-captured: 2026-10-03T15:31:59+00:00
Pipeline attestation