fix(autonudge): let members arm their own loops; report arm outcome - #8919
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound fix at the right chokepoint for arming, but the crew/member update rule is enforced in one caller instead of the authorizer — future callers bypass it silently. Watch
Suggestions
[DESIGN-REVIEWED] 447595b |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence is gathered: the full 2980-line patch, the intent file, and repository verification of consumers ( First-Principles-Verdict: CONCERNS The ✅ success notice rides along in a refusal-visibility fix — the transcript's tool_result row already said "Monitor loop started" before this PR. What this change shipsIntent: let a crew/member session schedule its own wake (real incident: conductor never woken) and make a refused arm visible — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 447595b |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
41dfd38 to
3332652
Compare
Disposition — review round 1 (head
|
3332652 to
c80021e
Compare
c80021e to
6ba1e3c
Compare
Disposition — review round 2 (head
|
b916a2c to
a4521c1
Compare
|
/ai-review override gpt a4521c1: The bypass chain named (failed revoke keeps a stale entry, forged same-id row reuses it) is the revocation-OSError residual already judged on this PR: it needs a trust-root write failure AND a forged store row — the agent-writable-store class tracked in #8980. The unreadable-record half is availability-only: a corrupt trust file already refuses every wake (total readers), so an upsert rebuilding it opens no bypass. Coupling store removal to trust-file I/O is the #8980 redesign, not this fix. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
a4521c1 to
9c07790
Compare
|
fire_immediately dropped per maintainer request |
9c07790 to
a66671d
Compare
Disposition — review round 5 (head
|
Disposition — review round 6 (head
|
|
@bolichen97 — thanks; taking the three items in order. Your review pre-dates two pushes, so the state has moved:
Current head |
|
@bolichen97 — responding to review 5125901086 point by point, then the owner decision it asked for.
Agreed, and since your review it was tightened further at the GPT lane's request: the persisted
Resolved by removal, not sign-off: the owner asked for
@kyleseaman was pinged on this thread (comment 5562718621) with what the guard keeps and the one exception it now admits. The owner decision below records the product intent that #5184's blanket refusal did not distinguish.
Filed as #9142 (admit the self-armed loop's own delivered wake as a second explicit producer — not a widening of the blanket rule) and linked in the PR body under "Follow-up". Owner decision (CrysisDeu): member self-armed loops are intended product behaviour — a crew member is an autonomous long-running role and must be able to schedule its own wakes; external injection into member threads stays refused. Provenance: #5184 introduced the blanket refusal without distinguishing self-arm from external arm; see #8908. Current head |
|
/ai-review override gpt 521aad0: False positive: trust/autonudge-self-armed.json lives under the data home's trust/ directory, which is already a gateway-only OS-masked leaf (sandbox._CREW_HIDDEN_LEAVES lists "trust") AND on the file-tool sensitive floor (security._CREW_SECRET_LEAVES lists "trust") — the same fence as the SEL HMAC key, member DM bindings and member rules. The record is exactly the gateway-only masked leaf the finding prescribes; agent tools can neither read nor write it. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
Follow-up to the owner decision above (comment 5565825909) — the review's third point is now fixed in this PR, not deferred:
Owner ruling: the admission condition is "initiator == the target slot itself", whether the turn was started by a human or by the slot's own nudge/monitor wake. Implemented as a second explicitly named producer rather than a blanket "any turn in the session", so the GPT round-2 boundary (cron / app / sub-agent injections into a member thread stay refused) is untouched:
@kyleseaman — for the record, this is the shape of the exception to #5184's guard: the guard's refusal of outside injection into a member thread is kept verbatim; what is admitted is the member's own turn (human-started, or its own loop's wake) arming a loop on the member's own slot. #5184's description did not distinguish self-arm from external arm, which is the gap #8908 records. Head |
… outcome The crew/member guard in autonudge_authz (from #5184 "expose session monitors to agents", both sites) refused EVERY arm on such a slot ("<mode>-mode sessions do not accept direct automation turns"), including the member's own monitor_start. The MCP tool had already answered "requested", so nothing told anyone: the conductor member thread armed its patrol loop, ended its turn, and was never woken again -- autonudge.json never held the loop. #5184's intent (no outsider injects automation turns into a member thread) is right; refusing the member's own arm is the side effect this fixes. Self-arm exception - authorize_and_add_nudge / authorize_and_update_monitor take initiator_slot_key; is_self_arm() admits only a non-blank exact match. Only the session-directive consumer passes it (it applies a directive to the exact session whose turn produced it); REST, workflow ctx.nudge and app callers stay refused with 409. - A self-arm emits a distinct SEL "self_armed" outcome and is persisted as NudgeLoop.self_armed so the fire-time mode re-check in _fire_dashboard_nudge lets a self-armed member loop wake while still refusing an externally armed loop on a slot that switched into crew mode. - The arm-time admission check for a self-armed loop requires the slot mode to be unchanged between authorization and commit. Arm outcome reported on two channels - The applier's return (which overwrites the transcript tool_result row) names the loop id and the first wake time on success, and carries the status code on refusal ("… [status 409]"). - A "notice" row is appended to the session on both outcomes: "Automation loop armed: loop <id> · every 20 min · no cycle cap · first wake in ~5s" / "Automation loop NOT armed: <reason> [status N]". - The MCP tool's ack now says how the outcome is reported instead of "requested". fire_immediately (immediate first wake) was dropped at the maintainer's request; monitor_start keeps its historical first-fire timing. Tests (written, not run locally per repo rule) cover self-arm admitted, external still refused, both notice channels, the status code, the immediate first deadline and the tool payload shape. Spec updated in learn-cron-dashboard.md. Review hardening (protective direction only) - Self-arm provenance is supplied only for human-started turns (producer_is_user_facing); cron/sub-agent/app/nudge turns in a member session pass an empty initiator and stay refused. - The persisted self_armed bit is a hint, not authorization: _load() normalises non-booleans, the fire-time guard compares `is True` AND requires the keystone-gated trust record trust/autonudge-self-armed.json (new autonudge_selfarm.py) written by the authorizer, fail-closed. - Ratchet test: only session_directive_apply.py may pass initiator_slot_key. - Fire-time boundary applies to every dashboard loop (prompt loops too) and refusals are SEL-audited; trust entry revoked on loop removal; record transaction runs under an exclusive lock file. - Trust entry revoked only after the store commits the removal (#8980 tracks payload authentication of the loop store as a separate design question). - Self-arm trust entry is written BEFORE svc.add with a pre-minted loop id (fail-closed with the store untouched; a refused add forgets the entry). - The pre-minted self-arm id is reserved collision-free against the store before the trust entry is written. - Owner ruling: the slot's own loop wake counts as the slot itself. A second self-arm producer (_directive_self_wake, set only by _fire_dashboard_nudge) lets a member re-arm or revise its loop from inside its own cycle; cron, app and sub-agent turns still carry no provenance (closes #9142).
Motivation (real incident)
The conductor member thread
member-kirocrew-conductorhas not been woken since last night. Root cause: the guard insrc/kiro_crew/autonudge_authz.py(if mode in {"crew","member"}: "<mode>-mode sessions do not accept direct automation turns", present at both sites —authorize_and_update_monitor~:115 andauthorize_and_add_nudge~:670, same origin) refuses everymonitor_starton a crew/member-mode slot — including the member's own. Meanwhile the MCP tool layer answers the caller with "requested" (arming is applied asynchronously by the session-directive consumer), so the caller has no signal, and the loop store never held the loop.Where the guard comes from: #5184 "feat: expose session monitors to agents" (Kyle Seaman, commit
76196b6cd, 2026-09-03). Its intent is right and is kept: no cron, other session or app may inject automation turns into a member's own thread. The side effect it did not intend is that a member's own arm is refused too — a product contradiction, since a crew member is a self-directed resident agent, and forbidding it from scheduling its own wake means a member that is never woken.What changes
1. Self-arm exception (authorizer) — external injection still refused
authorize_and_add_nudge/authorize_and_update_monitortake a newinitiator_slot_key.is_self_arm(slot_key, initiator_slot_key)admits only a non-blank exact match (blank never matches blank, so a caller that failed to resolve the target cannot self-arm by accident).session_directive_apply.py:_monitor_start,_monitor_watch,_structured_monitor_update) passes it — it applies a directive to the exact session whose turn produced it, so its binding IS the initiator. REST, workflowctx.nudgeand app callers pass nothing and stay refused with 409.self_armedoutcome (reusing the file's existing_auditpattern);self_armed=Truerides in the invoked/success metadata.NudgeLoop.self_armed+ fire-time re-check:slack/gateway.py::_fire_dashboard_nudgere-checks the slot mode before a structured wake enters the provider. Without a record it cannot tell "this slot was a member when its own turn armed the loop" from "this slot switched into crew mode after an outsider armed it". Persisted for the same reasongateis (a restart re-arms every loop); absent decodes to False.2. Arm outcome reported on two channels (no more bare "requested")
The MCP tool is stateless (#755): it answers over its own pipe before the directive is applied, so the in-band tool result cannot carry the outcome by construction. The outcome is reported where it is known, on both channels:
tool_resultrow): success →Monitor loop <id> started on this session: … ; first wake in ~Ns (HH:MM:SS UTC)(read off the ARMED record'snext_due_ts, a full interval after arming); refusal →Failed to start monitor loop: <reason> [status 409|404|503].noticerow appended to the session viaappend_and_surface:✅ Automation loop armed: loop <id> · every 20 min · no cycle cap · first wake in ~1200s (…)or⚠️ Automation loop NOT armed: <reason> [status N](redacted). Slot-less channelTurnDrivercallers keep the string as their surface. Best-effort: never masks a denial or fails an arm.3. Member/crew self-arm rule
Exactly: initiator == target slot ⇒ admit; anything else ⇒ refuse (unchanged from the first revision). Hardened after review, all in the protective direction:
producer_is_user_facing, the flag theset_projectgate already uses), and the delivered wake of a loop bound to this very slot (producer_is_self_wake, set only by_fire_dashboard_nudgevia_run_chat(_directive_self_wake=True)). A member re-arming or revising its loop from inside its own cycle is therefore admitted — the conductor pattern. A cron injection, a sub-agent sharing the slot or an app-driven turn carries neither mark and stays refused (visibly, via the notice row). The wake mark never unlocksset_project/reset_conversation._load()normalises a non-booleanself_armedtoFalse, the fire-time guard comparesis True, and it ALSO requires the keystone-gated trust recordtrust/autonudge-self-armed.json(newautonudge_selfarm.py;trust/is on the sensitive-path floor so agent file tools cannot reach it) to name the loop id on that slot. The authorizer mints the loop id, writes the record BEFOREsvc.addand fails closed (503, store untouched — a displaced stopped loop survives); a refused add forgets the pre-written entry if it cannot. A booleantrueforged into the agent-writableautonudge.jsonhas no trust entry and refuses. Entries are pruned to live loop ids on every write.session_directive_apply.pyever suppliesinitiator_slot_key=; the structured-update path passes the SESSION's binding, not the loop's own key echoed back.messagein the agent-writableautonudge.jsoncan be rewritten out-of-band, member or not) — an architecture question for the whole loop store, filed as AutoNudge loop store is agent-writable: authenticate loop payloads (follow-up to #8919) #8980 and overridden here rather than bolted onto the self-arm exception.monitor_fire/denied); the trust entry is revoked when its loop is removed or replaced (remove_sync→forget_self_arm); the record's read-prune-write runs under an exclusive lock file so concurrent self-arms cannot drop each other's entries.Scope note: the earlier
fire_immediately(immediate first wake) item was removed at the maintainer's request;monitor_startkeeps its historical first-fire timing (a full interval after arming).#9142 resolved here: the autonomous re-arm gap the Design Review named is closed in this PR by the second producer above (the owner ruled that the slot's own wake counts as the slot itself).
Declared partial: the refusal notice covers the two ARMING directives (
monitor_start,monitor_watch) — the ones whose silent refusal leaves a session that is never woken.monitor_update/monitor_stop/autonudge_stopshare the "tool ack'd before the consumer ran" shape but a refused update or stop leaves an already-visible loop in place; extending the notice to them is a follow-up, not this fix.Pattern harvest
Rule candidate: ratchet test
Pattern: a security boundary whose relaxation is decided by a caller-supplied claim (
initiator_slot_key) must pin WHO may supply it — scan the source tree for the kwarg and fail on any call site outside the one module that owns that provenance. Second candidate (semgrep): a persisted boolean that relaxes a guard read from an agent-writable store must be normalised at load and comparedis True, never by truthiness (gatealready had this;self_armedgained it only on review).Tests (
test/test_autonudge_member_self_arm.py, written, not run locally per repo rule): (a) self-arm admitted formonitor_start/monitor_watch/ structured update incl. audit + kwargs + admission closure + store round-trip; (b) external arm still refused (no initiator, other-session initiator, REST/workflow sources) and the external kwargs shape unchanged; (c) refusal writes the notice row with[status 409], success writes the notice row naming loop id + first wake, slot-less returns only the string, notice failure never masks the denial; fire-time re-check admits self-armed and refuses externally armed.Spec:
docs/system-specs/modules/learn-cron-dashboard.md→ Structured monitors (self-armed exception, #5184 provenance, refusal visibility) and the AutoNudge paragraph (arm-outcome reporting on two channels).Verified locally
black / isort / flake8 clean on all touched files; mypy error count on the touched files unchanged vs main (0 new). pytest/vitest deliberately not run locally — CI owns test execution.