Skip to content

feat(babysit): gate a monitor loop on a zero-token probe, not a timer - #7634

Merged
iamwhatever merged 1 commit into
mainfrom
feat/irq-gated-monitor-loop
Sep 3, 2026
Merged

feat(babysit): gate a monitor loop on a zero-token probe, not a timer#7634
iamwhatever merged 1 commit into
mainfrom
feat/irq-gated-monitor-loop

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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.irq is the
interrupt kernel (dedupe, coalescing, epoch reset, error backstop) and the
gh-pr probe observes one pull request with a single bounded gh call. But the
only 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:

  • Symptom: quiet cycles cost a full turn.
  • Because: the tick had no way to ask "did anything change?" before
    spending one.
  • Because: the component that can answer was reachable only from a cron
    driver, and the kernel's verdict is raised as a cron_script exception.
  • Because: the probe itself lived under
    builtin_skills/kirocrew-dev/babysit/scripts/, a hyphenated path that is not
    importable, 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's pr_watch.py becomes a 53-line cron
adapter over it and keeps its registered watch(ctx) entry point. The two
monkeypatch targets in its suite move with the gh chokepoint, because patching
the driver would have faked nothing and quietly run the real subprocess in 27
cases. irq.poll() is added as a non-raising front door beside run(), which is
untouched; 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. AutoNudgeService now decides per tick
whether to spend a turn:

  • A loop is gated when its own instruction names exactly one pull request by
    explicit public URL -- https://github.com/<owner>/<repo>/pull/<N>. Nothing
    else selects a subject: a bare owner/name#123 leaves the loop ungated.
    A second, DIFFERENT pull request named anywhere in the instruction -- as
    owner/name#42 or as PR #42 -- refuses inference entirely, because the loop's
    own 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.
  • QUIET re-arms without touching the fire callback: one bounded gh call, no
    model turn. WAKE fires. TERMINAL stops the loop and emits expired so the
    user 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.
  • The terminal transition is committed ONCE: marks in memory with no await
    between them, one durable write, and active = False in place of a deactivation
    call 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.
  • Every uncertain path fires: no inferable target, an unknown subject kind, a
    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.
  • Each wake buys one follow-up turn. The probe watches the subject, so an
    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.
  • No loop can be starved, only slowed. After ten consecutive quiet
    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.
  • Two previously pinned refusals are deliberately flipped, because the gate makes
    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_fallbacks and floor_ticks are recorded per
    monitor. 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.
  • Two records are refused outright rather than gated. A monitor whose version
    this gateway does not implement is never ARMED -- its stored active intent is
    left 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 gh in a thread
    for up to 25 seconds, so an observation of the old subject must not be applied
    to the new one.
  • The probe pins its host, unconditionally, because every inferred subject came
    from a URL that named it. It addresses the subject as a bare owner/name slug
    and never passes --hostname, while GH_HOST is forwarded from the ambient
    environment -- 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.
  • The arming surface says so. A gated loop's acknowledgement names the subject
    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: false is the escape, and it is an opt-OUT. The measured failure this
    change 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 same
    escape 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.
  • The babysit skill no longer teaches the manual cron for the case this covers.
    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_green knobs. It also
    claimed 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_cycles now bounds DELIVERED TURNS rather than ticks
for 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 the
one 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 the
kernel'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. gate defaults to
false on the dataclass, on add(), on the authorizer and on POST /api/autonudge;
only monitor_start's own directive defaults it true. A record persisted before
this change carries a monitor dict with no gate key, so it decodes to false and
the 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_start arms a NEW
one 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:

  • a quiet tick dispatches zero turns and leaves the loop active
  • a waking tick dispatches exactly one
  • an unknown subject kind, and a probe that raises, both still fire
  • a terminal subject stops the loop without a turn
  • a wake buys exactly one follow-up turn, and gating resumes after it -- proven
    by the probe iterator not advancing on the bypassed tick
  • nine consecutive quiet ticks spend nothing and the tenth is delivered anyway,
    with the streak reset and the delivery counted as a floor tick, not a wake
  • a paused current-version monitor is resumable, and an edited instruction
    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
  • an unsupported monitor version gets NO timer, while its stored active intent
    survives untouched
  • a verdict is discarded when the loop was retargeted mid-poll, leaving the new
    watch unsettled
  • a refused fire charges no wake but DOES buy one gate-free retry, because the
    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 failed terminal write keeps the watch alive and fires instead of raising, and
    a probe that could not reach its subject is never reported as quiet
  • an inferred subject pins github.com on the gh call, unconditionally, since only
    a URL selects a subject; a bare owner/name#123 is refused and the loop stays
    ungated (test_a_shorthand_subject_is_refused_so_the_loop_stays_ungated); an
    unpinned config keeps today's resolution, and any other host -- malformed or a
    plausible enterprise name -- is refused
  • a gated loop's ack names its subject and drops the plain per-interval promise;
    an ungated one keeps it; an ambiguous instruction is reported as ungated
  • gate: false is reported as ungated AND arms an ungated loop -- asserted
    separately, because a flag that only changed the ack text would tell an
    act-on-quiet loop it was exempt and then slow it anyway
  • an opted-out loop stays ungated across an instruction edit and across a
    restart, asserted on the absence of a monitor the retarget would have attached
  • arming a loop that names one PR gates it with no parameter passed
  • an instruction naming two PRs arms ungated rather than guessing, because
    gating the blocker would silence the PR the loop owns
  • a source path with a line ref (autonudge.py#1751) is not a target
  • a gated monitor survives a restart and re-arms; a settled one does not; a
    future-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 own
over-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

  • Both specs move with the code, as the repo requires. babysit-pr-watch.md had
    said monitor_start re-injects per interval and located the probe under
    builtin_skills/; it now describes the gated default, the opt-out, what
    max_cycles bounds once gated, the starvation floor, and the narrowed role left
    for the manual cron. agent-interrupt-controller.md still owned the probe at
    the skill path and never mentioned irq.poll at all -- a new public kernel entry
    point absent from the kernel's own spec; it now documents both drivers, the
    Verdict outcomes, why FALLBACK is the failure direction, and why the
    kernel's bounds are not forwarded through poll.
  • gate: false ships with no producer yet, and that is deliberate rather than an
    oversight. The same standard removed irq.poll(**bounds) and narrowed the
    host key in this PR, so the difference is worth naming: those two generalised
    ahead 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.
  • Threading the kernel's brief into the woken turn is the obvious next
    increment: it is already computed and currently discarded.
  • Budgets remain unenforced on this path deliberately. The default cap is 8
    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.
  • Adding a second subject kind (a deployment, a ticket) is now one module plus
    one branch in probes.build, with no scheduler change.
  • The skill's pr_watch.py is a thin adapter over the installed package, so a
    stale copy in crons/ paired with a newer gateway is the one combination that
    can fail to import. The arm recipe already says to re-copy on every arm.
  • Once the brief is threaded into the woken turn, the skill's manual cron arm
    path has almost nothing left of its own -- known_reds, note and
    wake_on_green -- and measured zero armed instances even before this PR. It is
    a deletion candidate at that point, deliberately not bundled here.

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 1, 2026 14:34
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for a9417cca687205421882307fb9ccc57e7fcf464f; the recorded reason is authoritative for this commit.

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:
/ai-review override gpt a9417cca687205421882307fb9ccc57e7fcf464f: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of a9417cca687205421882307fb9ccc57e7fcf464f — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • Five per-loop credits (_pending_monitor_wake, _pending_floor_tick, followup_ticks, poll_in_flight, terminal_pending) are discarded/restored by hand at four-plus sites in autonudge.py; the diff's own comments admit the same defect class recurred three times during review ("third time a claim has been released in one set and forgotten in another") and name the fix — "one transition object with one restore" — without building it. The next edit to fire/retarget/cancel paths will silently miscount wakes or drop an owed turn, the exact trust the new counters exist to provide.
  • The monitor persists kind+target but not the host, so the binding is re-inferred from prose at three sites that must each remember the host_key comparison ("the third of the three places that comparison had to reach") — a fourth site that forgets it observes the wrong server.

Suggestions

  • Reify the credits into a single per-loop transition object with one discard and one restore, and persist the inferred binding (including host_key) on the monitor, invalidated only at arm/retarget — both as an immediate follow-up before more state is added.

[DESIGN-REVIEWED] a9417cc

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of a9417cca687205421882307fb9ccc57e7fcf464f — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: CONCERNS

The gate itself is cause-level and earns its place; what doesn't is a REST gate field nothing sends and host-comparison plumbing whose triggering case the design itself made unreachable.

What this change ships

Intent: stop paying a full agent turn for babysit cycles where nothing changed, by defaulting the existing zero-token probe into monitor_start. ADDITION.

  1. A loop naming one public PR URL skips turns on quiet ticks by default — justified (measured cost, five opt-ins at zero adoption)
  2. Merged/closed subject stops the loop and notifies, worded per outcome — justified
  3. gate=false opt-out on the monitor_start tool/directive — justified (act-while-quiet loops)
  4. gate body field on POST /api/autonudge — zero consumers
  5. Paused gated watch is resumable from the popover — justified (popover PATCH sends active: true)
  6. Editing the instruction retargets or clears the watch — justified (same PATCH sends message)
  7. Delivery floor after 10 quiet ticks — justified (starvation bound)
  8. One gate-free turn after each wake — justified
  9. Per-monitor counters persisted, exposed via existing GET /api/autonudge asdict — justified (the zero-adoption invisibility is the named harm)
  10. Probe moved to kiro_crew.probes.gh_pr + irq.poll front door — justified (hyphenated skill dir is unimportable; two drivers, one classifier)

Watch

  • Description says "no new parameter and no call-site change -- the MCP tool, the dashboard and the channels all inherit it"; the diff threads gate: bool through authorize_and_add_nudge and four surfaces, and the dashboard route/popover inherit nothing (default False, no UI sends it). The code's own comment at autonudge.py admits the claim "was true before that default moved and is not any more" — the description is a stale round.
  • Two parallel claim sets (_pending_monitor_wake, _pending_floor_tick) with four hand-written restores; the author's own comments count three defects from releasing one and forgetting the other, and already name the collapse. Accepted-and-deferred, but the fourth defect is priced in.
  • Black-baseline pruning reformats unrelated hunks (control.py's wait(), test_autonudge_stop_auth.py) inside a feature PR — AGENTS.md asks for that in its own commit. Rides along.

Subtractions

  • Drop the gate field from POST /api/autonudge (dashboard/handlers/autonudge.py:144-152): 0 producers (grepped api/autonudge + gate across website/src; the only POST body is {slot_key, message, idle_secs, max_cycles} at AutoNudgePopover.tsx:186). The route already defaults ungated; add the field with its first caller.
  • Drop Target.host_key and the same_host compare (targets.py:100, autonudge.py:1328): one value is ever constructed ("github.com", sole producer targets.py:193; the "default" default has 0). Since only public URLs infer, two inferred targets can never differ by host — this PR deleted the mid-poll host test as "an unreachable path [that] would only look like protection"; this is that path's sibling. Keep the probe's host pin, key identity on loop.id.

[FIRST-PRINCIPLES-REVIEWED] a9417cc

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed a9417cca687205421882307fb9ccc57e7fcf464f — this comment is updated in place on each push.

Review details

No blocking issues.

FINDING — src/kiro_crew/mcp_tools/control.py:962 — for a gated loop armed with an explicit max_cycles=0, the ack renders "...cost no turn and the {max_cycles or 0} cap counts delivered turns""the 0 cap counts delivered turns", immediately followed by the trailing clause ", with NO cycle cap", so the same message asserts both a 0 cap that counts turns and no cap at all → Fix: only append " and the {max_cycles} cap counts delivered turns" when max_cycles is truthy.

[OPUS-REVIEWED] a9417cc

Verdict parsed from the review's SHA-scoped output markers for commit a9417cca687205421882307fb9ccc57e7fcf464f.

False positive or not applicable? A repository writer can comment:
/ai-review override fable a9417cca687205421882307fb9ccc57e7fcf464f: <one-sentence reason>

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 1 dispositions -- head 244d337

GPT BLOCKING, autonudge.py: "a quiet tick permanently loses its timer" -- FIXED

Real, and the worst possible shape for this PR: the first quiet observation would
have been the last one the watch ever made. Verified the mechanism rather than
taking the report on trust. The delivered paths re-arm elsewhere -- a dashboard
slot through notify_turn_complete, a channel key through the fire cycle's own
exit -- and the quiet branch returns before either, so nothing rescheduled the
timer.

Fixed by advancing next_due_ts and self-re-arming before returning.
_cancel_timer documents that it refuses to cancel the currently running timer
task, so a self-re-arm from inside _timer is the supported pattern and is what
the delivered path's own finally block already does.

The more useful finding is why my tests missed it: they asserted only that no
turn was spent, and a dead watch spends nothing either. Added
test_a_quiet_tick_re_arms_its_own_timer, which asserts a timer is armed and a
deadline is set. Mutation-verified: with the arm call removed it fails with
"the quiet tick left no timer armed", and passes with it restored.

GPT FINDING, gh_pr.py:329 bool(params.get("wake_on_green", True)) -- PRE-EXISTING, out of scope

Correct as an observation: a JSON string "false" is truthy, so an explicit
disable would be ignored. But this line is byte-identical on origin/main:

git show origin/main:.../babysit/scripts/pr_watch.py | grep -n wake_on_green
329:        self.wake_on_green = bool(params.get("wake_on_green", True))

This PR moves that file without changing that expression, so the hole predates it
and belongs to its own issue rather than to a diff that is already large. Filing
it separately; happy to fold it in instead if a maintainer prefers that.

Evidence: the saving, measured against a real pull request

The unit tests stub the kernel boundary, so the whole chain was also run for
real, once, against this PR itself:

inferred target : kirodotdev/KiroCrew#7634
probe config    : {"repo": "kirodotdev/KiroCrew", "pr": 7634}
probe resolved  : PrWatchProbe
monitor kind    : gh-pr  target=kirodotdev/KiroCrew#7634
tick 1          : quiet=True quiet_ticks=1 wakes=0 fallbacks=0
turns spent     : 0

fallbacks=0 is the part that matters: the quiet verdict came from a real
observation, not from the safe-direction fallback that fires when gh cannot be
reached. A fallback would also have spent a turn, so it could not be mistaken
for the saving.

Two properties worth a reviewer's attention, stated in the body already

max_cycles now bounds wakes rather than ticks for a gated loop, and the
kernel's brief is computed and currently discarded rather than threaded into the
woken turn. Both are deliberate for this PR and both are called out in the
description rather than left to be discovered.

@chenmingwei23
chenmingwei23 force-pushed the feat/irq-gated-monitor-loop branch from edfa891 to 244d337 Compare September 1, 2026 14:47
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 2 dispositions -- head 2ca653c

Both blocking findings were real. One of them was introduced by round 1's own
fix, which is worth saying plainly: fixing a silent failure created a different
one, and only the reviewer's second pass caught it.

BLOCKING, autonudge.py: terminal verdicts re-arm the stopped loop -- FIXED

Correct and self-inflicted. _monitor_tick_is_quiet returns True for TERMINAL
too, because "do not spend a turn" is the right answer for a merged pull request
-- but the round-1 re-arm treated that True as "keep watching" and rescheduled a
timer on a loop it had just deactivated. Result: a merged PR would be polled
forever and its expiry notification re-emitted every tick.

Fixed by re-arming only while the loop is still active AND still registered. The
registration half matters separately: a loop removed during an observation must
not be resurrected by its own in-flight tick.

Added test_a_terminal_verdict_leaves_no_timer_armed, which asserts the
ABSENCE of a timer rather than only the absence of a fire. Mutation-verified:
with the liveness guard removed it fails with "a finished subject must not stay
armed".

BLOCKING, autonudge.py: message updates leave the probe bound to the old PR -- FIXED, but not by the prescribed remedy

The finding is real. update(message=...) changed the instruction without
re-inferring, so a loop retargeted from PR A to PR B kept polling A: B was never
watched, and A merging would have retired the loop while the work it was pointed
at sat unobserved.

The prescribed remedy -- "remove automatic monitor attachment until message
updates also retarget it" -- would delete the feature, since automatic attachment
is the entire point of the change (a passed-in parameter is an opt-in, and the
opt-in version of this saving measured zero adoption for over a week). The second
half of that same sentence names the actual fix, and that is what is implemented:
message updates now retarget.

Semantics, all three pinned by tests:

  • a changed instruction naming a different subject replaces the monitor
  • a changed instruction naming the SAME subject keeps the existing monitor, so
    refining wording does not silently reset the metering counters or the
    post-wake follow-up allowance
  • an instruction that no longer names one subject clears the monitor, returning
    the loop to a plain timer rather than leaving it bound to a stale target

Mutation-verified: with the retarget disabled the test fails with
assert 'acme/widgets#42' == 'acme/widgets#77'.

Note on the review pattern so far

Both rounds found the same class of defect -- a state transition where "cheap
tick" and "loop lifecycle" disagree -- and in both cases my tests had asserted a
presence (no turn fired, loop deactivated) where the bug lived in an absence (no
timer armed, timer armed when it should not be). Every assertion added in this
round is mutation-verified for that reason.

285 assertions green across the touched suites; flake8, mypy and the repo black
gate clean.

@chenmingwei23
chenmingwei23 force-pushed the feat/irq-gated-monitor-loop branch from 244d337 to 2ca653c Compare September 1, 2026 14:59
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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.

@chenmingwei23
chenmingwei23 force-pushed the feat/irq-gated-monitor-loop branch from 2ca653c to 69a14cc Compare September 1, 2026 15:05
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 3 dispositions -- head a6ef84d

Three lanes, five concerns, all dispositioned. The Design concern about
act-on-quiet loops was the most valuable finding of this review so far and
changed the behaviour, not just the docs.

Opus BLOCKING (terminal tick re-arms forever) -- ALREADY FIXED, verdict pinned to an older SHA

Same defect GPT raised, and Opus's prescribed fix is what shipped: the re-arm is
guarded (if loop.active and loop.id in self._loops:) and the terminal path now
records the finish on the monitor as well. Its verdict names 244d337, two heads
back; it re-rolls on the current head. Nothing to change.

Design: gating is inferred from a mention, so an act-on-quiet loop is silently inverted -- FIXED, behaviour changed

This was right and I had missed it. My safety argument covered probe FAILURE and
said nothing about intent-inference failure. 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 no
observable change, so a pure gate would never deliver it again. That is not
hypothetical: an auto-fix pipeline in this repo requires a heartbeat refresh on
every monitor cycle, and a cleanup job reaps the work directory when it goes
stale, so silencing those cycles destroys work.

Inference cannot read that intent out of the wording, and guessing it would be
worse than bounding it. So the gate now bounds how long ANY loop can go
undelivered: after ten consecutive quiet observations the loop is delivered
anyway, and the streak resets. Nine ticks in ten still cost nothing, so the
saving is substantially intact, while "gating slows an act-on-quiet loop" is now
true instead of "gating silences it". Delivered-by-floor is counted in its own
floor_ticks field so the metering never reports a periodic delivery as a real
signal. Mutation-verified: with the floor disabled the test fails with "the floor
must deliver a turn".

Design + First Principles: phantom disclosure -- FIXED

Both lanes caught the same thing independently, and they were correct: a comment
claimed the max_cycles reinterpretation was "stated in the tool description"
while this diff touched no tool description. Rather than delete the claim and
leave the surface silent, the description now says it -- gating, what max_cycles
counts once gated, and the one wording decision an agent controls (naming exactly
one pull request is what turns gating on). control.py is in the repo's
black baseline, so only the description string changed and the file's existing
layout was left alone; reformatting it would have added ~50 lines of unrelated
churn to an already large diff.

First Principles: irq.poll(**bounds) has zero production consumers -- ADOPTED

Correct by the kernel's own standard, which already refuses to recognize a
tuning key with no producer. Dropped. The tests that needed a coalescing floor
now declare it through the probe's own tuning() hook, which is how a real probe
asks for one -- so the suite exercises the shipped mechanism instead of a
test-only passthrough. 71 assertions in the irq suite still green.

First Principles: items 5 and 6 undeclared -- FIXED in the description

A paused current monitor being resumable, and editing the instruction
retargeting or clearing the watch, are both real user-visible flips of previously
pinned refusals. Both are now in the body's fix and test lists.

First Principles: the skill's manual cron arm path is now near-redundant -- ACCEPTED, deferred

Agreed, and the reasoning is sound: once the wake brief is threaded into the
woken turn, its remaining distinct capability is known_reds / note /
wake_on_green. Not deleted here because that judgment belongs with the brief
work, and deleting a shipped user-facing arm path in a PR about scheduler
behaviour would bundle two decisions.

286 assertions green across the touched suites; flake8, mypy and the repo black
gate clean; no mutation probes left in the tree.

@chenmingwei23
chenmingwei23 force-pushed the feat/irq-gated-monitor-loop branch from 69a14cc to a6ef84d Compare September 1, 2026 15:19
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/irq-gated-monitor-loop branch 2 times, most recently from ca565d4 to 0ac84d2 Compare September 1, 2026 16:06
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 44 -- head 823a3a6

The body edit for the description mismatch re-rolled the codex lane on the same SHA, and
the re-roll found something real on code it had already passed twice. Worth saying plainly,
because it is evidence about the review process as well as about this defect: a clean
verdict is a sample, not a proof of absence.
The diff was byte-identical.

BLOCKING: the terminal revalidation spent the tick's dedupe credit -- FIXED

Round 42 added a re-observation before settling, and I chose to RE-ASK rather than remember
that a terminal had been delivered, specifically to avoid adding per-loop state. That was
the right trade for the reason I gave, but I did not account for the re-ask having a side
effect.

identity is the kernel's dedupe key. poll's own contract says so: it "replaces the cron
job id in the state digest, so two drivers watching one subject keep independent dedupe
memories". The revalidation passed the SAME identity the tick uses --
f"{loop.id}:{target.host_key}" at both autonudge.py:2205 and :2728 -- while returning
only a bool. So a reopened subject with a fresh comment, observed by the revalidation, was
recorded as REPORTED and then discarded. The next real tick read the same state as
unchanged, called it quiet, and the signal was gone until the streak floor.

A poll whose answer is thrown away must not be able to swallow a signal. The revalidation
now observes on its own memory, ...:terminal-recheck, and leaves the tick's untouched.
That is not a design choice -- it is the documented purpose of the parameter, which I had
passed incorrectly.

Mutation-verified: sharing the tick's key fails with "the discarded poll must not spend the
tick's key". 1153 assertions across eighteen suites; flake8, mypy and the black gate clean.

Count, since it is the argument

Fourteen findings have now landed in the terminal-delivery mechanism, and this is the
FOURTH consecutive round where the finding sits in the previous round's remedy: 41 fixed a
claim not released, 42 fixed a settlement that never re-observed, 43 fixed the
revalidation accepting any terminal, 44 fixes the revalidation consuming dedupe.

Each fix has been small, correct, and has created the next finding's surface. I am still
fixing what is reachable -- this one was a single string and it closed a real data-loss
path -- but four rounds of that pattern is not convergence, and I am not going to design
the collapse of this lifecycle inside a change whose subject is gating. Decision (a) is
unchanged.

Everything else on the previous head was clean, including Opus, Design, First Principles
and CodeQL, and the description mismatch that prompted the body edit is addressed.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 45 -- head 5980d4f

All five lanes reported clean on 823a3a60d -- GPT, Opus, Design, First Principles and
CodeQL -- but both marker BODIES carried a finding, and both were real. Neither is prose:
each one emits a false statement to the user, and one of them makes a claim in this
Description untrue, which is the class a maintainer requested changes for two hours ago.
So both are fixed rather than dispositioned.

Opus: a merge landing ON the capping delivery was announced as an unmet goal -- FIXED

slack/gateway.py guarded its terminal-reason branch with not capped_out. The delivery
that CARRIES the terminal news increments cycle_count before the settlement records
stopped_reason -- deliberately, since round 35 moved the accounting ahead of the
settlement so a cancelled write could not lose the turn. So a pull request merging on the
delivery that reaches max_cycles made capped_out true, skipped the terminal branch, and
fell through to "hit its cycle cap ... goal possibly unmet ... restart it" -- about a
subject that had merged. That is precisely the message the branch exists to prevent.

I verified the prescribed remedy, not just the finding, because dropping one guard is only
sufficient if the chain order cooperates: the cap wording is the terminal else of the
chain, so removing not capped_out from the terminal elif is enough -- it is evaluated
first and the fallthrough is never reached. A finished subject is terminal whether or not
cycles remained; the cap is the less true of the two readings, because the reason the loop
stopped is that there was nothing left to watch.

GPT: the dashboard applier promised an every-interval re-injection for a gated loop -- FIXED

dashboard/session_directive_apply.py defaults gate to True and then acknowledged the
arming with "the message re-injects every {idle_secs}s". For a gated loop that is untrue --
a quiet tick spends no turn at all -- so the surface was wrong about its own default. The
MCP tool's ack already disclosed this; the dashboard directive path was the second arming
surface and did not.

That also made a Description claim false: the body says the arming surface names the
subject and states that the message re-injects only when it changes. One arming surface
did not, which is the same description/code mismatch class bolichen97 requested changes
for at 10:10. Fixing it is what makes that claim true rather than reworded.

The applier now reads the cadence off the ARMED loop -- monitor present and gate set --
rather than re-inferring from the message. This surface knows what the tool has to guess:
whether a monitor was actually attached.

Black baseline

Formatting the two touched files graduated them out of .github/black-baseline.txt, so the
gate demanded a prune; three lines are removed and 1219 entries remain. Declaring it because
the prune is a diff to a shared file, not a side effect of mine to leave unexplained.

Both fixes mutation-verified separately: restoring the cap guard fails with "a finished
subject is terminal, cap or no cap"; disabling the disclosure fails with "the ack must state
the gated cadence". 1236 assertions across twenty suites; flake8, mypy and the black gate
clean.

Neither finding was in the terminal-delivery mechanism, so the fifth-landing rule I set for
myself did not apply -- one is the Slack notifier's wording chain and the other is a
dashboard applier.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 46 -- head 999ea3f

All five lanes were clean on 5980d4f0a and Opus's body was empty for the first time. GPT's
two advisories were both real and both fixed. One of them is the same defect I fixed last
round, one branch further up -- which is why this round changes the SHAPE rather than adding
another guard.

GPT: a terminal subject was still preempted, now by the wall-clock budget -- FIXED

Round 45 removed not capped_out from the terminal branch so a merge landing on the capping
delivery would not read as an unmet goal. That was not enough: the runtime-budget and
approval-stall branches are evaluated BEFORE it, so a terminal delivery that had also
exhausted its wall-clock budget still reported "its budget ran out without it reporting
done, so its goal may still be unmet" -- about a subject that had merged.

I did not add a third guard. The rule is now stated once, as a terminal flag the earlier
branches defer to: every other branch in that chain explains why the loop stopped SHORT of
its goal, and a terminal subject is not short of anything.
Adding not capped_out, then
not runtime_expired, then whatever the next reviewer finds is the pattern that produced
two rounds of this; naming the invariant once ends it.

GPT: "PRs #42 and #7" saw only #42 -- FIXED

The bare-form ambiguity check required the literal PR/pull request prefix on each
reference, so a chained list carried the prefix once and the second number went unseen. A
loop gated on the URL for #42 would then retire with the work on #7 unfinished -- the exact
harm the ambiguity check exists to prevent, reached through the way a person naturally
writes a pair.

The prefix now covers a chain of #N separated by a comma, and or &, and every number
in it is compared. The chain is BOUNDED on purpose: it stops at the first token that is
neither, so an unrelated #7511 later in the instruction is not swept in and cannot refuse
a legitimate subject. A chain naming one subject twice still gates.

Both mutation-verified: restoring the branch order fails with "a finished subject is not a
spent budget"; reading only the chain's first number infers acme/widgets#42 from the
"PRs #42 and #7" case. 1238 assertions across twenty suites; flake8, mypy and the black gate
clean.

Two process notes, since both cost me a step this round

The first attempt at the notifier fix left the chain in two pieces -- a leading if paired
with the wrong body -- because I edited a branch condition and its block separately. I read
the chain back, saw a duplicated if, and repaired it before running anything; the flag
shape is also what made the repair small.

Formatting also produced two NEW black offenders this time rather than graduating baselined
files, so only the two files the gate named were formatted. Last round's prune stands at 1219
entries.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 47 -- head b735962, and this is the last change I intend to make

All five lanes were clean on 999ea3f91, bodies read as well as checks. Opus's body is
worth quoting because it shows the bar working in the other direction: it considered a
candidate and dropped it itself -- "a metering off-by-one the design explicitly tolerates
... in a sub-second window ... clears neither the 80+ confidence bar nor any BLOCKING
class."

GPT's one advisory: the spec understated what the cap spends -- FIXED

docs/system-specs/features/babysit-pr-watch.md said max_cycles "bounds wakes". It bounds
DELIVERED TURNS: a wake is one of four things that consume the budget, alongside a
streak-floor delivery, a gate fallback and a post-wake follow-up. Reading the cap as a wake
count under-states what it spends.

Why this prose finding earned a push when the others did not

I have been declining prose fixes since round 38, on the measured grounds that five
consecutive advisory pushes each produced a fresh advisory and each cancelled the slowest
lane mid-run. That reasoning still holds for the PR body and for code comments. It does not
hold here: a spec ships. Both specs move with the code in this commit, so leaving this
one wrong lands an incorrect statement of the feature's own cap semantics on main, where the
next reader has no review thread to correct it. The stale PR-body line I am still declining
(autonudge.py:666) is a comment in a diff; this was a claim in a published contract.

I also swept for the same claim elsewhere before pushing: the tool description already says
"the cap counts delivered turns", and the kernel spec does not make the claim at all.

Timing was deliberate too -- I held this from the previous cycle so the push would not
cancel Opus mid-run for a second time. It concluded first.

89 assertions over the touched suites; the black gate is clean and the baseline still stands
at 1219 entries.

Where this leaves the PR

Nothing outstanding that I can act on. Every lane has been clean on a head, the CodeQL alert
is state=fixed, QA passed four acceptance checks in an isolated pod with gate_fallbacks
at zero throughout, and bolichen97's description/code mismatch is corrected and answered. I
am not merging this, and the four decisions in the body remain the author's:

  1. Split the channel terminal delivery into its own PR -- fourteen findings landed in that
    one mechanism and four consecutive rounds each found the defect inside the previous
    round's remedy.
  2. Whether first-party gh reads should require PreToolUse approval.
  3. Add a surface that READS the metering. Nothing renders these counters today.
  4. Collapse the four hand-written claim restores into one transition object.

One advisory stays open by choice, recorded rather than patched: autonudge.py:666 still
says malformed values normalize "to the DEFAULT rather than to False", which round 27
inverted. One line, no behaviour, pinned by tests.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 48 -- head e014cba

I said round 47 would be the last change. It is not, because GPT found something I could
not leave: my round-39 and round-40 accounting fixes never worked in the case they were
written for.

BLOCKING: re-arming a timer erased the claim the refusal had just re-owed -- FIXED

A refused fire re-owes its wake so the retried delivery still charges it. The refusal path
then re-arms with a backoff -- and _arm_timer cancels before it creates, while
_cancel_timer dropped both claims. So the sequence was: re-own the claim, then erase it
one statement later, on the ordinary path taken every time a slot is busy.

The wake was undercounted and its follow-up turn lost, and both fixes that were supposed to
prevent exactly that were defeated by the cleanup meant to protect them. The comment on the
cleanup even records why it was safe -- "the fire path releases the claim on both its
delivered and refused branches, and neither runs once the task is gone" -- which was true
when written and made false by the round-39 change that re-owes on refusal. A later fix
invalidated an earlier comment's premise, and the code resting on that premise was not
revisited.

Replacing a timer is now distinguished from cancelling a cycle: _arm_timer passes
drop_claims=False, every other caller keeps today's behaviour.

An over-reach I caught and withdrew, because it matters more than the fix

My first attempt deleted the claim drop from the cancel path entirely, reasoning that a
cancelled cycle delivered nothing so its wake is still owed. An existing test --
test_a_cancelled_cycle_does_not_bequeath_its_wake -- failed, and it was RIGHT: after a
cancel the next delivered turn can be a floor tick, which would then be charged as a floor
tick AND a wake, counting one turn under two counters. That is a deliberate trade the test
pins: an undelivered observation is lost rather than attributed to a turn that did not carry
it.

So I withdrew the wider change and implemented the narrow one, which is what the finding
actually prescribed. Worth stating because the failing test was the only thing between a
real fix and a metering bug introduced while fixing a metering bug.

And a test that could not fail, caught the same way

The first version of the new test passed WITH the bug present, because _cancel_timer
returns early when no timer task exists and the test had never armed one. It now arms a real
timer and asserts it is there before firing. That is the third unfalsifiable test I have
caught in this review; the check that catches them is always the same -- mutate the fix and
require the test to go red.

Mutation-verified: making the re-arm drop claims fails with "the re-arm must not erase the
debt", while the sibling cancel test stays green. 1239 assertions across twenty suites;
flake8, mypy and the black gate clean.

This is the FOURTH finding in claim bookkeeping -- rounds 14, 26, 41 and now 48 -- which is
decision (d) in the body: collapse the hand-written claim sites into one transition object
with one release. Each of the four was a claim released, restored or erased in one place and
not another.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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:

  • _pending_monitor_wake and _pending_floor_tick are plain in-memory sets on the
    service (autonudge.py:612 and :616); neither appears in the serialized state.
  • followup_ticks IS persisted -- it is a MonitorState field (models.py:236) and is
    listed in the round-trip at :310.

So a busy-slot refusal leaves a PERSISTED allowance and a MEMORY-ONLY debt. Restart in
that window and the next tick spends the allowance, delivers the turn, finds no claim, and
charges neither wakes nor floor_ticks.

Why I am declining to fix a BLOCKING

The prescribed remedy is to persist the debt. That means two more durable per-loop fields,
taking the hand-written claim sites from eighteen references to twenty, and this review has
measured what that costs: rounds 14, 26, 41, 48 and now 49 are five findings in claim
bookkeeping, and every one of them was a claim handled in one place and not another.
Round
41's own comment predicted the site that broke in round 41. Round 48 was my round-39 fix
being erased by a cleanup whose premise my round-39 fix had invalidated.

Adding durable state to that structure is not a fix, it is the sixth site. The remedy this
finding actually points at is decision (d) in the body: one owed-charge record, durable,
with one release -- which is design work, and one round ago I wrote down that I would not
spend it unilaterally inside a change whose subject is gating. Fixing it narrowly here would
contradict that within a single cycle, and the inconsistency would be worse than the defect.

The honest impact, so the decision is made on facts

The window is a restart between a refused fire and its retry. The effect is a metering
under-count: the delivered retry is not charged, so the saving reads BETTER than it is by one
turn. Direction matters and I will not spin it -- an over-stated saving is the failure mode
this whole line of work exists to prevent, which is exactly why the structural fix should be
made deliberately rather than as a sixth patch.

For calibration: Opus dropped a neighbouring candidate on an earlier head as "a metering
off-by-one the design explicitly tolerates ... in a sub-second window". This one is wider --
it spans a restart, not a sub-second race -- so I am not claiming it is below the bar. I am
saying the bar is not what decides it.

State of the PR

Design, First Principles and CodeQL are clean on this head; Opus is still running. Nothing
else is outstanding that I can act on. The four decisions in the body stand, and (d) is now
load-bearing rather than tidiness:

  1. Split the channel terminal delivery into its own PR -- fourteen findings, four consecutive
    rounds each finding the defect inside the previous round's remedy.
  2. Whether first-party gh reads should require PreToolUse approval.
  3. Add a surface that READS the metering.
  4. Collapse the claim sites into one durable owed-charge record with one release. Five
    findings, and this one cannot be fixed correctly without it.

I am not merging this.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

/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.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for e014cba1c9a54923a46d3fc2fa7ab65729f73835.

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.

This decision applies only to this commit. A new push requires a new judgment.

buluoray
buluoray previously approved these changes Sep 2, 2026

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) runs irq.poll(identity, target.message, probe) in an executor and returns True for a QUIET verdict before _on_fire is reached, so no model turn is dispatched. irq.poll (src/kiro_crew/irq.py) runs the probe and maps Skip/Report/Done to a Verdict; the gh-pr probe (src/kiro_crew/probes/gh_pr.py) observes via one bounded gh subprocess through github_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 = 10 the loop delivers anyway (_pending_floor_tick, returns False → fires) and resets the streak. Confirmed against test_a_long_quiet_streak_is_delivered_anyway, which drives ten real _timer calls and asserts the tenth delivers with floor_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 delivered turn that increments cycle_count in _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_cycles is still enforced on the new path. The cap check at autonudge.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 advances cycle_count via the floor every 10th tick, so the cap trips. Stop paths (sentinel file, notify_user_input cancel, autonudge_stop) are preserved.
  • Probe failure fails safe. In-flight-marker write failure, executor exception, irq.poll returning 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: … return blocks in _timer and _run_fire_cycle are removed and replaced by the gate; monitor loops now flow through the gated fire path. pr_watch.py is reduced to a 53-line cron adapter that keeps its watch(ctx) entry point; irq.run (cron contract) is untouched.
  • The LLM-facing surface was updated in the same commit. mcp_tools/control.py states URL-only gating, that max_cycles now counts delivered turns, and the gate: false opt-out; the babysit SKILL.md and 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, and test_a_terminal_subject_stops_the_loop_without_a_turn: each constructs a real loop, monkeypatches irq.poll to a fixed Verdict, drives service._timer(...), and asserts observable outcomes (fired, active, quiet_ticks, wakes, floor_ticks). Reverting the gate flips the waking test's fired == [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

  1. dashboard/handlers/autonudge.py:144 — the gate request field has no producer. No website/ file is changed in this PR, so the goal popover still POSTs {slot_key, message, idle_secs, max_cycles} and never sends gate. Consequence: none today — the route correctly defaults to gate=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.

  2. 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_timer and _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.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 50 -- disposition for @buluoray's independent review, and what my rebase cost it

First, the thing I owe you plainly: your review is now DISMISSED, and that is my doing.
Raymond asked me to clear a real merge conflict (mergeable=CONFLICTING, mergeStateStatus=DIRTY);
main was 138 commits ahead and the force-push dismissed your review along with the override.
I did not anticipate that cost when I pushed. Your verdict at e014cba1c was 0 blocking, 2
non-blocking, and it is worth more than a dismissed row -- so both findings are answered here,
on the record, rather than being lost with the review that carried them.

The conflict itself is worth reporting because of WHERE it landed: main has independently added
its own probe fields to MonitorState (probe_count, provider_error_count, last_probe_at,
last_decision: MonitorDecision, last_provider_error) in the same field block and the same
__post_init__ validation list. I resolved it as a union and verified three ways: zero conflict
markers, git diff origin/main on that file shows ZERO deleted lines (purely additive over
main), and main's own suites pass -- test_monitor_decision, test_monitor_persistence,
test_monitor_turn_completion and test_github_pull_request_monitor are in the 638 that ran green.

Finding 2 (hand-restored transitional state) -- ADOPTED as the recommendation, scheduled not shipped

You are right, and your framing is the one I will use: schedule it, do not leave it as a comment.
Since your review a FIFTH instance landed (GPT flagged the refused-delivery debt being memory-only
while followup_ticks persists), and I declined to patch it precisely because a sixth hand-written
site is not a fix. That is decision (d) in the body, and your review is the second independent
reviewer to name it. It is the follow-up, not a hunk in this PR.

Finding 1 (the gate request field has no producer) -- premise CONFIRMED, subtraction DECLINED

I verified your premise rather than taking it: AutoNudgePopover.tsx:186 builds exactly
{ slot_key, message, idle_secs, max_cycles }, so nothing sends gate today. I also confirmed the
directive path does NOT go through this route -- session_directive_apply.py:221 calls
authorize_and_add_nudge directly -- so this parse serves the REST route alone.

I am keeping it, and the reason is not forward-wiring. Deleting the parse would not restore a
default; the route already defaults to ungated. What it would change is that a REST caller sending
an explicit gate: true would be silently ignored instead of honoured. This PR already spent a
round on exactly that asymmetry in the opposite direction -- the tick had to learn to honour
loop.gate because an opt-out honoured by some paths and ignored by one is worse than none -- and
deleting this parse re-creates it on the arming side. The parse is also what rejects a non-boolean,
which you noted yourself.

Where I will not overstate my case: my argument rests on a REST caller that does not exist today,
and "someone might POST it" is weaker than a live producer. What makes it more than hypothetical is
that /api/autonudge is a gateway API surface, not a private popover channel -- apps and scripts
reach it too. If you read that differently, say so and I will drop the field; I am not defending
three lines, I am declining to introduce a silent-ignore path.

Two things you flagged as unverified, answered

You noted you did not run the suite or mutation-test. On the rebased head: 638 tests pass across my
suites plus main's four monitoring suites, and flake8, isort, mypy and black are clean. Every
new assertion in this PR was mutation-verified by breaking the fix and requiring the test to go red
-- that check caught three tests in this review that had passed WITH the bug present, which is the
only reason I trust the rest.

On the override: you read it correctly as a human ruling rather than a model pass, and you were right
not to re-adjudicate it. For the record it was Raymond's call on proportionality, and its reason says
reachable-but-disproportionate rather than false positive, deliberately -- overriding on a
false-positive claim would have laundered a known bug to green. That override died with this push and
will be re-posted on the new head only if GPT reproduces the same finding.

No push for this comment.

buluoray
buluoray previously approved these changes Sep 3, 2026

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) runs irq.poll(identity, target.message, probe) in a thread executor and returns True for QUIET before _run_fire_cycle is 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 a Verdict; the gh-pr probe (probes/gh_pr.py) observes through _run_ghgithub_runner.run_gh, one bounded subprocess. No acp/provider/model import anywhere in that chain.
  • max_cycles is 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 goes next_due_ts = … ; _arm_from_deadline(loop) (:2691) which schedules a fresh _timer, re-entering the check. A permanently-quiet loop still advances cycle_count via 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, returns False → fires) and resets the streak — a quiet subject cannot stall forever. At the other extreme, WAKE/FALLBACK/floor/follow-up all return False and each is a delivered turn that advances cycle_count in _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, target None/mismatch, poll_in_flight, retarget-mid-poll) returns False.
  • Old timer path migrated, not orphaned. The legacy if loop.monitor is not None: … return guard 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.py is reduced to a cron adapter keeping its watch(ctx) entry point; the cron irq.run() contract is untouched (irq.poll is added beside it, non-raising).
  • MCP tool description matches the code. mcp_tools/control.py:246 states URL-only gating (https://github.com/<owner>/<repo>/pull/<N>), that a quiet cycle costs no model turn, that max_cycles then counts delivered turns, the starvation floor, and the gate=false opt-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, and test_a_verdict_is_discarded_when_the_loop_was_retargeted_mid_poll each construct a real loop, patch irq.poll to a fixed Verdict, drive service._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

  1. dashboard/handlers/autonudge.py — the gate request field has no frontend producer. No website/ file is in this PR, so the goal popover never sends gate. Consequence: none today — the route defaults to gate=false on 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.
  2. 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_timer and _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.
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 51 -- both reviewers passed on their own, and I spent the push on two wrong comments

On fad0a3abc every lane came back clean with no override in play: GPT 5.6 "no blocking findings"
naming that head, Opus 4.8 the same, plus Design, First Principles and CodeQL. The override Raymond
authorised for the previous head was voided by the rebase and did NOT need re-posting -- GPT reached
green on its own reading of the code.

GPT's two non-blocking findings were both wrong statements in comments, and I pushed for them. That
reverses my own round-38 decision to leave the first one alone, so here is the reason it changed
rather than a quiet flip: that decision was made when a push would have voided an ACCEPTED override.
No override exists now, so the thing I was protecting is gone, and what remains is documentation that
ships to main saying the opposite of the code.

autonudge.py:665 -- worse than the finding described

GPT called it a stale sentence. Reading it, the file carried TWO STACKED COMMENT BLOCKS stating
OPPOSITE rules: the round-8 era text ("normalise to the DOCUMENTED DEFAULT rather than to False")
sat immediately above round 27's correction, which says the value is normalised to FALSE and explains
why round 8 had the asymmetry backwards. Round 27 wrote the right explanation and never deleted the
one it superseded, so a reader hits the wrong rule first and the code agrees with neither until they
read further.

The whole block is NOT deletable, which is why I read it before cutting. Its first half is
load-bearing and correct: the string "false" is truthy, so an unnormalised value would gate a loop
that asked not to be, and the boundary is the right place to fix that rather than every read site.
Only the trailing direction claim was wrong. That clause is gone; the hazard and the placement
rationale stay. grep for the stale claim now returns zero.

models.py:229 -- this one misdescribed the number the PR exists to justify

The comment said quiet_ticks counts QUIET ticks "and wakes the ones it did not." That is wrong
twice over. A tick the probe could not judge is gate_fallbacks, counted separately and deliberately
so a permanently broken gate cannot read as a busy watch; and wakes is charged at CONFIRMED
DELIVERY since round 39, so a fire the slot refuses is charged to neither -- it re-owes the claim.
Anyone reading the old sentence to interpret metering output would over-count real signals and
under-count gate failures.

This is not tidiness. The entire argument for this change is that the metering is honest, and the
comment next to the counter contradicted the counter. It now says wakes counts turns actually
delivered because the probe judged otherwise, and names both exclusions.

Verification

638 tests pass, including main's test_monitor_decision, test_monitor_persistence,
test_monitor_turn_completion and test_github_pull_request_monitor -- the suites belonging to the
probe fields main added while this branch was out. flake8, black, mypy clean on both edited
files. No behaviour changed: both edits are comment text, which is also why no new test accompanies
them -- a test asserting comment content would pin source text, which the reviewers have rightly
objected to elsewhere in this PR.

New head a9417cca6. The four decisions in the body stand, with (d) -- one durable owed-charge
record replacing the hand-written claim sites -- now named independently by the Design lane, by
@buluoray's review, and by the fifth finding that landed in it.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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,
so here is the evidence for each.

The premise holds -- I read the ordering rather than taking it

In _timer the stop paths run in this order, each with its own return:

  1. stop sentinel -> remove() -> return
  2. cycle cap -> update(active=False, stopped_reason="cycle_cap") -> _emit("expired") -> return
  3. wall-clock budget -> update(active=False, stopped_reason="runtime_budget") -> _emit("expired") -> return
  4. approval stall -> same shape

terminal_pending is set at autonudge.py:2379 and consulted far below all four. So the sequence
GPT describes is reachable exactly as written: terminal observed, terminal_pending set, the channel
fire refused because the thread is busy, and on the retry _timer hits the cap or the budget first
and deactivates without ever delivering the terminal turn.

It is also slightly worse than the finding says. The user does not merely lose a turn -- they get
expired with stopped_reason="cycle_cap", so a watch whose subject MERGED reports that it ran out
of cycles. The announce lives in the settlement, which this path never reaches, so nothing corrects
the impression. A misleading terminal state is worse than a missing one.

Why it is not fixed in this PR

This is the FIFTEENTH finding in the channel terminal-delivery mechanism (rounds 15-19, 25, 31,
33-36, 40-44, 48, and now 52). The pattern is not that the mechanism is unlucky. Rounds 41, 42, 43,
44 and 48 each found the defect inside the PREVIOUS round's remedy.
Every fix in there has been the
next finding's subject. That is the signal that iteration inside this mechanism is itself the defect
source, and the answer is not a sixteenth patch applied at round 52 of a review.

The prescribed fix -- "process terminal_pending retries before cap, runtime-budget, and
approval-stall checks" -- is a reordering of three independent stop paths around an outstanding-debt
check. Note what it actually decides: that a loop AT its cycle cap may still deliver one more turn.
That is probably right, and for a reason this PR already committed to elsewhere -- the Slack notifier
computes terminal once and the earlier branches carry not terminal, because every other branch
explains why a loop stopped SHORT of its goal and a terminal subject is not short of anything. GPT is
applying that same principle to _timer, which is evidence the finding is coherent with the design
rather than noise.

But "the cap may be overridden by an outstanding terminal debt" is a change to the meaning of
max_cycles, which an independent reviewer verified this week as the bound that keeps a runaway loop
bounded. That deserves its own PR, its own tests, and its own review pass -- not a hunk appended to a
change whose subject is gating.

Decision (a) in the body gates this: split the channel terminal delivery into its own PR. Fifteen
findings, five of them inside the previous fix, is the argument for it, and this finding is now the
strongest single piece of that argument. I am recording it there rather than half-solving it here.

What that leaves on the PR

Opus 4.8 and CodeQL are green on this head; Design and First Principles were green on it before the
GPT re-roll. GPT reported NO blocking findings on fad0a3abc, whose logic is byte-identical to this
head -- the only difference is the two comment corrections in round 51. A finding that appears on one
sample and not the other, on unchanged logic, is a reminder that a green verdict is a sample and not
proof of absence. That cuts both ways, which is why I verified this one against the source instead of
discounting it as a re-roll artifact.

I am not merging this, and I am not overriding this one either: unlike the round-49 finding Raymond
ruled disproportionate, this one has a nameable trigger, a misleading user-visible outcome, and a fix
whose shape is already understood. It should be fixed -- in the PR that owns the mechanism.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 53 -- two reviewers read the SAME ordering oppositely, which settles what round 52 is

@buluoray re-reviewed at fad0a3abc and returned 0 blocking a second time, re-tracing the path
rather than trusting his earlier review. My round-51 push dismissed that review too. That is twice
now, and it is my doing both times: he reviews the head, I push, GitHub dismisses him. I am stopping
pushing on this branch for that reason among others -- his verdicts are worth more than my comment
corrections were.

The reason I am posting this separately is that his review contains something that decides the
character of the finding in round 52, and it would be lost with a dismissed row.

He verified, as a PRESERVED SAFETY PROPERTY:

The sentinel-file, runtime-budget and approval-stall stop paths remain above the gate.

GPT, on the byte-identical logic one head later, called that exact ordering the defect:

Terminal result -> refused channel fire -> budget expires during retry backoff -> _timer
deactivates before delivering the terminal turn. Fix: process terminal_pending retries before cap,
runtime-budget, and approval-stall checks.

Both descriptions of the ordering are correct. They disagree about whether it is right. One reviewer
counts "the stop paths run first" as the guarantee that a runaway loop stops; the other counts it as
the reason an owed terminal turn is lost. That is not a bug report with an obvious patch -- it is an
unresolved question about which invariant wins
, and answering it changes the meaning of max_cycles
that the same reviewer verified in the same paragraph as the runaway backstop.

This is the clearest argument yet for decision (a), split the channel terminal delivery into its own
PR: the mechanism has taken fifteen findings, five of them inside the previous round's remedy, and now
two independent competent reviewers reading one ordering in opposite directions. A sixteenth patch
written at round 53 of a gating PR would be picking one side of that question silently.

Two smaller things from his review, for the record:

  • His finding 1 (gate has no frontend producer) now reads "ship as forward-wiring, or add the
    popover producer in a follow-up" -- the earlier "consider dropping it" is gone. My round-50
    disposition declined the subtraction for a different reason (deleting the parse would make an
    explicit gate: true from a REST caller silently ignored), and I still offer to drop the field if
    he wants it gone.
  • His finding 2 is decision (d) again. Three independent sources now name it: the Design lane, both of
    his reviews, and the fifth claim-bookkeeping finding that landed in it.

He notes he could not confirm the description's assertion count because backend lanes were still in
flight, and that bolichen97's earlier review cited failing Windows shards. On the current head those
shards are green: the only failing check is the GPT lane carrying the round-52 finding I declined to
patch. Nothing else is red.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

/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.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for a9417cca687205421882307fb9ccc57e7fcf464f.

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.

This decision applies only to this commit. A new push requires a new judgment.

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/-4
  • src/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:

  1. autonudge.py (~L662): removed a stale sentence that contradicted the retained block. The code still normalizes a present-but-non-bool / null gate to False (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.
  2. monitoring/models.py (~L226): tightened the counter docstring to say wakes counts turns actually DELIVERED (not every non-quiet tick), with undecidable gates charged to gate_fallbacks and refused fires to neither. Fields quiet_ticks, wakes, gate_fallbacks, floor_ticks all 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 gh as a subprocess offloaded to a thread (25s timeout), no model call.
  • Fail-safe direction: _monitor_tick_is_quiet returns True (skip) only on a definite quiet verdict; no-monitor, no-probe, un-inferable target, probe defect, no-kernel-verdict, and target drift all return False and fire as today.
  • Default / opt-out path unchanged: if not loop.gate: return False fires exactly as before, so an ungated loop is unaffected.
  • Starvation bound: _MAX_QUIET_STREAK = 10 floor still delivers on a quiet streak.
  • max_cycles backstop 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 reports stopped_reason=cycle_cap for 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 gate field with zero producers, Target.host_key/same_host whose 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.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 54 -- a second description/code mismatch, same class as @bolichen97's, found in the body's own correction

@bolichen97 Your Description / code mismatch review is addressed and the three items are live in
the body: the trigger now reads "names exactly one pull request BY EXPLICIT PUBLIC URL --
https://github.com/<owner>/<repo>/pull/<N>" and states that a bare owner/name#123 leaves the loop
ungated, the conditional host-pin paragraph is deleted rather than reworded, and the section-4 bullet
names the assertion that actually exists.

While re-checking that, I found a SECOND mismatch of the same class -- in a sentence the body had
already written about this very change, which is why it survived earlier rounds:

max_cycles now bounds wakes rather than ticks for a gated loop

That is wrong, and it understates the cap. The cap is charged where a turn is CONFIRMED
DELIVERED (autonudge.py:2855, inside the delivery path), so a follow-up turn, a streak-floor turn
and a gate-fallback turn each consume one too -- not only wakes. The MCP tool's own text
(mcp_tools/control.py:962, "the cap counts delivered turns") and the spec both already said
"delivered turns"; only the PR body said "wakes". A reader sizing max_cycles from the description
would have budgeted for wakes alone and been surprised by the floor and the follow-up.

The body now states delivered turns and enumerates all four consumers, and notes that wall-clock is
bounded separately by max_runtime_secs, which this change does not touch.

I also added the upgrade-safety statement the body never made, because "what does this do to my
running watches" is the first question a behaviour change like this should answer:

What changes for a loop that is already running: nothing. gate defaults to false on the
dataclass, on add(), on the authorizer and on POST /api/autonudge; only monitor_start's own
directive defaults it true. A record persisted before this change carries a monitor dict with no
gate key, so it decodes to false and the tick's own guard keeps that loop on the plain timer.

That is verified two ways: NudgeLoop.gate: bool = False with the tick's independent
if not loop.gate: return False guard, and a pod run where a loop record with no gate key showed
poll_called=False and fired on every tick.

No code changed and no push -- this is a body edit on the same head a9417cca6. Editing the body
re-triggers the GPT lane on that unchanged SHA, and a re-run does not honour the SHA-pinned override
marker, so if it re-blocks on the terminal-delivery finding already accepted at
#8060 I will re-post the same override on the
same SHA rather than treat it as a new finding.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 55 -- Opus's ack finding is real, filed as #8100, deliberately not pushed here

All five lanes are green on a9417cca6 and the body edit did NOT cost anything: the GPT marker now
reads "human override accepted" for this SHA, updated after the edit, so the re-roll honoured the
pinned marker rather than re-reviewing from scratch.

Opus passed as a check but its body carries a real finding, and I verified it in source rather than
taking the check as the verdict. mcp_tools/control.py adds
f"cost no turn and the {max_cycles or 0} cap counts delivered turns" unconditionally on the gated
branch, and a later fragment adds ", with NO cycle cap" when max_cycles is falsy. An explicit
max_cycles=0 -- which monitor_start's own description documents as "unlimited" -- therefore renders
both, so one sentence tells the agent that a 0 cap counts delivered turns AND that there is no cap.
That string is what the model reads back to learn the loop's semantics, on the parameter that bounds
unattended spend, so it is worth fixing.

Filed as #8100 with the one-line remedy and a
test worth adding, rather than fixed here.

Why not fixed here, when round 51 pushed for exactly this class

Round 51 pushed for two wrong statements in shipped comments, so declining now would look like a
reversal. It is not the standard that changed, it is the price. At round 51 there was no accepted
override and no standing approval; a push cost a re-roll and nothing else. Today a push would void the
SHA-pinned override on this head AND dismiss @buluoray's approval -- and his two earlier reviews on
this PR were already dismissed by my own pushes, which is twice that a reviewer did a full independent
trace and had it thrown away by me. Spending a third one on a one-line string, when the defect has a
tracked issue and a known remedy, is the wrong trade.

If a push becomes necessary here for any other reason, this fix goes in that push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants