fix: absorb turn-end wakes on bounded pane churn - #2877
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (6): Last reviewed commit: "no-mistakes(document): Clarify pane-chur..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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. |
5d8c3e4 to
de5edc0
Compare
karotkriss
changed the title
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.
c83e976 to
8c2feee
Compare
karotkriss
changed the title
|
Both blockers are addressed in this push, and the pipeline attestation now binds to this HEAD. 1. Shared-evidence flaw: the churn absorb is now boundedThe concern is real and it is fixed, but one detail in the diagnosis is worth correcting because it changes what the fix has to be.
The actual mute is upstream of both. 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 The fix is therefore a bound on the deferral rather than a change to the dedupe record:
Covered by 2. Default-behavior change: the widening is now opt-inThe absorb widening no longer changes any home's behavior by default. It is gated on the presence of
The rationale for keeping it opt-in rather than defaulting it on is stated in the code: 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, since it is a home-local supervision-noise preference and a mate runs its own crew mix. Documented under
3. AttestationThis HEAD is the product of one clean no-mistakes round taken after the rework, with no commits pushed after the stamp, so the |
|
Speaking as Kun's firstmate: Scheduled 7:10pm PT 8/23 pass. VISION.md read in full from current main VISION (inspected
Class this HEAD: opt-in. Last pass's default-behavior hold on the absorb itself is lifted: Security: no workflow/secret/exfil surface. Watcher-internal markers only. CI: all required checks green on THIS HEAD (Behavior portable/serial, Herdr, Lint, Repo invariants, macOS Bash, coverage guard, Greptile). Still not land-eligible. File overlap is a coordinator hold, not waiting on the author:
Not waiting on the author. NM matches and the absorb is now opt-in. Do not rebase: overlap would still need a captain/coordinator exception. Not flagging Firstmate: the remaining hold is file overlap, not a product decision. Land-eligible: NO. Captain-flag NOW: no. |
2 participants
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.
Decisions accepted during the review of this rework, now part of the binding intent. A reviewer reading only the diff would not know these were chosen deliberately rather than missed:
What Changed
FM_TURNEND_CHURN_ABSORB_SECS, while preserving fail-closed surfacing for expired windows, unchanged panes, secondmates, status signals, and invalid or ambiguous state.Risk Assessment
✅ Low: Captain, the change is opt-in, fail-closed, bounded per endpoint, and backed by behavioral coverage for its material safety boundaries.
Testing
The focused watcher triage runner passed, and direct process output plus persisted queue and marker state demonstrated opt-in absorption, default-off surfacing without pane capture, and bounded resurfacing after deadline expiry.
Evidence: End-to-end watcher transcript
Source: End-to-end watcher transcript
Enabled: absorbed, no queue entry, churn window opened. Disabled: surfaced and queued without pane capture. Deadline spent: surfaced and queued, churn window cleared.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.
git status --shortand targeted branch diff inspectionbash tests/fm-watch-triage.test.shHermeticbin/fm-watch.shexecution usingtests/wake-helpers.shfor enabled, disabled, and expired churn-window scenarios✅ **Document** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.