fix(autonudge): treat a merged subject as terminal when a paused loop's cap is raised - #8201
Conversation
…ent bound A channel-bound monitor loop does not settle on observation: the probe records the owed final turn in `monitor.terminal_pending` and leaves the loop active with no `outcome`. If that turn is refused because the channel is busy -- the ordinary case for a live thread -- and the retry finds a bound spent, the loop deactivates tagged with that bound before the settlement that would promote the debt ever runs. The expiry notice already consults the debt for its wording. The `monitor_update` paused-loop branch did not: it read `stopped_reason` alone, so the same loop was reported to the agent as "it hit its cycle cap", and a settled terminal loop as "it was paused manually". A patch that also raised the bound then satisfied the revival condition and injected `active=True`, re-arming a watch on a subject that had already merged -- the wasted fresh loop this branch exists to prevent. That branch now derives the terminal reading the same way the notice does -- `stopped_reason == monitor_terminal` or an outstanding `terminal_pending`, with the merged-vs-closed-unmerged distinction taken from the settled `outcome` falling back to the debt, which speaks the same `success`/`blocked` vocabulary -- and a terminal subject is refused with that news instead of revived. The precedence is expressed once, as a term in the revival decision, rather than as a guard per branch: the notice next door lost this same precedence three times because each newly added bound was evaluated ahead of it. Deliberately unchanged: no stop check moves, `max_cycles` semantics and `stopped_reason` stay untouched so the spent bound remains observable, the revival affordance for a genuine cap or budget is preserved (covered by a control test), and no field or schema is added. Whether an owed terminal turn should instead outrank the cap and be DELIVERED is the separate contested half of the issue and remains an open maintainer decision. Refs kirodotdev#8060 Refs kirodotdev#8122
github-actions
Bot
added
fork
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound fix, but it unifies two surfaces by hand-copying the derivation — re-seeding the exact "only one copy gets updated" failure it fixes. WatchThe terminal-precedence derivation ( Suggestions
[DESIGN-REVIEWED] 859bb93 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All the evidence is in. The fix is real and its zero option costs a re-armed watch on a merged PR — but the new code is a line-for-line second spelling of the derivation First-Principles-Verdict: CONCERNS The fix earns its place, but it ships as a second inline copy of the exact terminal/decided derivation the expiry notice already carries — the divergence the PR itself warns about. What this change shipsIntent: stop a cap/budget raise from re-arming a monitor loop whose subject already finished, and word the refusal with the real ending. A FIX.
WatchGrepped Subtractions
[FIRST-PRINCIPLES-REVIEWED] 859bb93 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
github-actions
Bot
added
readiness: passed
iamwhatever
left a comment
iamwhatever
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: monitor_update no longer revives a paused loop whose subject is already terminal -- terminality (a settled monitor.outcome, or the owed terminal_pending turn a channel loop records before settlement) is now a term in the revival decision itself, so raising a cap or budget cannot re-arm a watch on a subject that already merged; the second file is its test. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.
github-actions
Bot
removed
the
readiness: passed
2 participants
Problem / Motivation
Merged #8122 fixed the user-facing expiry notice: it now derives
terminalfromstopped_reason == monitor_terminalor an outstandingmonitor.terminal_pending. It deliberately left_timer's ordering,max_cyclessemantics andstopped_reasonalone.monitor_update's paused-loop branch insrc/kiro_crew/dashboard/session_directive_apply.pystill readstopped_reasonon its own. Two consequences on a loop whose subject has already reached a terminal state:max_cycles, the branch injectsactive=Trueand silently re-arms a watch on an already-merged pull request.Why it matters
The second one costs real work: a fresh loop wakes repeatedly to watch something that is finished, and nothing in the reply says the subject already merged. It is also a disagreement between two surfaces reading the same state — the notice says merged, the directive says out of cycles.
What changed
The paused-loop branch now reads the same two fields the expiry notice reads, with the same precedence, so the agent-facing and user-facing endings cannot diverge:
outcomeoutranks a stale owed turnThe reply now names the actual ending (merged, or closed without merging) instead of a bound that was not what stopped it.
Scope:
_timerdelivery,max_cyclessemantics andstopped_reasonitself are untouched, which is why this isRefsrather thanCloses— the contested delivery half of the issue remains open.Tests
test/test_autonudge_stop_auth.pygains five tests (it already owns the paused-loop revival and denial cases):Pre-fix, four fail: the loop is revived with
active: Trueon a merged subject, and a settled terminal loop reports "paused manually". After:50 passed. Neighbouring directive suites:100 passed. black, isort, flake8 clean;mypy 1.14.1reports no issues in 1279 source files.Manual verification
None beyond the tests — the behaviour is a directive reply string and a patch payload, both asserted directly.
Screenshots / video
Why no screenshot: a backend directive-reply change; no UI surface is touched.
Related Issues
Refs #8060
Not
Closes: this fixes themonitor_updatehalf. The_timerdelivery-ordering question the issue also raises is untouched and should keep the issue open.Pattern harvest
Rule candidate: review-prompt
Pattern: two surfaces deriving the same user-visible verdict from the same state, where only one is updated. The fix that lands in the notice leaves the directive reading a single field, and the two then disagree about the same loop.