Repository navigation
feat(bin): watcher-rung pipeline-state waits for no-mistakes spawns - #3979
NewAiCoder-bot wants to merge 17 commits into
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "no-mistakes(ci): Fixed the Greptile P2: ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
Contributing evidence for the Removing the poll loop and replacing it with a declared wait is the right change and the tests pin it well. One thing survives the rewrite that we think should not. The retained instruction rests on an inference the tool contradictsThe new text keeps:
The ten-minute cap is real, and we are not disputing it. What does not follow is "background that one call". The drive call does not sit in an unbounded hold, because it bounds itself for exactly that cap. Checked against the installed binary rather than recalled: and in the same help text:
The default is already below the cap the paragraph cites, and the documentation names that cap as the reason the default exists. So the retained sentence is a workaround for something the tool prevents by default, and it is the one place where the old premise carries into the new wording. The practical gap it leaves, which is the failure we hit"Background that one call [...] and resume from its result once it finishes" has no mechanism behind it. A backgrounded call returns in milliseconds, so it does not wait at all, and the worker still has to discover when the real work finished. The deterministic watch this patch adds is scoped, correctly and explicitly, to the between-rounds wait rather than to this call. That leaves the worker to invent its own way to learn the call returned, which is the polling this patch exists to remove. That is not hypothetical. A worker in our fleet followed the current instruction, and its runtime refused a bare foreground sleep and recommended backgrounding instead, so it could not wait in the foreground at all: every attempt to idle returned immediately and it polled again. Split at the first idle wait and at the cleanup, deduplicated by message id over the full session record:
Waiting was 2,649 of that session's 2,704 model calls and 1.205 of its 1.215 billion cache-read tokens: 99.2 percent of the session. The same pipeline finished green in 7 model calls once the worker simply let one bounded foreground call block. Measured from the session record, not estimated. Two scenarios for
|
|
Speaking as Kun's firstmate: First look on this tip vs main What this is
Gates
VISION.md (each rule)
Contract-class: new-default (always-on D5 arm + ship-lane DoD change). Never auto-merge. Not otherwise-ready — waiting on author, not the captain
Security: tip-only FYI, not a captain gate — Firstmate flag: no (CI / author blockers remain; do not escalate new-default until otherwise-ready). Merge-eligible: NO. |
e8c3e23 to
6c06878
Compare
NewAiCoder-bot
changed the title
|
Speaking as Kun's firstmate: Tip What this is (diff-inspected)
Gates
VISION.md (each rule)
Contract-class: new-default (always-on D5 nm-state arm + ship-lane DoD change; also carries #3970 repeat/edge always-on adapter surface). Never auto-merge. Otherwise ready except the default-behavior decision — with the captain, not the author. Do not merge/rebase from triage. If captain merges, close overlapping #3970 with thank-you naming this PR. Security: tip FYI only — Firstmate flag: yes (new-default otherwise-ready; prior author/CI blockers cleared). Merge-eligible: NO. |
76b4e83 to
d7500eb
Compare
NewAiCoder-bot
changed the title
|
Speaking as Kun's firstmate: Captain-decision hold — not waiting on the author. Tip moved on an existing waiting-captain new-default otherwise-ready hold; stamp updated only. Do not merge. Do not rebase from triage. Tip Tip delta (useful)
What this still is (unchanged class)
Gates
VISION.md (each rule)
Contract-class: new-default (always-on nm-state arm on every ship+no-mistakes spawn + DoD rewrite; also carries #3970 repeat/edge adapter surface). Never auto-merge. Otherwise ready except the default-behavior decision — with the captain, not the author. This is a captain-decision hold. Do not merge/rebase from triage. If captain merges, close overlapping #3970 with thank-you naming this PR. Security: tip FYI only — Firstmate flag: yes (tip changed on existing waiting-captain new-default otherwise-ready hold — parent will re-notify Firstmate). Merge-eligible: NO. |
6f3db31 to
1084cb9
Compare
NewAiCoder-bot
changed the title
…nt-when A `when` condition->action watch (`bin/fm-procevent-when.sh`) fired at most once: a successful fire was always a terminal outcome, and the runner retired the registration afterward. That is the right shape for a one-shot wait, but wrong for a source that must keep ringing every time a condition changes again - the only way to keep watching was to have some other agent notice the retirement and re-arm it by hand. - `fm-procevent-when.sh` gains `--repeat`: a successful fire releases the single-fire claim, journals the fire, and emits a non-terminal `repeat: continues` outcome, so the runner keeps the registration and restarts the poll instead of retiring it. The deadline is then measured from the last fire, so it means the condition stopped changing, not that the watch itself is old. - A new `silent` command makes that same successful repeat fire a routine no-op the runner records as handled without a wake, using the existing adapter seam, since the action has already done its job by the time the fire is journalled. Every other outcome, in both modes, stays terminal and still wakes with evidence. - `fm-procevent-when.sh` gains `--action-env NAME=VALUE`, recorded in the hash-bound spec (denying interpreter/loader-hijacking variable names), so an action can carry the environment it needs while the action executable itself stays argv[0] and its bytes stay trust-bound. tests/fm-procevent-when.test.sh covers arming with `--repeat`, the `repeat: continues` outcome, the `silent` no-wake path, the last-fire deadline reset, and `--action-env` validation and propagation. Fixes #3921
…GENTS.md, silent-command claim in verification doc
…event-when.sh cleared the fired marker on a successful fire and let the next reconcile's fresh `run` invocation fire again as soon as it saw a single true poll - even if the condition level had never actually gone false. That violates the documented "ring X every time Y changes" repeat semantics and causes duplicate action/task-notification rings on a condition that stays continuously true. Root-cause fix: added a small persisted per-source marker (`<sid>.needs-edge`) set whenever a repeat watch fires. A subsequent `run` invocation loads this marker and, while set, ignores true polls (treats them as non-counting) until it observes an actual false poll, at which point it clears the marker and resumes normal stable-true counting. The marker is cleaned up on retire and blocks re-arming under the same name if left behind, consistent with the existing `.fired`/`.fires` leftover checks. The existing "repeat watch rings again" test happened to remove-then-instantly-recreate the trigger file with no live poller ever actually running during the false window, so it was not proving a real edge; it now explicitly reconciles and waits during the false window so a poll genuinely observes it. Added a new regression test ("a repeat watch never refires on a level that never went false") that reproduces the exact reported bug - fails on the pre-fix code (verified by temporarily reverting the source fix) and passes after the fix, while also proving a genuine subsequent edge still re-arms the watch. Full suite (tests/fm-procevent-when.test.sh, 19 cases) passes; shellcheck is clean
…ch timing test; 4/4 local reruns pass)
…tch-stops-here path
…/when/ inventory for the edge-marker fix
…get to 30s Behavior portable serial N has flaked twice on 'the repeat watch never rang a second time' (once on serial 1, once on serial 3), while the same test passes reliably locally. This loop calls pe reconcile every 0.1s tick on top of the polling runner it waits on, making it more contention-sensitive under a loaded shared CI runner than the file's other passive wait_for_result/wait_for_file checks. Doubling its budget to 300 tries (30s) gives it the same headroom without slowing a healthy run, which still breaks out of the loop on the first successful poll.
…efire semantics and edge-marker failure path in the script's own --help contract
…ling A no-mistakes ship worker used to end its turn on a foreground `sleep 600` between `no-mistakes axi status` polls while a pipeline round ran. Every wake-up was a full model turn that re-read the whole context to learn nothing, and a worker that forgot the sleep just busy-polled instead. fm-spawn.sh now arms a per-task deterministic watch (bin/fm-nm-state-condition.sh) whenever it spawns a no-mistakes ship worker. The watch polls a snapshot of `no-mistakes axi status` outside any model turn and rings the task's steering inbox only on a real state change; fm-teardown.sh retires it. The worker-facing Definition of done (bin/fm-dod-lib.sh) is rewritten to match: end the turn on a declared `paused: no-mistakes run in progress, clears on its own` instead of polling, resume from the ring, and answer the parked gate with a short foreground call. A single drive call can still outlive what the calling harness lets one command run, so that guidance stays: background that one call and resume from its result, which is a harness-timeout workaround for one call, not the between-rounds wait the watch now covers. tests/fm-nm-state-condition.test.sh and tests/fm-dod-wait.test.sh cover the new condition script and the rewritten no-sleep/end-your-turn contract; tests/fm-brief.test.sh gains a scoped assertion that the generated no-mistakes brief names the watch and the declared-wait phrases, absent from scout briefs (which never run no-mistakes). Fixes #3922
Stacked on the fm-procevent-when.sh --repeat/--action-env feature merged into this branch: without repeat mode, the watch fm-spawn.sh arms for every no-mistakes ship rang once and then retired, so a worker would stall forever on its second and every later pipeline-state change. Arm it with --repeat --action-env "FM_HOME=$FM_HOME" instead, matching the shape tests/fm-procevent-when.test.sh's own end-to-end case already proves keeps ringing. tests/fm-nm-state-watch-arm.test.sh drives the real fm-spawn.sh for a ship+no-mistakes task and reads the watch's own on-disk spec to assert repeat=1 and the FM_HOME action-env are actually armed, rather than reading fm-spawn.sh's source; verified it fails without the --repeat/--action-env arguments and passes with them.
…rom the needs-edge gate
…changes; no edits needed
…ept the default `--stable 2` threshold, but a self-differencing condition reports each transition true only once (then rewrites its snapshot and reports false), so two consecutive true polls can only coincide by a timing accident — an `--edge` watch armed without an explicit `--stable 1` would stall past its deadline and report `never-true`, never firing. Root-cause fix in `bin/fm-procevent-when.sh` cmd_arm: arming now dies with a clear error if `--edge` is combined with any `--stable` other than 1 (covers both the implicit default and an explicit higher value), and the usage banner documents the requirement. Added two regression tests (`tests/fm-procevent-when.test.sh`) that arm `--edge` with the default stable count and with an explicit `--stable 2`, asserting the arm is refused; verified both fail on the pre-fix code (temporarily stashed the fix) and pass after it. Ran the full `fm-procevent-when.test.sh`, `fm-nm-state-watch-arm.test.sh`, and `fm-dod-wait.test.sh` suites — all pass, including the existing production caller in `bin/fm-spawn.sh` which already arms with `--stable 1 --edge` and is unaffected. shellcheck on the changed file shows no new warnings
…t backgrounding The retained 'Background that one call...' paragraph described a workaround from before no-mistakes axi run/respond grew --wait: it told every worker to background a single drive call and resume from its result, which is now a false premise (measured 2,649 waiting model calls / ~1.2B cache-read tokens versus 7 calls after one bounded foreground hold). --wait (default 8m0s, chosen to return before Claude Code's ten-minute command ceiling) already bounds the hold in-process; a worker just reattaches with the same drive call on an elapsed wait. Pinned in tests/fm-dod-wait.test.sh: the block must not mention backgrounding a drive call, and must name --wait's bounded default.
…cs completeness for edge marker
…DOD text (bin/fm-dod-lib.sh) instructs the worker to append a `resolved: run returned` status line without a `[at=<epoch>]` stamp, unlike every other status-line instruction in the brief. tests/fm-brief.test.sh's test_pause_verb_override_renders_all_brief_scaffolds extracts every status-signal template the brief instructs a worker to append and verifies each one carries a worker-written epoch stamp; this one didn't, so it failed with "ship:no-mistakes signal carries no worker-written stamp: resolved: run returned". Fix: added the missing `[at=<epoch>]` marker to the `resolved` line in bin/fm-dod-lib.sh (matching the format used by every other `resolved [at=<epoch>]: ...` instruction in the same file), updated the matching assertion in tests/fm-brief.test.sh (test_no_mistakes_dod_names_pipeline_state_watch) to expect the stamped form, and fixed the identical unstamped `resolved: run returned` instruction in the fm-spawn.sh ring message that fires when the pipeline-state watch triggers (same bug, no test covered it, but it's the same root cause). Verified locally: tests/fm-brief.test.sh, tests/fm-dod-wait.test.sh, and tests/fm-nm-state-watch-arm.test.sh all pass (exit 0, no failures), and bash -n syntax-checks clean on both edited shell scripts
a3513bf to
cb5f0b2
Compare
NewAiCoder-bot
changed the title
4 participants
Intent
Rebase PR #3979 (feat(procevent): watcher-rung pipeline-state waits for no-mistakes runs) onto the freshly rebased PR #3970 tip, resolving its CONFLICTING state; dropped three commits whose content was already redundant with #3970's rebased history, merged the --edge exemption feature with #3970's edge-marker-write-failure escalation fix
What Changed
bin/fm-nm-state-condition.sh, a deterministic condition script that projectsaxi status(status/outcome/step/round) into a snapshot and reports true only when that projection changes since the last poll, avoiding false fires from churn like elapsed-time fields.bin/fm-procevent-when.shwith repeat-watch support:--repeatkeeps a watch alive after a successful fire (ringing an action every time a condition changes rather than once),--edgeexempts self-differencing conditions from the generic false-then-true dedup (and is rejected unless combined with--stable 1, since an edge-detecting condition can never accumulate multiple consecutive true polls),--action-envpasses validated environment assignments to the action, and a newsilentsubcommand marks a repeat watch's successful fire as a handled no-op rather than a wake; also fixes a bug where the fired marker could let a still-true condition refire without an actual edge, and hardens the edge-marker write path to escalate on failure instead of silently retrying.bin/fm-spawn.sharms a repeating, edge-awarenm-state-<id>watch (via the new condition script) when spawning aship/no-mistakestask so the worker can end its turn on a declared wait instead of polling, andbin/fm-teardown.shretires that watch and cleans up its snapshot on teardown.bin/fm-dod-lib.sh's Definition of Done guidance to match the--wait-based drive-call flow (rather than backgrounding) and adds the[at=<epoch>]stamp to theresolved: run returnedstatus line, matching every other worker-written status-line instruction in the brief.bin/fm-test-run.sh's coverage-guardcomminvocations to forceLC_ALL=C, avoiding locale-dependent sort-order mismatches..agents/skills/process-event-sources/SKILL.md,AGENTS.md,docs/configuration.md,docs/scripts.md, anddocs/verification/process-event-sources.mdto document repeat/edge watch semantics, the action-environment option, and the new pipeline-state watch.tests/fm-nm-state-condition.test.sh(new),tests/fm-nm-state-watch-arm.test.sh(new),tests/fm-dod-wait.test.sh(new), plus expanded cases intests/fm-procevent-when.test.sh,tests/fm-brief.test.sh,tests/fm-pr-check-security.test.sh, andtests/fm-test-run.test.sh.Risk Assessment
✅ Low: The diff implements repeat/--edge watch semantics, a pipeline-state condition script, and D5 spawn/teardown wiring with extensive behavior-driven test coverage (executed watches, real fire journals, crash/failure-injection cases) and complete documentation; both findings from the prior review round (stale test variable, missing env-var doc) are verified fixed in the current tree, and manual tracing of the --edge/needs-edge state machine, the fired-claim release ordering, the env-assignment denylist (including the backslash-continued case pattern, verified not to introduce a matching bug), and the usage() header-extraction change turned up no new correctness issues.
Testing
Ran the five targeted test files that exercise every script this change touches, against the real fm-procevent-when.sh/fm-nm-state-condition.sh/fm-spawn.sh/fm-dod-lib.sh binaries (real processes polling real files, not mocked) - all 40+ cases passed, including the two review-round-1 fixes. No regressions found; one pre-existing test-infra gap (teardown's D5-watch retirement) noted as untested rather than guessed passing.
Evidence: fm-procevent-when.test.sh full run (27 cases)
Evidence: fm-nm-state-watch-arm.test.sh (drives real fm-spawn.sh, inspects on-disk watch spec)
Evidence: fm-nm-state-condition.test.sh (drives real fm-nm-state-condition.sh against a stubbed no-mistakes + real git worktree)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
tests/fm-procevent-when.test.sh:851- In tests/fm-procevent-when.test.sh, the "retire stops a repeat watch" block (around lines 851-859) reuses the stale$Hvariable left over from the immediately preceding edge-marker-write-failure block ($TMP_ROOT/h-repeat-edgefail) instead of resettingHback to$TMP_ROOT/h-repeat, which is where the watch namedrepeat(and$REPEATLOG) actually live.when "$H" retire repeattherefore runs against a home where norepeatregistration exists;fm-procevent.sh retireis idempotent/no-op for an unknown id, so it silently succeeds. Every assertion in the block (assert_absent "$H/state/procevent/when-repeat.source",assert_absent "$H/state/when/when-repeat.fires", and the finalassert_contains "$(count_lines "$REPEATLOG")" "$STOPPED") is then trivially true regardless of whether retire actually stops a repeat watch, because the block never reconciles or inspects the realh-repeathome again. The test passes and prints "retire stops a repeat watch" without exercising that behavior at all, and docs/verification/process-event-sources.md's "stops ringing once retired" coverage claim for this suite is consequently unproven. Fix is mechanical: addH="$TMP_ROOT/h-repeat"before this block (nonew_homere-init, since the home must be the one therepeatwatch is already armed in) so the retire call and its follow-up reconcile/sleep actually target the right registration and log.docs/configuration.md:1044-FM_WHEN_FIRES_JOURNAL_LINES(bin/fm-procevent-when.sh:199, default 200) is a new user-configurable environment variable - validated at run time the same wayFM_WHEN_OUTPUT_TAIL_BYTESis (a non-positive-integer value produces arejectedoutcome) - but it is not added to docs/configuration.md's env-var reference block, where its siblingFM_WHEN_OUTPUT_TAIL_BYTESis documented on the adjacent line (docs/configuration.md:1044). This is a small documentation-completeness gap in a commit range whose own stated theme includes doc completeness for this feature.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ No issues found.
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
a✅ No issues found.
bash tests/fm-nm-state-condition.test.sh- all 7 checks ok, exercising the real bin/fm-nm-state-condition.sh via its public CLI against a stubbed no-mistakes binary and a real git worktreebash tests/fm-nm-state-watch-arm.test.sh- drives the real bin/fm-spawn.sh end-to-end with a fake claude binary and inspects the real on-disk when-spec filebash tests/fm-procevent-when.test.shcase "--edge with the default stable count is refused at arm time" (full suite run, exit 0)repeatwatch is actually armed in) befo…bash tests/fm-dod-wait.test.sh- all 6 checks ok, calling the real fm_dod_block function from bin/fm-dod-lib.shstatus: rejected/ `detail: FM_WHEN_FIRES_JOURNAL…bash tests/fm-nm-state-condition.test.shbash tests/fm-dod-wait.test.shbash tests/fm-nm-state-watch-arm.test.shbash tests/fm-procevent-when.test.sh (full suite, 27 cases)Manual live CLI drive: fm-procevent-when.sh arm + fm-procevent.sh reconcile with FM_WHEN_FIRES_JOURNAL_LINES=0 against a real repeat watch✅ No issues found.
bash tests/fm-nm-state-watch-arm.test.shbash tests/fm-nm-state-condition.test.shbash tests/fm-procevent-when.test.shbash tests/fm-dod-wait.test.shbash tests/fm-brief.test.shgrep confirmation: bin/fm-dod-lib.sh:286 and bin/fm-spawn.sh:5004 both carryresolved [at=<epoch>]: run returnedgrep confirmation: docs/configuration.md documents FM_WHEN_FIRES_JOURNAL_LINES alongside FM_WHEN_OUTPUT_TAIL_BYTES✅ **Document** - passed
✅ No issues found.
✅ No issues found.
✅ No issues found.
🔧 **Lint** - 1 issue found → no changes applied ✅
🔧 No changes applied.
✅ Re-checked - no issues remain.
✅ No issues found.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.
✅ No issues found.