feat(babysit): gate a monitor loop on a zero-token probe, not a timer - #7634
Conversation
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @chenmingwei23 overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Verified the remaining surfaces (probe move, skill adapter, spec updates) are consistent with the description; the diff and description account for each other on both sides. Design-Verdict: CONCERNS Sound gating design with the right failure direction, but the scheduler's per-loop credit bookkeeping has accreted into a hand-rolled state machine with a documented defect record. Watch
Suggestions
[DESIGN-REVIEWED] a9417cc |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The gate itself is cause-level and earns its place; what doesn't is a REST What this change shipsIntent: stop paying a full agent turn for babysit cycles where nothing changed, by defaulting the existing zero-token probe into
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] a9417cc |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo blocking issues. FINDING — src/kiro_crew/mcp_tools/control.py:962 — for a gated loop armed with an explicit [OPUS-REVIEWED] a9417cc Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Round 1 dispositions -- head 244d337GPT BLOCKING, autonudge.py: "a quiet tick permanently loses its timer" -- FIXEDReal, and the worst possible shape for this PR: the first quiet observation would Fixed by advancing The more useful finding is why my tests missed it: they asserted only that no GPT FINDING, gh_pr.py:329
|
edfa891 to
244d337
Compare
|
Filed the pre-existing wake_on_green truthiness bug as #7644, as promised in the round-1 dispositions. Kept out of this diff on purpose: the expression is byte-identical on main, so folding it in here would grow a large diff with an unrelated fix. |
Round 2 dispositions -- head 2ca653cBoth blocking findings were real. One of them was introduced by round 1's own BLOCKING, autonudge.py: terminal verdicts re-arm the stopped loop -- FIXEDCorrect and self-inflicted. Fixed by re-arming only while the loop is still active AND still registered. The Added BLOCKING, autonudge.py: message updates leave the probe bound to the old PR -- FIXED, but not by the prescribed remedyThe finding is real. The prescribed remedy -- "remove automatic monitor attachment until message Semantics, all three pinned by tests:
Mutation-verified: with the retarget disabled the test fails with Note on the review pattern so farBoth rounds found the same class of defect -- a state transition where "cheap 285 assertions green across the touched suites; flake8, mypy and the repo black |
244d337 to
2ca653c
Compare
|
Self-audit push, head 69a14cc. Both review rounds so far found the same defect class -- a disagreement between the cheap tick and the loop's lifecycle -- so I went looking for the rest of it rather than waiting for round 3. One residue found and fixed: a terminal verdict deactivated the loop but left no outcome on the monitor record, so a finished watch was indistinguishable from a paused one and the generic resume path would have re-armed it onto an already-merged pull request (observe TERMINAL, deactivate, be revivable again). The finish is now recorded on the monitor as SUCCESS with a stopped_at, and the terminal test asserts that a generic save cannot revive it. 139 assertions green in the scheduler and monitor-persistence suites; flake8, mypy and black clean. |
2ca653c to
69a14cc
Compare
Round 3 dispositions -- head a6ef84dThree lanes, five concerns, all dispositioned. The Design concern about Opus BLOCKING (terminal tick re-arms forever) -- ALREADY FIXED, verdict pinned to an older SHASame defect GPT raised, and Opus's prescribed fix is what shipped: the re-arm is Design: gating is inferred from a mention, so an act-on-quiet loop is silently inverted -- FIXED, behaviour changedThis was right and I had missed it. My safety argument covered probe FAILURE and Inference cannot read that intent out of the wording, and guessing it would be Design + First Principles: phantom disclosure -- FIXEDBoth lanes caught the same thing independently, and they were correct: a comment First Principles:
|
69a14cc to
a6ef84d
Compare
ca565d4 to
0ac84d2
Compare
Round 44 -- head 823a3a6The body edit for the description mismatch re-rolled the codex lane on the same SHA, and BLOCKING: the terminal revalidation spent the tick's dedupe credit -- FIXEDRound 42 added a re-observation before settling, and I chose to RE-ASK rather than remember
A poll whose answer is thrown away must not be able to swallow a signal. The revalidation Mutation-verified: sharing the tick's key fails with "the discarded poll must not spend the Count, since it is the argumentFourteen findings have now landed in the terminal-delivery mechanism, and this is the Each fix has been small, correct, and has created the next finding's surface. I am still Everything else on the previous head was clean, including Opus, Design, First Principles |
Round 45 -- head 5980d4fAll five lanes reported clean on Opus: a merge landing ON the capping delivery was announced as an unmet goal -- FIXED
I verified the prescribed remedy, not just the finding, because dropping one guard is only GPT: the dashboard applier promised an every-interval re-injection for a gated loop -- FIXED
That also made a Description claim false: the body says the arming surface names the The applier now reads the cadence off the ARMED loop -- Black baselineFormatting the two touched files graduated them out of Both fixes mutation-verified separately: restoring the cap guard fails with "a finished Neither finding was in the terminal-delivery mechanism, so the fifth-landing rule I set for |
Round 46 -- head 999ea3fAll five lanes were clean on GPT: a terminal subject was still preempted, now by the wall-clock budget -- FIXEDRound 45 removed I did not add a third guard. The rule is now stated once, as a GPT: "PRs #42 and #7" saw only #42 -- FIXEDThe bare-form ambiguity check required the literal The prefix now covers a chain of Both mutation-verified: restoring the branch order fails with "a finished subject is not a Two process notes, since both cost me a step this roundThe first attempt at the notifier fix left the chain in two pieces -- a leading Formatting also produced two NEW black offenders this time rather than graduating baselined |
Round 47 -- head b735962, and this is the last change I intend to makeAll five lanes were clean on GPT's one advisory: the spec understated what the cap spends -- FIXED
Why this prose finding earned a push when the others did notI have been declining prose fixes since round 38, on the measured grounds that five I also swept for the same claim elsewhere before pushing: the tool description already says Timing was deliberate too -- I held this from the previous cycle so the push would not 89 assertions over the touched suites; the black gate is clean and the baseline still stands Where this leaves the PRNothing outstanding that I can act on. Every lane has been clean on a head, the CodeQL alert
One advisory stays open by choice, recorded rather than patched: |
Round 48 -- head e014cbaI said round 47 would be the last change. It is not, because GPT found something I could BLOCKING: re-arming a timer erased the claim the refusal had just re-owed -- FIXEDA refused fire re-owes its wake so the retried delivery still charges it. The refusal path The wake was undercounted and its follow-up turn lost, and both fixes that were supposed to Replacing a timer is now distinguished from cancelling a cycle: An over-reach I caught and withdrew, because it matters more than the fixMy first attempt deleted the claim drop from the cancel path entirely, reasoning that a So I withdrew the wider change and implemented the narrow one, which is what the finding And a test that could not fail, caught the same wayThe first version of the new test passed WITH the bug present, because Mutation-verified: making the re-arm drop claims fails with "the re-arm must not erase the This is the FOURTH finding in claim bookkeeping -- rounds 14, 26, 41 and now 48 -- which is |
Round 49 -- head e014cba unchanged. I am NOT fixing this one, and here is why.GPT reports a BLOCKING and it is real. I verified it before deciding anything:
So a busy-slot refusal leaves a PERSISTED allowance and a MEMORY-ONLY debt. Restart in Why I am declining to fix a BLOCKINGThe prescribed remedy is to persist the debt. That means two more durable per-loop fields, Adding durable state to that structure is not a fix, it is the sixth site. The remedy this The honest impact, so the decision is made on factsThe window is a restart between a refused fire and its retry. The effect is a metering For calibration: Opus dropped a neighbouring candidate on an earlier head as "a metering State of the PRDesign, First Principles and CodeQL are clean on this head; Opus is still running. Nothing
I am not merging this. |
|
/ai-review override gpt e014cba: Reachable but disproportionate, not a false positive. Trigger is a busy-slot refusal plus a gateway restart before the retry delivers; effect is one delivered turn missing from wakes/floor_ticks -- no crash, no lost user data, no removed guard, and it under-counts so it cannot overstate the saving. The correct fix is one durable owed-charge record replacing five hand-written claim sites, tracked as its own decision rather than a sixth patch. Maintainer ruling. |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
buluoray
left a comment
There was a problem hiding this comment.
Verdict: 0 blocking, 2 non-blocking. Reviewed at head e014cba1c9a54923a46d3fc2fa7ab65729f73835.
This is an independent substantive review focused on the crux: this changes when an autonomous monitor loop wakes a model, so cost and runaway behavior are what matter. I traced the changed path rather than trusting the description or the prior review rounds.
What I verified
- The gate is genuinely zero-token on the quiet path.
_monitor_tick_is_quiet(src/kiro_crew/autonudge.py) runsirq.poll(identity, target.message, probe)in an executor and returnsTruefor a QUIET verdict before_on_fireis reached, so no model turn is dispatched.irq.poll(src/kiro_crew/irq.py) runs the probe and maps Skip/Report/Done to aVerdict; the gh-pr probe (src/kiro_crew/probes/gh_pr.py) observes via one boundedghsubprocess throughgithub_runner.run_gh. No model call on any helper in that chain. - A quiet subject cannot stall the loop forever. QUIET increments
quiet_streak; at_MAX_QUIET_STREAK = 10the loop delivers anyway (_pending_floor_tick, returns False → fires) and resets the streak. Confirmed againsttest_a_long_quiet_streak_is_delivered_anyway, which drives ten real_timercalls and asserts the tenth delivers withfloor_ticks == 1,quiet_streak == 0,wakes == 0. - The other extreme (fire every interval) is bounded. WAKE/FALLBACK/floor/follow-up all fire, and each is a
deliveredturn that incrementscycle_countin_run_fire_cycle; only quiet ticks are free. FALLBACK during a probe outage fires every interval, which is exactly today's cost, not a new hot loop. max_cyclesis still enforced on the new path. The cap check atautonudge.py:1809(if loop.max_cycles and loop.cycle_count >= loop.max_cycles: deactivate) sits at the top of_timer, upstream of and untouched by the gate hunks. The quiet re-arm calls_arm_from_deadline→ schedules a fresh_timer, which re-enters that check. A permanently-quiet loop still advancescycle_countvia the floor every 10th tick, so the cap trips. Stop paths (sentinel file,notify_user_inputcancel,autonudge_stop) are preserved.- Probe failure fails safe. In-flight-marker write failure, executor exception,
irq.pollreturning FALLBACK (blind Skip), and a no-verdict return all resolve to firing one turn (logged once per tick), never to silence and never to a tight error loop. - The old timer guard was migrated, not orphaned. The
if loop.monitor is not None: … returnblocks in_timerand_run_fire_cycleare removed and replaced by the gate; monitor loops now flow through the gated fire path.pr_watch.pyis reduced to a 53-line cron adapter that keeps itswatch(ctx)entry point;irq.run(cron contract) is untouched. - The LLM-facing surface was updated in the same commit.
mcp_tools/control.pystates URL-only gating, thatmax_cyclesnow counts delivered turns, and thegate: falseopt-out; the babysitSKILL.mdand both specs (babysit-pr-watch.md,agent-interrupt-controller.md) move with the code. The tool description text matches the code semantics I traced. - New tests pin behavior, not source text. Sampled
test_a_quiet_probe_tick_dispatches_zero_turns,test_a_waking_probe_tick_dispatches_exactly_one_turn,test_a_long_quiet_streak_is_delivered_anyway, andtest_a_terminal_subject_stops_the_loop_without_a_turn: each constructs a real loop, monkeypatchesirq.pollto a fixedVerdict, drivesservice._timer(...), and asserts observable outcomes (fired,active,quiet_ticks,wakes,floor_ticks). Reverting the gate flips the waking test'sfired == [loop]to[]and removes the floor delivery, so they fail on revert. - All 68 CI checks are green at this SHA; Opus 4.8 Review is clean.
Non-blocking findings
-
dashboard/handlers/autonudge.py:144— thegaterequest field has no producer. Nowebsite/file is changed in this PR, so the goal popover still POSTs{slot_key, message, idle_secs, max_cycles}and never sendsgate. Consequence: none today — the route correctly defaults togate=False(ungated, the safe direction for a generic route) and rejects a non-boolean, so behavior is identical whether the field exists or not. Suggestion: this is fine to ship as forward-wiring, but consider dropping it until the popover sends it (matches the First Principles lane's subtraction), or add the popover producer in a follow-up. Non-blocking: unused input, no reachable defect. -
autonudge.py— per-loop transactional state is restored by hand at multiple sites. Six pieces of transitional state (poll_in_flight,terminal_pending,followup_ticks,quiet_streak,_pending_monitor_wake,_pending_floor_tick) are released/restored across the gate,update,_cancel_timerand_run_fire_cycle. This review's own history shows the failure mode is real — several rounds fixed a field missing from one restore set. Consequence is bounded in the safe direction (a miss double-charges or drops a turn; it does not silence a live watch), which is why I do not treat it as blocking. Suggestion: the consolidation into one transition object that both the Design lane and the author's own comments call for is the right follow-up; schedule it rather than leave it as a comment.
What I could not verify
- I did not re-run the suite or independently mutation-test. The mutation-verification claims per round are the author's; I confirmed statically that the sampled tests exercise the real
_timer/fire path and would fail on revert, but I did not execute them. - I did not verify live runtime behavior (the QA-in-a-pod counter samples the author reports). My review is static analysis of the diff plus the current-head bot verdicts.
- The GPT 5.6 lane on this SHA is a human override by the author, not a model pass — the recorded override reason is the disposition of record for
e014cba. I did not re-adjudicate that override; my own read of the changed path found nothing blocking independent of it.
Round 50 -- disposition for @buluoray's independent review, and what my rebase cost itFirst, the thing I owe you plainly: your review is now DISMISSED, and that is my doing. The conflict itself is worth reporting because of WHERE it landed: main has independently added Finding 2 (hand-restored transitional state) -- ADOPTED as the recommendation, scheduled not shippedYou are right, and your framing is the one I will use: schedule it, do not leave it as a comment. Finding 1 (the
|
buluoray
left a comment
There was a problem hiding this comment.
Verdict: 0 blocking, 2 non-blocking. Re-review after a rebase, at head fad0a3abc0d421cac61316503e89bde7b73e0c50.
An earlier approval by buluoray at e014cba was dismissed by the rebase. I re-traced the changed path at the current head rather than trusting that review or the description. The e014cba…fad0a3a compare is dominated by ~139 commits of unrelated main churn pulled in by the rebase; I reviewed the PR's own 27-file diff (gh pr diff), not that compare.
What I verified (re-confirmed at the current head)
- Zero-token quiet path.
_monitor_tick_is_quiet(src/kiro_crew/autonudge.py:2666) runsirq.poll(identity, target.message, probe)in a thread executor and returnsTruefor QUIET before_run_fire_cycleis reached, so no turn is dispatched. Traced the whole chain for a model call:irq.poll(irq.py:1059) only runs the probe and maps Skip/Report/Done to aVerdict; the gh-pr probe (probes/gh_pr.py) observes through_run_gh→github_runner.run_gh, one bounded subprocess. Noacp/provider/model import anywhere in that chain. max_cyclesis still the runaway backstop, upstream of the gate. The cap check sits at the top of_timer(autonudge.py:2587), before the gate call at:2666, and is untouched by the gate hunks. Every quiet re-arm goesnext_due_ts = … ; _arm_from_deadline(loop)(:2691) which schedules a fresh_timer, re-entering the check. A permanently-quiet loop still advancescycle_countvia the floor (see below), so the cap is reachable. The sentinel-file, runtime-budget and approval-stall stop paths remain above the gate.- No stall, and no hot loop. QUIET increments
quiet_streak; at_MAX_QUIET_STREAK = 10(autonudge.py:90,:2505) the loop delivers anyway (_pending_floor_tick, returnsFalse→ fires) and resets the streak — a quiet subject cannot stall forever. At the other extreme, WAKE/FALLBACK/floor/follow-up all returnFalseand each is a delivered turn that advancescycle_countin_run_fire_cycle; only quiet ticks are free. A FALLBACK during a probe outage fires once per interval — today's cost, not a new tight loop. - Probe failure fails safe. In-flight-marker write failure, executor exception, a blind
Skip(subject unreachable →Outcome.FALLBACK, deliberately NOT QUIET,irq.py:1099), and a no-verdict return all resolve to firing one turn, logged once per tick. Every uncertain path in_monitor_tick_is_quiet(no monitor,gate=False, no probe, targetNone/mismatch,poll_in_flight, retarget-mid-poll) returnsFalse. - Old timer path migrated, not orphaned. The legacy
if loop.monitor is not None: … returnguard is gone; monitor loops now flow through the gated fire path, and a no-monitor loop falls straight through to the unchanged legacy fire.pr_watch.pyis reduced to a cron adapter keeping itswatch(ctx)entry point; the cronirq.run()contract is untouched (irq.pollis added beside it, non-raising). - MCP tool description matches the code.
mcp_tools/control.py:246states URL-only gating (https://github.com/<owner>/<repo>/pull/<N>), that a quiet cycle costs no model turn, thatmax_cyclesthen counts delivered turns, the starvation floor, and thegate=falseopt-out. The text a model decides from matches the semantics I traced. - Tests are behavioral and would redden on revert.
test_a_quiet_probe_tick_dispatches_zero_turns,test_a_waking_probe_tick_dispatches_exactly_one_turn,test_a_long_quiet_streak_is_delivered_anyway,test_a_terminal_subject_stops_the_loop_without_a_turn,test_a_probe_that_raises_still_fires_rather_than_going_silent, andtest_a_verdict_is_discarded_when_the_loop_was_retargeted_mid_polleach construct a real loop, patchirq.pollto a fixedVerdict, driveservice._timer(...), and assert observable outcomes; reverting the gate flips the waking test's delivery to zero and removes the floor delivery. I confirmed these statically; I did not execute the suite (see below).
On the other reviewer's CHANGES_REQUESTED (bolichen97, review 5088312261 at 4d1f07b)
That review is a description ↔ code mismatch, not a code defect: it argued the PR body still described pre-narrowing behavior — gating "when its instruction names exactly one pull request" and a conditional host pin — while the code is URL-only inference with an unconditional pin. I verified the code at head: probes/targets.py:122 scans only _PR_URL, and :172-176 sets config["host"] = _PUBLIC_HOST unconditionally with the comment "Always pinned, because the only spelling that reaches here NAMED the host." The current head body already addresses this: §3 now states gating requires an explicit https://github.com/<owner>/<repo>/pull/<N> and that a bare owner/name#123 leaves the loop ungated; the host paragraph now says "pins its host, unconditionally"; and §4 cites test_a_shorthand_subject_is_refused_so_the_loop_stays_ungated — the exact test bolichen97 asked for, which exists at head. bolichen97's review predates the current body (it was submitted against 4d1f07b), so their state has simply not caught up. Their required change looks satisfied at this head. I am not resolving, dismissing, or replying to their review — flagging for the humans that a re-review by them is what clears the remaining CHANGES_REQUESTED block.
Non-blocking findings
dashboard/handlers/autonudge.py— thegaterequest field has no frontend producer. Nowebsite/file is in this PR, so the goal popover never sendsgate. Consequence: none today — the route defaults togate=falseon that generic path (the safe, ungated direction) and rejects a non-boolean, so behavior is identical whether the field is sent or not. Suggestion: ship as forward-wiring, or add the popover producer in a follow-up. Non-blocking: unused input, no reachable defect.autonudge.py— per-loop transitional state is released/restored by hand at many sites (poll_in_flight,terminal_pending,followup_ticks,quiet_streak,_pending_monitor_wake,_pending_floor_tick), across the gate,update,_cancel_timerand_run_fire_cycle. The review history shows this failure mode is real (rounds fixed a field missing from one restore set). It is bounded in the safe direction — a miss double-charges or drops a turn, it does not silence a live watch — which is why it is not blocking. Suggestion: the consolidation into one transition object that the author's own comments already call for is the right follow-up; schedule it.
What I could not verify
- I did not execute the suite or independently mutation-test; I confirmed statically that the sampled tests drive the real
_timer/fire path and would fail on revert. The per-round mutation-verification claims are the author's. - CI at this head is still in progress (24 checks running: backend/frontend tests, builds, and the Opus/GPT/Design/First-Principles bots); zero checks are failing at the moment I write this, but I cannot confirm the description's "784 assertions … all green" until the backend lanes complete. bolichen97's earlier review (
b2ba60f9) cited failing Windows shards; those lanes are re-running on this head and not yet complete. - I did not verify live runtime behavior or the QA counter samples the author reports; this is static analysis of the diff plus current-head signals.
A babysit loop spent a full agent turn on every cycle, including the cycles whose only finding was that nothing had changed. The cheap detector for that already existed -- the irq kernel and its gh-pr probe, both shipped -- but it was reachable only as a script cron the agent had to arm by hand, and the measured arm count was zero. The default path was never touched, so five merged steps of this line moved no spend. The gate now lives on the default path. Every armed loop whose own instruction names one pull request becomes a monitor, the tick observes the subject with one bounded gh call, and only an unexpected observation spends a turn. There is no parameter to pass and no call site to update, because a parameter is an opt-in and an opt-in is what measured zero. Failure resolves toward spending, never toward silence: an un-inferable target, an unknown subject kind, a probe defect and a kernel that reaches no verdict all fall through to the timer the loop had before. Each wake also buys one follow-up turn, because the probe watches the subject and cannot see an agent that was woken and has not pushed yet.
Round 51 -- both reviewers passed on their own, and I spent the push on two wrong commentsOn GPT's two non-blocking findings were both wrong statements in comments, and I pushed for them. That
|
Round 52 -- FIFTEENTH finding in the same mechanism. Verified real, deliberately NOT fixed here.GPT's finding is REAL and I am not going to patch it. Both halves of that sentence are load-bearing, The premise holds -- I read the ordering rather than taking itIn
It is also slightly worse than the finding says. The user does not merely lose a turn -- they get Why it is not fixed in this PRThis is the FIFTEENTH finding in the channel terminal-delivery mechanism (rounds 15-19, 25, 31, The prescribed fix -- "process But "the cap may be overridden by an outstanding terminal debt" is a change to the meaning of Decision (a) in the body gates this: split the channel terminal delivery into its own PR. Fifteen What that leaves on the PROpus 4.8 and CodeQL are green on this head; Design and First Principles were green on it before the I am not merging this, and I am not overriding this one either: unlike the round-49 finding Raymond |
Round 53 -- two reviewers read the SAME ordering oppositely, which settles what round 52 is@buluoray re-reviewed at The reason I am posting this separately is that his review contains something that decides the He verified, as a PRESERVED SAFETY PROPERTY:
GPT, on the byte-identical logic one head later, called that exact ordering the defect:
Both descriptions of the ordering are correct. They disagree about whether it is right. One reviewer This is the clearest argument yet for decision (a), split the channel terminal delivery into its own Two smaller things from his review, for the record:
He notes he could not confirm the description's assertion count because backend lanes were still in |
|
/ai-review override gpt a9417cc: Accepted KNOWN defect, not a false positive and not disproportionate. I verified the ordering in _timer and the finding is exactly right: a refused terminal turn plus a spent cap or budget reports stopped_reason=cycle_cap for a subject that merged. Overridden because the remedy decides whether an owed terminal turn outranks the runaway cap, which needs its own change and tests. Tracked as #8060, with the fifteen-finding history of this mechanism. Maintainer ruling. |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
buluoray
left a comment
There was a problem hiding this comment.
Re-review at head a9417cca687205421882307fb9ccc57e7fcf464f.
My prior approval at fad0a3abc was dismissed by a subsequent push (main enforces require_last_push_approval). This re-review establishes what actually changed since that approved head and re-confirms the properties that carried it.
The delta since the approved head is comment-only
Both fad0a3abc and a9417cca6 share the identical parent 48c393d5, so there is no main movement between them and no rebase noise — the two commits are siblings on the same base. The exact incremental diff (git diff fad0a3abc a9417cca6) is therefore the whole change since approval, and it is:
src/kiro_crew/autonudge.py+1/-4src/kiro_crew/monitoring/models.py+4/-1
Every added/removed line is a # comment or #: docstring line — a filter for non-comment changed lines returns zero. No executable line changed. The large +5747/-782 headline is the already-reviewed feature; none of it moved since fad0a3abc.
The two edits are documentation reconciliations, and both now match the shipped code:
autonudge.py(~L662): removed a stale sentence that contradicted the retained block. The code still normalizes a present-but-non-bool /nullgatetoFalse(ungated) and logs it — the fail-safe direction, since an unreadable value costs a turn rather than silently deactivating a task. Comment now agrees with the code.monitoring/models.py(~L226): tightened the counter docstring to saywakescounts turns actually DELIVERED (not every non-quiet tick), with undecidable gates charged togate_fallbacksand refused fires to neither. Fieldsquiet_ticks,wakes,gate_fallbacks,floor_ticksall exist and are counted apart as described.
Established properties re-confirmed at this head
Because the executable code is byte-identical to the approved head, the five properties hold by construction; spot-checked directly:
- Zero-token quiet path: the probe runs
ghas a subprocess offloaded to a thread (25s timeout), no model call. - Fail-safe direction:
_monitor_tick_is_quietreturnsTrue(skip) only on a definite quiet verdict; no-monitor, no-probe, un-inferable target, probe defect, no-kernel-verdict, and target drift all returnFalseand fire as today. - Default / opt-out path unchanged:
if not loop.gate: return Falsefires exactly as before, so an ungated loop is unaffected. - Starvation bound:
_MAX_QUIET_STREAK = 10floor still delivers on a quiet streak. max_cyclesbackstop remains upstream of the gate and reachable from re-arm paths.- Test files are not in the delta (byte-identical) and all Backend Tests shards (Linux, Windows, namespace sandbox) are green at this head, so the gate's guards would still redden on revert.
Review lanes at this head (by comment body, current SHA)
- Opus 4.8: no blocking findings.
- GPT 5.6: human override accepted by the maintainer (
#8060). The overridden finding is a genuine but narrow known defect — a refused terminal turn coinciding with a spent cap reportsstopped_reason=cycle_capfor a subject that merged; the loop still stops and delivers its terminal notice, only the recorded reason label is wrong in that race. Accepted-and-deferred with a tracking issue; not a crash, data loss, or security hole. Non-blocking. - Design Review: CONCERNS (advisory, self-declared). Per-loop credit bookkeeping is a hand-rolled state machine the author wants reified into one transition object. State-machine hygiene / refactor suggestion — not blocking.
- First Principles: CONCERNS (advisory). Points at a dead REST
gatefield with zero producers,Target.host_key/same_hostwhose triggering case is unreachable, and stale description prose. Dead-code and documentation — not blocking.
None of the four lanes carries a BLOCK on this head.
bolichen97 CHANGES_REQUESTED
bolichen97's latest review (4d1f07b, CHANGES_REQUESTED) is still formally outstanding — it was not superseded by a later bolichen97 review. Its substance is a Description/code mismatch (the body still describes pre-narrowing gating on "names a pull request" and a conditional host pin, while the code narrowed to explicit-URL-only inference with an unconditional pin). That is a PR-body doc concern, not a code defect on the changed path, and clearing it is the author's/reviewer's to do — I am not clearing it here. The earlier bolichen97 CHANGES_REQUESTED (b2ba60f9, "all green" vs failing Windows shards) named the missing gate kwarg on the test double; that is fixed and all shards are green at this head.
Verdict
Blocking: 0. Non-blocking still-live: the two advisory CONCERNS above and the accepted known defect #8060, none introduced by the delta since approval. The incremental change since my dismissed approval is comment-only and correct, and re-approving satisfies the last-push-approval requirement without re-litigating the already-reviewed feature.
Round 54 -- a second description/code mismatch, same class as @bolichen97's, found in the body's own correction@bolichen97 Your While re-checking that, I found a SECOND mismatch of the same class -- in a sentence the body had
That is wrong, and it understates the cap. The cap is charged where a turn is CONFIRMED The body now states delivered turns and enumerates all four consumers, and notes that wall-clock is I also added the upgrade-safety statement the body never made, because "what does this do to my
That is verified two ways: No code changed and no push -- this is a body edit on the same head |
Round 55 -- Opus's ack finding is real, filed as #8100, deliberately not pushed hereAll five lanes are green on Opus passed as a check but its body carries a real finding, and I verified it in source rather than Filed as #8100 with the one-line remedy and a Why not fixed here, when round 51 pushed for exactly this classRound 51 pushed for two wrong statements in shipped comments, so declining now would look like a If a push becomes necessary here for any other reason, this fix goes in that push. |
1. What is the problem?
A babysit loop spends a full agent turn on every cycle, including the cycles
whose only finding is that nothing changed. On a long watch that is most of
them: a PR sits in CI for ten minutes and in review for longer, and each
uneventful tick re-sends the session's whole accumulated context to the model
to learn "still pending".
The cheap detector for exactly this already ships.
kiro_crew.irqis theinterrupt kernel (dedupe, coalescing, epoch reset, error backstop) and the
gh-pr probe observes one pull request with a single bounded
ghcall. But theonly way to reach them was a script cron the agent had to arm by hand, per PR,
from the owning session. The decision to arm it sat in a skill's decision
table, evaluated by an agent that was mid-fix, for a saving it could not see.
The default path --
monitor_start-- was never changed.2. Why this issue matters to the user
The saving was built and then not delivered. Measured on this installation,
loop-owning dashboard sessions accounted for the large majority of a day's
credits, while the zero-token watch had zero armed instances and its last
real tick was over a day old. Several loops were self-declared pure-wait --
their own armed instruction reads "if not fixed: stay silent" -- and were still
paying full price to confirm that nothing had moved.
There is a second cost that matters more than tokens. A capability with no
adoption signal is indistinguishable from a capability in use, so nothing
surfaced the zero. That is why this change ships its own counters.
3. How our fix solves it
Chained from the symptom:
spending one.
driver, and the kernel's verdict is raised as a
cron_scriptexception.builtin_skills/kirocrew-dev/babysit/scripts/, a hyphenated path that is notimportable, so the scheduler could not use it at all.
So the fix is in two separable parts.
Part A -- mechanical, no behaviour change. The gh-pr probe moves to
kiro_crew.probes.gh_pr. The skill'spr_watch.pybecomes a 53-line cronadapter over it and keeps its registered
watch(ctx)entry point. The twomonkeypatchtargets in its suite move with the gh chokepoint, because patchingthe driver would have faked nothing and quietly run the real subprocess in 27
cases.
irq.poll()is added as a non-raising front door besiderun(), which isuntouched; the kernel was already driver-agnostic (its own suite drives it with a
two-field
SimpleNamespace) and this makes that a named, tested entry point.Part B -- the behaviour change.
AutoNudgeServicenow decides per tickwhether to spend a turn:
explicit public URL --
https://github.com/<owner>/<repo>/pull/<N>. Nothingelse selects a subject: a bare
owner/name#123leaves the loop ungated.A second, DIFFERENT pull request named anywhere in the instruction -- as
owner/name#42or asPR #42-- refuses inference entirely, because the loop'sown subject and a blocker it merely mentions cannot be told apart, and gating on
the wrong one retires the loop while its real work is unfinished.
Inference happens where the loop is constructed, so there is no new
parameter and no call-site change -- the MCP tool, the dashboard and the
channels all inherit it. A parameter would have been another opt-in, and an
opt-in is precisely what measured zero.
QUIETre-arms without touching the fire callback: one boundedghcall, nomodel turn.
WAKEfires.TERMINALstops the loop and emitsexpiredso theuser is told rather than noticing silence -- and only a MERGED subject is
recorded as a success. One closed without merging ended on a question (reopen
or abandon), so it is recorded as blocked; reporting success there would tell
the user "no action needed" about the outcome that needs them most. The
classification reads the probe's own observation keys, not its prose.
between them, one durable write, and
active = Falsein place of a deactivationcall whose timer cancel would reach the very task doing the work. A failed write
undoes the marks and fires, so a disk error cannot leave a dead watch that looks
like a calm one.
probe defect, a kernel that reaches no verdict, and a probe that could not reach
its subject at all. That last one is why a failed fetch is not QUIET: an
unobserved subject has not been shown unchanged, and calling it quiet is how an
expired credential becomes an indefinitely silent watch. A wrongly-quiet tick is
silence with half-finished work behind it; a wrongly-spent tick costs what
today already costs.
agent that was woken and has not pushed yet is invisible to it, and a pure
gate would starve the work it just started. The allowance is bounded at one so
it cannot quietly disable gating.
observations the loop is delivered anyway and the streak resets. The gate can
only see the subject, so a loop whose duty is to act while the subject is
quiet -- refresh a heartbeat file every cycle, chase a reviewer who has not
replied, keep a branch rebased on a moving base -- produces nothing for the
probe to observe and a pure gate would never deliver it again. Inference cannot
read that intent out of the wording, and guessing it is worse than bounding it.
Nine ticks in ten still cost nothing.
both wrong. A paused current-version monitor is now resumable through the
generic update path -- it used to be refused, which for a gated loop meant a
watch that could be paused and never resumed. And editing the instruction
now retargets the watch: same subject keeps the monitor and its counters, a
different subject rebinds it, no named subject clears it and the loop reverts
to the plain timer. Without this an edited loop kept observing the PR it was
no longer about.
quiet_ticks,wakes,gate_fallbacksandfloor_ticksare recorded permonitor. Fallbacks are counted apart from wakes so a permanently broken gate
cannot read as a busy, well-used watch, and floor deliveries are counted apart
again so a periodic delivery is never reported as a real signal. A wake is
charged where delivery is CONFIRMED, not where it was observed: a refused fire
(busy slot, callback error, a loop deactivated mid-flight) must not report a
turn that never ran, and a terminal observation delivers no turn at all so it
is not a wake either. The claim is transient, so a restart forgets it rather
than inventing a turn, and it is released if the cycle is cancelled -- otherwise
the loop's next delivered fire would inherit it.
versionthis gateway does not implement is never ARMED -- its stored
activeintent isleft untouched, because that intent belongs to the newer gateway that wrote it
and must survive a downgrade, so inertness has to come from the arm refusing
rather than from rewriting the record. And a verdict is discarded if the loop
was RETARGETED while the probe was in flight: the poll runs
ghin a threadfor up to 25 seconds, so an observation of the old subject must not be applied
to the new one.
from a URL that named it. It addresses the subject as a bare
owner/nameslugand never passes
--hostname, whileGH_HOSTis forwarded from the ambientenvironment -- so on a machine configured for an enterprise host the same slug
resolves to a different repository, where a same-numbered pull request could be
merged and stop a watch on a live one. Requiring the URL is what makes the pin
unconditional: there is no inferred-but-unpinned subject, because a spelling
that carries no host does not select a subject at all.
The key accepts ONLY that one value -- it pins the public host, it does not
choose a host, because this module's rule is that an enterprise host comes from
the operator's own gh configuration and never from data.
and states that the message re-injects only when it changes, instead of
promising a plain re-injection every interval -- which for a gated loop is
untrue. The ack calls the scheduler's OWN decision function rather than
re-deriving the answer from the target, because a subject can infer cleanly and
still fail to form a valid monitor: one function, one answer, since a
disclosure that can be wrong is worse than none. This PR exists because a change
nobody could see had no effect; shipping the gate invisibly would have repeated
that.
gate: falseis the escape, and it is an opt-OUT. The measured failure thischange answers is that five consecutive OPT-INS produced zero adoption, because
a default nobody changes never moves. An opt-out has the opposite shape: the
default gates every surface, and the flag only releases the minority of loops
whose duty is to act while the subject is quiet, which an observation of that
subject cannot see. It is threaded through the tool, the directive, the shared
authz chokepoint and
add(), so the dashboard and channel paths get the sameescape rather than only the surface that happens to show a description; ABSENT
means gated, so a caller written before the flag keeps the default. The decision
is PERSISTED on the loop, because the instruction is the target: without a
remembered answer, editing the wording would re-infer a subject and silently
re-gate a loop whose caller had opted out -- through the documented way to revise
a loop, and invisibly, since an ungated loop and a re-gated one look identical
until the turns stop arriving.
Its watch-mode section now opens by saying you probably do not need it and names
only what the gate cannot reach -- an enterprise host, detection with no owning
loop, or the cron's own
known_reds/note/wake_on_greenknobs. It alsoclaimed a nudge cycle "pays a full agent turn", which is now false for exactly
the case that section is about, and two watchers on one pull request would have
woken separately for the same event.
Two follow-on notes.
max_cyclesnow bounds DELIVERED TURNS rather than ticksfor a gated loop -- and a delivered turn is not the same as a wake: a follow-up
turn, a streak-floor turn and a gate-fallback turn each consume one too, because
the cap is charged where a turn is confirmed delivered. A watch can therefore sit
on a pull request for days inside a small cap, which is the intended reading but
is a real change in what the number means -- so
monitor_start's own description now states it, along with gating itself and theone wording decision that controls it, since the agent composing the instruction
is the only party who can keep an act-on-quiet loop on the plain timer. Wall-clock
is bounded separately by
max_runtime_secs, which is unchanged. And thekernel's brief is not yet threaded into the woken turn's prompt: the woken agent
re-reads the PR exactly as it does today, so this PR's saving is entirely in the
quiet ticks. All are called out rather than left to be discovered.
What changes for a loop that is already running: nothing.
gatedefaults tofalse on the dataclass, on
add(), on the authorizer and onPOST /api/autonudge;only
monitor_start's own directive defaults it true. A record persisted beforethis change carries a monitor dict with no
gatekey, so it decodes to false andthe tick's own guard keeps that loop on the plain timer -- an upgrade re-gates
nothing. The behaviour change reaches a loop only when
monitor_startarms a NEWone whose instruction carries an explicit pull-request URL.
4. What tests we did
784 assertions across sixteen suites, all green, plus flake8, isort, the repo's
black gate and mypy on the touched files. Every new assertion above was
mutation-verified: the property was broken in the source, the test was watched to
fail on the specific claim, and the source restored. That step is what this PR
learned the hard way -- the first review round found a defect whose test passed
because it asserted a presence ("no turn was spent") while the bug lived in an
absence ("no timer was armed"), and a dead watch spends nothing either.
New behavioural pins, each written so it fails if the property is lost:
by the probe iterator not advancing on the bypassed tick
with the streak reset and the delivery counted as a floor tick, not a wake
retargets the watch, rebinds it, or clears it -- asserted on the absence of a
stale binding, not only on the presence of a new one
survives untouched
watch unsettled
kernel has already deduped the observation it was carrying -- without the retry
the next tick would call an unchanged subject quiet and the real wake would be
lost until the floor; and a cancelled cycle leaves no claim for a later fire to
inherit
a probe that could not reach its subject is never reported as quiet
github.comon the gh call, unconditionally, since onlya URL selects a subject; a bare
owner/name#123is refused and the loop staysungated (
test_a_shorthand_subject_is_refused_so_the_loop_stays_ungated); anunpinned config keeps today's resolution, and any other host -- malformed or a
plausible enterprise name -- is refused
an ungated one keeps it; an ambiguous instruction is reported as ungated
gate: falseis reported as ungated AND arms an ungated loop -- assertedseparately, because a flag that only changed the ack text would tell an
act-on-quiet loop it was exempt and then slow it anyway
restart, asserted on the absence of a monitor the retarget would have attached
gating the blocker would silence the PR the loop owns
autonudge.py#1751) is not a targetfuture-version record keeps the active intent its own gateway wrote
Three ratchets that pinned the previous deliberately-inert contract were
rewritten rather than deleted, and their names said
unwired. Two of my ownover-broad edits were caught by the monitor-persistence suite and are fixed: a
settled monitor must not re-arm on load, and a future-version monitor's active
intent must survive a generic save.
5. Any other suggestions on the work
babysit-pr-watch.mdhadsaid
monitor_startre-injects per interval and located the probe underbuiltin_skills/; it now describes the gated default, the opt-out, whatmax_cyclesbounds once gated, the starvation floor, and the narrowed role leftfor the manual cron.
agent-interrupt-controller.mdstill owned the probe atthe skill path and never mentioned
irq.pollat all -- a new public kernel entrypoint absent from the kernel's own spec; it now documents both drivers, the
Verdictoutcomes, whyFALLBACKis the failure direction, and why thekernel's bounds are not forwarded through
poll.gate: falseships with no producer yet, and that is deliberate rather than anoversight. The same standard removed
irq.poll(**bounds)and narrowed thehostkey in this PR, so the difference is worth naming: those two generalisedahead of any NAMED harm, while this one answers a named, reachable one -- a loop
whose duty is to act while its subject is quiet is invisible to an observation
of that subject, and the floor bounds that harm without removing it. An opt-out
also inverts the adoption argument this PR is built on: an opt-IN measured zero
because the default never moved, whereas an opt-out with few callers is what
success looks like.
increment: it is already computed and currently discarded.
agent turns while real loops run dozens of cycles, so enforcing it here would
have stopped working watches early -- a regression wearing a budget's clothes.
one branch in
probes.build, with no scheduler change.pr_watch.pyis a thin adapter over the installed package, so astale copy in
crons/paired with a newer gateway is the one combination thatcan fail to import. The arm recipe already says to re-copy on every arm.
path has almost nothing left of its own --
known_reds,noteandwake_on_green-- and measured zero armed instances even before this PR. It isa deletion candidate at that point, deliberately not bundled here.