fix: optionally absorb turn-end wakes on bounded pane churn - #2877
fix: optionally absorb turn-end wakes on bounded pane churn#2877karotkriss wants to merge 16 commits into
Conversation
Confidence Score: 4/5The PR is not yet safe to merge because a churn reset can inherit prior wedge escalation state and demand deep inspection prematurely. The retained Files Needing Attention: bin/fm-watch.sh Reviews (4): Last reviewed commit: "no-mistakes(lint): Fix watcher arithmeti..." | Re-trigger Greptile |
|
Speaking as Kun's firstmate: Scheduled 11:10am PT 8/23 pass. VISION.md read in full from current main VISION (inspected Class: default-behavior. Default-behavior is never auto-eligible. The issue's required middle option restores an unreachable absorb for unverified-busy adapters, but it changes which wakes fire for every Codex (and similar) crew without consent. Security: none. No workflow-file / secret / injection risk. Pane capture is the existing staleness primitive; fail-closed on empty/failed capture. Overlap / HOLD: not a standing spawn/teardown/herdr hold. Same-file overlap with open #2701 / #2884 / #2809 / #2796 on CI / NM: HEAD Workflows: already approved (CI completed SUCCESS on this HEAD). Run IDs: 32652354998 (CI), 32652354993 (Require no-mistakes). No pending first-time-fork approval. Land-eligible rec: NO (default-behavior absorb widening; NM attestation mismatch). Captain-flag NOW: no. |
|
The firstmate's triage is accurate. Two clear blockers:
Both blockers are independent and either alone is sufficient to hold eligibility. The fix itself is coherent and the fail-closed properties are real, but it cannot advance without:
|
47617a1 to
5d8c3e4
Compare
|
Speaking as Kun's firstmate: Scheduled 3:10pm PT 8/23 pass. VISION.md read in full from current main VISION (inspected Class: default-behavior. Default-behavior is never auto-eligible. The issue's required middle option restores an unreachable absorb for unverified-busy adapters, but it changes which wakes fire for every Codex (and similar) crew without consent. Security: none. No workflow-file / secret / injection risk. Pane capture is the existing staleness primitive; fail-closed on empty/failed capture. Overlap / HOLD: not a standing spawn/teardown/herdr hold. Same-file overlap with open #2701 / #2809 / #2796 / #2320 / #2882 / #2867 on CI / NM: HEAD Workflows: already approved (CI completed SUCCESS on this HEAD). Run IDs: 32662209266 (CI), 32662209251 (Require no-mistakes). No pending first-time-fork approval. Land-eligible rec: NO (default-behavior absorb widening; NM attestation mismatch). Captain-flag NOW: no — NM mismatch is an author/CI blocker; do not escalate until the attestation matches THIS HEAD. Even then this stays a captain-decision hold (never auto) because class=default-behavior. Waiting on the author for a HEAD-matching no-mistakes attestation. Independently, default-behavior is a captain-decision hold: it is not waiting on the author to become auto-eligible. |
|
Triage receipt confirmed and recorded at this HEAD. On the NM attestation mismatch: The body still attests On Captain-decision hold stands: default-behavior widening with no flag, and the architecture-level question above, are both captain decisions. The NM attestation mismatch is an independent author blocker. Neither unblocks automatically. |
The watcher's "absorb a benign turn-end when the crew is provably working" triage was structurally unreachable for any harness whose semantic busy state has no verified source. crew_absorb_class only reports working for an actively running no-mistakes step or an exact busy verdict, and bin/fm-crew-state.sh can only answer unknown for such an adapter, so codex crewmates surfaced a signal wake at every turn boundary with nothing to act on - a full supervisor drain, inspect and acknowledge turn per worker turn, scaling with the number of workers in flight and drowning the wakes that matter in identical noise. Widen the proof rather than bound the wake rate. A wake carrying only bare turn-ended markers is now also benign when the task's pane content changed since the previous poll, compared against the same state/.hash-* marker the staleness backbone already records and already trusts as liveness. That evidence claims no harness semantics, so it fabricates no busy verdict an adapter has not earned, and it needs no adapter cooperation. Absorb stays evidence-driven in both directions. A wake naming any status file keeps the strict proof, every captain-relevant verb still surfaces immediately, and an unresolvable task, a missing prior hash, a failed or empty capture, or an unchanged pane all surface exactly as before. The absorb defers rather than swallows: a crew that has stopped renders nothing further, so its now-static pane surfaces through the staleness backbone within a poll or two. Bounding the surfacing rate instead would have suppressed genuinely stopped workers. The derivation lives with the .hash-* marker format in bin/fm-watch.sh, which owns it, and costs one bounded capture reached only for a no-verb turn-end whose crew is not already provably working.
…ions and secondmates
…store Herdr sourcing
Make the pane-churn form of positive work evidence opt-in per home and bound how long it may defer one endpoint's bare turn-ends. Absorbing a bare turn-end on pane churn is now reached only when the home creates config/turnend-churn-absorb. The other two proofs read a verdict the harness itself vouches for, while this one infers execution from rendered bytes, so widening the absorb is a home's choice rather than a default every fleet inherits. With the flag absent the predicate returns on its first line and triage is unchanged. Churn and pane staleness read the same pane, so neither can be the other's only backstop. A pane that renders continuously never presents the two consecutive identical hashes the staleness backbone needs, so an unbounded churn absorb left a worker that had genuinely stopped behind such a renderer with no path to surface at all. One endpoint's turn-ends may now ride churn evidence for at most FM_TURNEND_CHURN_ABSORB_SECS, tracked in state/.churn-since-*, after which the wake surfaces and the window restarts. The bound is evaluated before any .stale- state is touched, so a wake that surfaces there leaves the staleness backbone's own classification alone. Covers both with behavioral tests: the same churning fixture that absorbs with the flag surfaces and queues without it, and a spent deferral window surfaces and restarts. The four existing safety guards now run with the flag enabled so they keep proving their specific guard.
5d8c3e4 to
de5edc0
Compare
| for key in "${churned_keys[@]}"; do | ||
| if ! rm -f "$STATE/.stale-$key"; then | ||
| for created in "${created_keys[@]}"; do | ||
| rm -f "$STATE/.churn-since-$created" | ||
| done | ||
| return 1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Wedge escalation survives churn reset
When a churn-proven pane begins a new quiet interval after previously accumulating wedge escalations, this cleanup removes only .stale-$key and retains .wedge-escalations-$key. A later provably-working stale classification increments the old counter, causing demand-deep-inspection to surface after fewer current-interval escalations than configured.
Intent
Fix issue #2374 in kunchenguid/firstmate: turn-end wakes are never absorbed for a harness whose semantic busy state has no verified source (codex is the concrete case), so a primary running N codex crewmates is woken once per worker turn, indefinitely, with nothing to act on. Each wake costs a full supervisor turn and real tokens, scaling with worker count. The required and binding fix shape is the issue's own middle option: widen positive-work evidence to include pane-content churn since the previous poll, which is harness-independent and already computed on the staleness path, while preserving absorb-only-on-positive-evidence semantics. Explicitly NOT the issue's third option of bounding the surfacing rate per task, because that would suppress genuinely stopped-worker wakes, and explicitly NOT giving codex a fabricated semantic busy source, because harness adapters must earn trust through verification. The predicate lives in bin/fm-watch.sh rather than bin/fm-classify-lib.sh because it consumes the state/.hash-* marker format that bin/fm-watch.sh declares it alone owns.
This round is rework accepted from maintainer review of the existing PR 2877. Two review requirements are now part of the accepted intent and supersede the original "no new configuration knob, no new state file" constraint for exactly these two points and nothing else:
OPT-IN, not a default-behavior change. The project's vision rule is that new capability arrives as opt-in, and the reviewers held this PR as class=default-behavior because it widened the default absorb with no flag. The widening is now gated on the home creating config/turnend-churn-absorb. With the flag absent - which is every existing home - signal_turnend_panes_churned returns on its first line and triage is byte-for-byte the pre-change behavior. The rationale for keeping it opt-in rather than defaulting it on is that the other two proofs read a verdict the harness itself vouches for while this one infers execution from rendered bytes, which is a weaker claim and therefore a home's choice to make. The flag is local and gitignored, and deliberately NOT inherited by secondmate homes, because it is a home-local supervision-noise preference and a mate runs its own crew mix.
Pane churn must not be able to perpetually mute the stale-recovery path. Reviewers observed that the churn absorb and the staleness backbone read the same pane, so one continuously-rendering pane could silence both. That concern is correct, though one detail of the stated mechanism is not: state/.stale- stores the hash the backbone has ALREADY CLASSIFIED, so it is a dedupe record and not the timer; clearing it makes a later stale render more likely to surface, not less. The real mute is upstream of both paths - a pane that renders continuously (a terminal clock, an animated prompt, a status bar, or a harness that leaves a background renderer alive after its agent yields) never produces two consecutive identical hashes, so .count- never reaches 2 and the staleness backbone never classifies it at all, which is already true on main. What this change added was a second path that also stayed quiet on the same evidence, leaving a worker that had genuinely stopped behind such a renderer with no path left to surface. The fix is therefore a bound on the deferral rather than a change to the dedupe record: one endpoint's bare turn-ends may ride churn evidence for at most FM_TURNEND_CHURN_ABSORB_SECS (default 900), tracked per window in state/.churn-since-*, after which the wake surfaces and the window restarts, so a perpetually churning pane produces at most one turn-end wake per window instead of either one per turn or none ever. The bound is evaluated BEFORE any .stale- state is touched, so a wake that surfaces there leaves the staleness backbone's own classification untouched. The .stale- clear is deliberately kept for its original reason: a later stopped render whose bytes happen to match an earlier classified stale hash must surface through ordinary staleness rather than inherit the earlier interval's wedge timer.
Both requirements are covered by behavioral tests colocated in tests/fm-watch-triage.test.sh: the same churning fixture that absorbs with the flag surfaces and queues without it and opens no deferral window, and a spent deferral window surfaces, queues, and restarts. The four existing safety guards (a .status file anywhere in the batch, a secondmate, a malformed prior hash, an ambiguous marker key) now run with the flag enabled so they keep proving their specific guard rather than passing vacuously on the disabled path.
Every negative outcome of the predicate still returns 1 so absence of evidence surfaces exactly as before: the flag being absent, a status file in the batch, a secondmate, an unresolvable task, no recorded endpoint, an ambiguous marker key, a missing or malformed prior hash, a capture that fails or returns empty, an exhausted deferral bound, and an unchanged pane.
Binding constraints, unchanged from the original task. This is firstmate's own shared tracked material, so .agents/skills/firstmate-coding-guidelines applies: the knowledge-placement decision tree, one owner per contract with cross-references rather than restatements, one full sentence per line in tracked Markdown, plain dash and never an em dash, never an agent name as commit co-author, bin/*.sh shellcheck-clean through bin/fm-lint.sh which is the single owner of the lint definition, and tests colocated in tests/ extending the existing runner and exercising behavior through the executable interface rather than asserting implementation source bytes. docs/architecture.md owns the watcher triage contract and docs/configuration.md owns the operator-facing flag and environment variable, with AGENTS.md carrying only the one-line layout entries. Prefer quality, simplicity, robustness, scalability and long-term maintainability over development speed, and the smallest diff in the right place rather than a patch that papers over a symptom.
Delivery: update the EXISTING pull request 2877 against kunchenguid/firstmate in place, pushed to the karotkriss fork remote, never to the clone's default branch, and never opening a duplicate PR. The body must cite "Fixes #2374" and nothing else. The pipeline attestation must bind to the final head, so no commits are pushed after the stamp.
Known out of scope, deliberately excluded and reported separately rather than folded in: tests/fm-watch-triage.test.sh contains pre-existing load-sensitive flakes (test_nonterminal_stale_repairs_missing_or_corrupt_timer and test_paused_authoritative_working_preserves_wedge_timer both use a 3-second wait_numeric_file budget measured from watcher process spawn), proven by a 6-round interleaved A/B against pristine origin/main to flake independently of this change. Those belong to their own task, and this PR must close #2374 only.
What Changed
Risk Assessment
✅ Low: The opt-in change is localized, bounded, fail-closed, and preserves the authoritative default path without any substantiated correctness or intent-conformance defects.
Testing
After correcting an initial test-wrapper path setup error, all focused watcher tests and manual evidence scenarios passed. The opted-in Codex-style wake was absorbed with a bounded deadline, while default-off and expired-deadline wakes surfaced into the durable queue. No visual evidence was captured because this is a shell-based supervision behavior with no rendered UI.
Evidence: End-user watcher behavior transcript
Source: End-user watcher behavior transcript
Evidence: Focused turn-end churn test matrix
Source: Focused turn-end churn test matrix
Evidence: Secondmate status guard tests
Source: Secondmate status guard tests
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed (5) ✅
bin/fm-watch.sh:450- The accepted constraint says "No quadratic work," but each referenced task invokesrecorded_windows, which scans every.meta; a coalesced batch of N turn-ends therefore performs O(N²) metadata reads. Build marker-key counts in one predicate-local fleet scan, then check each referenced task against that map. This preserves the localized guard without adding per-poll work, and I recommend fixing it because worker-count scaling is the issue's core concern.bin/fm-watch.sh:470- Failure to create.churn-since-<key>is ignored, so corrupt state such as that path being a directory makes every churning turn-end absorb without ever establishing the load-bearing deadline. Require the marker write to succeed or return 1 and surface fail-closed.bin/fm-watch.sh:472-FM_TURNEND_CHURN_ABSORB_SECSis used in arithmetic without validation. A value such asbogusaborts the watcher underset -uinstead of surfacing the wake. Validate a positive integer before this predicate can absorb, otherwise return 1 fail-closed.🔧 Fix: Fail closed on invalid churn deferral state
3 errors still open:
bin/fm-watch.sh:457- The accepted constraint forbids quadratic work, but a batch of N unique turn-end tasks callsrecorded_windowsfor every task, rereading all N metadata files each time;window_backendandwindow_labelalso perform fleet scans. Build marker-key counts from one predicate-local fleet snapshot and use the already-known task metadata for capture arguments.bin/fm-watch.sh:471- Existing deadline state is not validated fail-closed. For example,.churn-since-<key>=09reaches Bash arithmetic as invalid octal and terminates the watcher, a future timestamp produces a negative age and exceeds the configured maximum, and nonnumeric content is overwritten before the wake is absorbed. Initialize only an absent marker; require existing content to be a representable canonical base-10 epoch no later thannow_s, otherwise return 1 without rewriting it.bin/fm-watch.sh:1403- Batch evidence is combined as(every task authoritative) OR (every pane churned)instead of requiring positive evidence per task. With task A authoritatively working on a static pane and unverifiable task B on a churning pane, the first predicate fails on B and the churn predicate fails on A, so both turn-ends surface although each task has eligible positive evidence. Evaluate the proof per task, applying churn only where authoritative evidence is absent.🔧 Fix: Validate persisted churn deadlines before arithmetic
4 issues (2 errors, 2 warnings) still open:
bin/fm-watch.sh:457- The accepted constraint says "No quadratic work," but each task callsrecorded_windows, scanning all metadata, followed by additional fleet scans inwindow_backendandwindow_label. A batch of N tasks therefore performs O(N²) metadata reads. Build one predicate-local endpoint snapshot and key-count map, then reuse its task metadata for capture arguments.bin/fm-watch.sh:1410- Positive evidence is combined per proof type instead of per task. If task A is authoritatively working with a static pane and task B is unverifiable with a churning pane, both predicates return false and the whole batch surfaces despite every task having eligible evidence. Evaluate authoritative-or-churn evidence separately for each referenced task.bin/fm-watch.sh:474- Deadline creation is not transactional across a batch. If task A has no marker and a later task has an invalid, expired, or unwritable marker, A's.churn-since-*is created before the predicate returns 1 and surfaces the batch. A later turn can therefore inherit a deferral interval that never absorbed a wake. Validate every key first, then create missing markers only after the batch is known absorbable.bin/fm-watch.sh:434- The all-digit bound is converted with fixed-width Bash arithmetic without a representability check. An oversized positive decimal can wrap to another positive value and be accepted with a materially different deadline. Reject values beyond Bash's supported positive integer range before arithmetic expansion.🔧 Fix: Make churn deadlines transactional and bounds safe
4 errors still open:
bin/fm-watch.sh:462- The binding constraint forbids quadratic work, but every referenced task invokesrecorded_windows, thenwindow_backendandwindow_label, each scanning all metadata. A coalesced N-task turn-end batch therefore performs O(N²) metadata reads. Build one predicate-local metadata snapshot with marker-key counts and capture metadata, then reuse it for every task.bin/fm-watch.sh:1431- Positive evidence is composed by proof type rather than by task. For a batch where task A is authoritatively working on a static pane and task B has an unknown verdict but a churning pane, both aggregate predicates fail and the entire batch surfaces despite every task having eligible evidence. Evaluate authoritative-or-churn evidence independently for each referenced task.bin/fm-watch.sh:462- The ambiguity guard counts deduplicated window values, not recorded endpoint entries. If two task metadata files record the same target,recorded_windowsemits it once,matchesremains 1, and one task can absorb using marker evidence that is not uniquely attributable to it. Count metadata records by derived key in the predicate-local snapshot instead of counting deduplicated windows.bin/fm-watch.sh:464- Command substitution strips trailing newlines before validation, so a.hash-*containing32_lowercase_hex + newlinepasses as valid even though the accepted fail-closed contract requires every whitespace-bearing marker to surface. Preserve the exact file bytes while validating the digest format.🔧 Fix: Compose turn-end evidence per task from one snapshot
3 errors still open:
bin/fm-watch.sh:494- With the opt-in enabled, an authoritative verdictcontinues before the status and secondmate guards. A working secondmate's.statusreply is therefore absorbed, and a batch containing a working task's.statusplus another task's churn-proven turn-end is also absorbed. This contradicts the required status-file and secondmate fail-closed behavior. Route any status-bearing batch throughsignal_crew_provably_workingbefore allowing per-task churn evidence.bin/fm-watch.sh:453- The binding opt-in contract requires the absent-flag path to return on the function's first line, but the gate runs only after the new filename parsing and quadratic task deduplication. Move the flag check before all new processing so existing homes immediately delegate to the unchanged authoritative predicate.bin/fm-watch.sh:478- The requested snapshot removed repeated metadata reads but not quadratic work: each referenced task linearly searches all snapshot tasks and then recounts its key across every snapshot record. With N signaled tasks and N records this performs roughly 2N² Bash comparisons. Derive task indexes and per-key record counts once while constructing or normalizing the snapshot, then perform direct per-task lookups.🔧 Fix: Restore strict turn-end fallback guards
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
Ran 18 focused executable cases fromtests/fm-watch-triage.test.shcovering opted-in pane-churn absorption, unchanged-pane surfacing, stale recovery, default-off behavior, bounded deferral, mixed positive evidence, malformed markers, collisions, status and secondmate guards, invalid bounds, and transactional deadline handling.Rantest_secondmate_status_signal_never_absorbed_classifierandtest_secondmate_status_note_surfaced_despite_busy_agent.Executed three end-user-aligned watcher scenarios and captured actual watcher output, durable queue records, absorb audit output, and.churn-since-*state for opted-in, default-off, and exhausted-deadline behavior.Rangit status --shortafter testing and confirmed the worktree remained clean.✅ **Document** - passed
✅ No issues found.
🔧 Fix: Fix watcher arithmetic lint issues
1 warning still open:
✅ **Push** - passed
✅ No issues found.