Repository navigation
feat(procevent): add repeat mode and action-env to the when watch adapter - #3970
NewAiCoder-bot wants to merge 11 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 P1: ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Speaking as Kun's firstmate: First look on main What I inspected
VISION.md (each rule)
contract-class: opt-in. Auto-merge eligible when CI + NM are green and review stays safe. Pairing spawn arm for #3921 stays a separate new-default decision. Security FYI (not waiting-captain): |
1cd3f28 to
51dd780
Compare
NewAiCoder-bot
changed the title
NewAiCoder-bot
changed the title
|
Speaking as Kun's firstmate: Tip Closes #3921 — verified link, partial delivery
Gates
Contract-class: opt-in (unchanged). New flags stay off unless an arm site passes them; this tip still does not stand up the nm-state watch by default. The always-on spawn arm + DoD rewrite remain on #3979 (new-default). VISION.md (each rule)
Overlap hold — do not merge this tip Security FYI (not a merge gate / not waiting-captain): Firstmate flag: no for this PR (opt-in; flag already raised on sibling #3979). Merge-eligible: NO (overlap hold). |
b5fc5ff to
eabd3b8
Compare
NewAiCoder-bot
changed the title
|
Speaking as Kun's firstmate: Overlap hold — not waiting on the author. Hold for #3979 captain decision / stack. Do not merge. Do not rebase from triage. Tip Tip delta
Closes #3921 — verified link, partial delivery
Gates
Contract-class: opt-in (unchanged). New flags stay off unless an arm site passes them; this tip still does not stand up the nm-state watch by default. The always-on spawn arm + DoD rewrite remain on #3979 (new-default). VISION.md (each rule)
Overlap hold — do not merge this tip Security FYI (not a merge gate / not waiting-captain): Firstmate flag: no for this PR (opt-in; flag already raised on sibling #3979). Merge-eligible: NO (overlap hold). |
4ddeb26 to
b1b612c
Compare
NewAiCoder-bot
changed the title
b1b612c to
6400d0b
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
… fields on republish
…e row to verification doc
…ead of fixed sleep
6400d0b to
1a95aa8
Compare
3 participants
Intent
Rebase PR #3970 (feat(procevent): add repeat mode and action-env to the when watch adapter) onto upstream main to resolve its CONFLICTING state; content unchanged except conflict resolution merging in upstream's since-merged rebind-all feature comments/tests
What Changed
bin/fm-procevent-when.sh armgains--repeat(keep a watch running after a successful fire, ringing the action again on each subsequent edge instead of stopping after one fire) and--action-env NAME=VALUE(repeatable environment assignments for the action, hash-bound in the spec and refused for names that could hijack an interpreter/loader such asPATH,LD_PRELOAD,PYTHONPATH); addssilentalongside the existingclassify/terminalsubcommands so the generic runner can tell a repeat watch's successful fire (recorded handled, no wake) apart from every other terminal outcome.<sid>.needs-edgemarker so a level that stays continuously true rings once and holds silent until an actual false poll clears the marker; fires are appended to a bounded per-source journal (<sid>.fires, default 200 lines) that also becomes the base for the deadline clock on repeat watches (measured from the last fire rather than from arming). A failed edge-marker write or journal write is escalated to a terminalfired-but-stopped outcome instead of silently continuing.rebind-all(viapublish_spec/rebind_one) now round-trips therepeatflag and action-env assignments when refreshing a watch's action hash, instead of dropping them;retirecleans up the new.fires/.needs-edgefiles alongside the existing spec/trust/fired records.AGENTS.md,docs/configuration.md,docs/scripts.md,docs/verification/process-event-sources.md, and the process-event-sources skill doc to describe repeat mode, the action-env flag, and the new silent/non-terminal fire outcome; expandedtests/fm-procevent-when.test.shwith coverage for repeat firing/edge semantics, action-env behavior, and rebind-all field preservation.Risk Assessment
✅ Low: The round-1 finding (publish_spec dropping repeat/env_argc/env assignments on rebind-all) is correctly fixed with field order matching cmd_arm exactly, and is covered by a genuine behavioral regression test that observes the action's actual output and the repeat: continues outcome; the rest of the branch (repeat mode, edge-marker fail-open escalation, action-env allowlist/denylist, doc updates, and the flaky-test fix waiting on real marker state) is internally consistent, matches its own documentation, and shows no new correctness or security issues.
Testing
Ran the full fm-procevent-when adapter test suite live (24/24 pass, including the dedicated regression test for this exact fix and the adjacent repeat-refire edge-marker tests that had flaked in earlier rounds - clean this run). Also independently drove the scenario by hand: armed a --repeat --action-env watch against a real in-repo action, rewrote the action's bytes to simulate a self-update, ran rebind-all, and diffed the spec file - repeat=1 and env_argc=1/FM_MARK=hello are identical before and after, while action_sha256 correctly changes to the new bytes' hash. No regressions found; worktree left clean.
Evidence: Full fm-procevent-when.test.sh run (24/24 passed)
Evidence: Manual live spec diff around rebind-all
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-procevent-when.sh:725- cmd_arm writes repeat=<0|1>, env_argc=<n>, and any --action-env assignments into the spec (bin/fm-procevent-when.sh:283-296), but publish_spec (bin/fm-procevent-when.sh:725-756), the function rebind_one uses to republish a watch's spec/trust after a self-update, only re-emits armed/interval/stable/deadline/condition_timeout/action_timeout/error_budget/action_sha256/condition_argc/action_argc/argv - it omits the repeat field, env_argc, and the environment assignments entirely. spec_load defaults a missing repeat field to 0 and a missing env_argc to 0 (documented at bin/fm-procevent-when.sh:354-357 as backward-compat for specs armed before these fields existed), so any watch armed with --repeat and/or --action-env that later goes through rebind-all (its documented purpose: refreshing an in-repo action's trust binding after a self-update, per the REPEAT-MODE-adjacent watch's own action executable living under FM_ROOT) has its spec silently rewritten as a plain one-shot watch with no action environment. The watch keeps running but permanently loses repeat behavior and its configured environment on the very next fire, with no error surfaced anywhere - a wrong-behavior-without-erroring regression. No test exercises rebind-all against a --repeat or --action-env watch, which is why this gap is unnoticed. Fix: publish_spec must also emit repeat=%s and env_argc=%s plus the ENV_ARGV assignments, exactly mirroring the block cmd_arm already writes, using the SPEC_REPEAT/ENV_ARGV values spec_load populated.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ No issues found.
✅ No issues found.
✅ No issues found.
✅ No issues found.
grep -rn '^ |^ $|^ 'across bin/fm-procevent-when.sh, tests/fm-procevent-when.test.sh, AGENTS.md, docs/configuration.md, docs/scripts.md, docs/verification/process-event-sources.md, .agents/skills/p…Manual live drive:bin/fm-procevent-when.sh arm rebind-repeat-env --interval 0.1 --stable 1 --repeat --action-env FM_TEST_MARK=keep --condition ... --action ...against a real FM_ROOT_OVERRIDE repoMutated the in-repo action file's bytes (v1→v2) to simulate a self-update, then ranbin/fm-procevent-when.sh rebind-allInspected the persisted spec file before and after rebind-all to confirm repeat=1, env_argc=1, and the ENV_ARGV assignment line survived republish alongside the refreshed action_sha256Ranbin/fm-procevent.sh reconcile, triggered the condition file, and waited for the action to fire and a result file to be capturedInspected the fired result file (status: fired, repeat: continues) and the action's own log output (v2 ran with FM_TEST_MARK=keep) to confirm both repeat-mode and the action environment carried through the rebindgrep for leftoverRetired the manually-armed watch and confirmed no leftover procevent/watchdog processes or temp directories remained after cleanupℹ️ The unrelated pre-existing test 'the repeat watch never rang a second time' (tests/fm-procevent-when.test.sh, a different code path than this fix - repeat-refire timing, not publish_spec/rebind-all) failed twice in a row on this host. It is self-documented in the test file's own comment as contention-sensitive under a loaded shared runner, and
uptimeshowed load average 5.40 on only 2 cores at the time. This is a flaky/infrastructure issue, not a regression introduced by review-1's fix - the two tests that actually cover publish_spec/rebind-all (including the newly added repeat/action-env preservation test) passed consistently across both full-suite runs.Live validation: ✅ go - 4 of 4 scenarios driven live against the product
bash tests/fm-procevent-when.test.sh (run twice, live CLI behavior suite for bin/fm-procevent-when.sh)Standalone extracted regression assertion run against git-worktree scratch checkout of pre-fix commit 3fbf49e68b579d687ae16a426affbfcc4d0bd220 (fails as expected)Standalone extracted regression assertion run against git-worktree scratch checkout of target commit b1b612ce5d74cad10559d0715d74439e31795c04 (passes)ℹ️ The pre-existing test assertion 'the repeat watch never rang a second time' (repeat-refire timing via reconcile polling, not the publish_spec/rebind-all path this round's fix touches) failed in all 3 consecutive full runs on this 2-core host. It is self-documented in the test file as contention-sensitive under a loaded shared runner and was already reported/dispositioned as flaky infrastructure (not a code regression) in round 2 of this same pipeline. Reporting again for visibility since it reproduced 3/3 this round too, but not attributing it to this change.
🚨 live validation verdict: no-go (5 of 5 scenarios were driven live against the product); failed: a repeat watch rings a second time after its first fire (adjacent repeat-mode behavior, unrelated code path)
Live validation: ❌ no-go - 5 of 5 scenarios driven live against the product
bash tests/fm-procevent-when.test.sh (run 3x)scenario: 'rebind-all preserves a watch's repeat flag and action environment across a self-update'scenario: 'rebind-all refreshes an in-repo watch's trust binding after a self-update and leaves an out-of-repo one alone'scenario: 'rebind-all matches FM_ROOT through a symlinked checkout path'scenario: 'rebind-all reaches a watch whose run process was already polling when the self-update landed'🔧 Fix applied.
✅ Re-checked - no issues remain.
bash tests/fm-procevent-when.test.sh (run 3x; all 24 cases passed each clean run)manual CLI drive: arm --repeat --action-env watch, rewrite action bytes, run rebind-all, diff spec before/after✅ No issues found.
bash tests/fm-procevent-when.test.sh (24/24 passed, full targeted suite for bin/fm-procevent-when.sh)manual live drive: bin/fm-procevent-when.sh arm manual-check --repeat --action-env FM_MARK=hello, then rebind-all after rewriting the in-repo action, diffing the published .spec file before/after✅ **Document** - passed
✅ No issues found.
✅ No issues found.
✅ No issues found.
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ No issues found.
✅ No issues found.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.
✅ No issues found.
✅ No issues found.