fix(bin): suppress empty Grok watcher turns - #2250
Open
4mb1t10n wants to merge 15 commits into
Open
Conversation
4 tasks
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 #2227 exactly as specified by the full issue. Eliminate billable empty Grok watcher-cycle model turns while preserving every actionable wake, OPEN DECISIONS entry, watcher failure, durable-queue/lifecycle semantic, session-lock ownership boundary, away-mode supervision transfer, and supervision safety. Focus only on eliminating billable empty Grok watcher-cycle turns; exclude unrelated harness and quota work and exclude the upstream Grok automatic session recap behavior. Target main. Reproduce the billing-triggering control flow and add focused behavioral regression evidence. For the unchanged remote-decision watcher regression, inspect its exact expected contract and compare the owning implementation and fixture against main before changing implementation; if unrelated or pre-existing, record equivalent focused evidence, and if caused by this branch, fix it causally rather than repeating the same test-only attempt. No database or browser behavior is involved; do not start a stack and never run Cypress locally. Complete No Mistakes, obtain exact-head authoritative CI and mergeability, never merge, and ensure the final PR body contains a standalone Closes #2227.
What Changed
Closes #2227
Decisions
.afk. If ownership transferred or away mode took supervision, keep the former Grok task dormant and leave durable work or failure evidence for the current owner. Executable coverage exercises actionable-close, classification-failure, and arm-failure transfer sequences.Risk Assessment
✅ Low: The change now consistently enforces the ownership and away-mode boundary across all completion paths, preserves transferred failures through the durable queue, and performs bounded identity- and membership-checked child retirement without a substantiated remaining defect.
Testing
Targeted shell tests exercised the end-user Grok tracked-task flow and rendered launch/policy surfaces: empty closes re-armed without completing the tracked task, while durable wakes, OPEN DECISIONS, recovery states, and failures completed visibly; ownership transfer, away mode, lifecycle, bounded-output, and cleanup safety also passed. Equivalent open-decision coverage passed in the new suite, and the unchanged remote-decision contract matched base. No visual artifact was attempted because the authoritative intent explicitly excludes browser behavior.
Evidence: Grok persistent watcher behavioral transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (12) ✅
bin/fm-watch-arm-grok.sh:189- The required “session-lock ownership boundary” and “away-mode supervision transfer” remain bypassable when either state changes while the child arm is running: after the child closes,durable_action_pendingcan return actionable and this branch immediately completes the old Grok task without rechecking ownership or.afk. That wakes a session which no longer owns supervision (or competes with away-mode supervision). Recheck these boundaries after every child close and before actionable completion; when ownership transferred, leave the durable work for its new owner and keep this old task dormant. The added tests cover transfer only after an empty close, so they do not exercise this reachable sequence.🔧 Fix: Guard actionable Grok closes after supervision transfer
1 error still open:
bin/fm-watch-arm-grok.sh:196- The ownership/away check occurs beforedurable_action_pending, which can wait on the queue lock and scan other state. If.afkappears or.locktransfers during that classification, verdict 0 still completes the old Grok task. Recheck supervision ownership immediately before this exit; the new tests transfer only inside the child arm and therefore miss this remaining sequence.🔧 Fix: Recheck Grok ownership after durable classification
1 error still open:
bin/fm-watch-arm-grok.sh:212- A supervision transfer duringdurable_action_pendingis handled only when the verdict is actionable. If.afkappears or the session lock changes while classification waits and classification then fails (for example, the queue lock times out), this branch still completes the old Grok task with a failure, violating the required away-mode/session-ownership boundary and waking a session that no longer owns supervision. Recheck supervision ownership after classification for every completing verdict; if ownership transferred, leave the old task dormant and let the new owner surface durable state or failure.🔧 Fix: Centralize post-classification Grok ownership guard
1 error still open:
bin/fm-watch-arm-grok.sh:183- The required “session-lock ownership boundary” and “away-mode supervision transfer” remain bypassable for child-arm failures. After the check at line 176, failure detection can inspect the captured output; if.afkappears or.locktransfers during that work, lines 183–190 still complete the old Grok task and wake its former session. Route child failure through the same centralized ownership guard immediately before any task completion, and add executable coverage for transfer during failure classification.🔧 Fix: Centralize all Grok completion ownership checks
1 error still open:
bin/fm-watch-arm-grok.sh:199- The captain required transferred completion paths to “leave durable work/failure evidence for the new owner,” and the intent requires preserving every watcher failure. Here a classification failure exists only incompletion_failure; if ownership or away mode changes, lines 205–209 delete the stream and continue without printing or durably recording that failure. The old task then remains dormant, so neither it nor the new supervisor surfaces the failed classification. Persist the failure at the centralized completion boundary before yielding to the new owner, using the earliest existing durable failure/recovery channel.🔧 Fix: Persist transferred Grok failures through recovery
1 error still open:
bin/fm-watch-arm-grok.sh:205- Transferred failures are reduced to a generic downtime marker, so the required watcher failure evidence is lost; additionally, publishingdowntimerewrites an existingpending:handling:<generation>episode topending:downtime:<generation>, making an already-delivered recovery actionable again and risking duplicate resurfacing. Persist the actual failure through the existing durable wake channel without changing an active recovery episode. The new tests only verify a fresh generic marker, not failure visibility to the successor or coexistence withpending:handling.🔧 Fix: Queue exact transferred failures without reopening recovery
1 error still open:
bin/fm-watch-arm-grok.sh:168- The accepted transfer contract says not to print or complete the old task, butteestreams child output before the final ownership/away-mode check. A child can printwatcher: FAILED, then ownership transfers while failure classification is blocked; the failure is durably queued for the successor, yet it has already been printed through the former owner. Buffer child output until the centralized completion boundary, emitting it only when this owner may complete; on transfer, publish the captured failure solely through the durable queue. This also avoids printing ordinary child failures twice viateeand line 215.🔧 Fix: Buffer Grok terminal output behind ownership checks
1 error still open:
bin/fm-watch-arm-grok.sh:169- The accepted fix explicitly requires a “bounded reader,” but this loop writes every child-output line tooutputand usually a second copy toterminalwithout any byte or line limit. A noisy or malfunctioning arm can therefore grow state files until disk exhaustion and make terminal replay arbitrarily long, undermining supervision safety and widening the final ownership-transfer window. Enforce a finite capture limit and convert overflow into a typed failure handled through the existing completion boundary.🔧 Fix: Bound Grok output and handle overflow safely
1 error still open:
bin/fm-watch-arm-grok.sh:184- The required finite output cap is bypassable by a child that emits an unbounded stream without newline characters. Bashreadbuffers until a delimiter or EOF, so this loop never reaches the byte-count check, cannot retire the child, and can grow memory indefinitely. This contradicts the required supervision safety and bounded-reader behavior. Consume fixed-size byte chunks or otherwise enforce the cap before newline framing, then route overflow through the existing centralized completion boundary. Add executable coverage using a no-newline producer.🔧 Fix: Enforce Grok cap before newline framing
1 error still open:
bin/fm-watch-arm-grok.sh:192- Output overflow sends only SIGTERM and the parent then waits indefinitely for the child. A malfunctioning/noisy arm that ignores or mishandles SIGTERM can therefore keep producing output, leave the tracked owner hung, and prevent the typed overflow failure from reaching either owner. The new cleanup tests use cooperative TERM handlers, so they miss this reachable failure. Add a bounded TERM grace period followed by forced retirement of the child/process tree before completing through the centralized ownership boundary.🔧 Fix: Bound Grok overflow process-tree retirement
1 error still open:
bin/fm-watch-arm-grok.sh:104-pidsis a cached list collected before the TERM grace, then every numeric PID is sent SIGKILL without revalidating that it still belongs to this child tree. A cooperative descendant can exit during the grace and its PID can be reused by an unrelated process, which this loop would then kill—violating the supervision-safety requirement and the requested exact-child identity boundary. Before KILL, re-resolve descendants/process-group membership and validate identity; never signal stale cached PIDs.🔧 Fix: Validate Grok child identities before forced retirement
1 error still open:
bin/fm-watch-arm-grok.sh:131- Forced retirement still retains every pre-TERM identity record and sends KILL based only on unchanged PID identity. If the root exits during the grace period and a descendant is reparented, that process is no longer in the currently resolved child tree but lines 131–134 retain and kill it anyway, violating the required current-membership check. Build KILL candidates only from the freshly resolved tree intersected with recorded identities; treat escaped former descendants as incomplete retirement without signaling them. The new mismatch test covers changed identity, not unchanged identity after reparenting.🔧 Fix: Require current membership before forced Grok retirement
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
gh-axi issue view 2227 --fullbash tests/fm-grok-watch-arm.test.shbash tests/fm-supervision-instructions.test.shbash tests/fm-arm-pretool-check.test.shbash tests/fm-watch-arm.test.sh(the unchanged remote-decision fixture reached three passing cases before its pre-existing Linux process-kill behavior terminated the invoking shell)git diff b5d430d6fdcd961ce9b681bf196f365c1825c284..636b1654872a93aa35765a2be091f8b155aebf1b -- tests/fm-watch-arm.test.sh bin/fm-watch-arm.sh bin/fm-watch-once.sh bin/fm-wake-daemon.sh(confirmed the remote-decision fixture and owning implementation are unchanged from base)✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: Fix subshell-safe classification lock path
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.