Skip to content

fix(autonudge): let members arm their own loops; report arm outcome - #8919

Merged
iamwhatever merged 1 commit into
mainfrom
fix/autonudge-member-self-loop
Sep 8, 2026
Merged

fix(autonudge): let members arm their own loops; report arm outcome#8919
iamwhatever merged 1 commit into
mainfrom
fix/autonudge-member-self-loop

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Motivation (real incident)

The conductor member thread member-kirocrew-conductor has not been woken since last night. Root cause: the guard in src/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 and authorize_and_add_nudge ~:670, same origin) refuses every monitor_start on 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_monitor take a new initiator_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).
  • Only the session-directive consumer (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, workflow ctx.nudge and app callers pass nothing and stay refused with 409.
  • A self-arm emits a distinct SEL self_armed outcome (reusing the file's existing _audit pattern); self_armed=True rides in the invoked/success metadata.
  • The arm-time TOCTOU admission check for a self-armed loop requires the slot's mode to be UNCHANGED between authorization and commit; the external rule ("never into crew/member") is unchanged.
  • Persisted NudgeLoop.self_armed + fire-time re-check: slack/gateway.py::_fire_dashboard_nudge re-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 reason gate is (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:

  • Applier return (overwrites the transcript tool_result row): success → Monitor loop <id> started on this session: … ; first wake in ~Ns (HH:MM:SS UTC) (read off the ARMED record's next_due_ts, a full interval after arming); refusal → Failed to start monitor loop: <reason> [status 409|404|503].
  • notice row appended to the session via append_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 channel TurnDriver callers keep the string as their surface. Best-effort: never masks a denial or fails an arm.
  • The tool's own ack now states how the outcome is reported (transcript notice + applier result) instead of a bare "requested".

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:

  • Which turns are "the session's own" (owner ruling: initiator == the slot itself): two producers, each explicit — a turn a HUMAN started in this session (producer_is_user_facing, the flag the set_project gate already uses), and the delivered wake of a loop bound to this very slot (producer_is_self_wake, set only by _fire_dashboard_nudge via _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 unlocks set_project / reset_conversation.
  • The persisted bit is a hint, not authorization: _load() normalises a non-boolean self_armed to False, the fire-time guard compares is True, and it ALSO requires the keystone-gated trust record trust/autonudge-self-armed.json (new autonudge_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 BEFORE svc.add and fails closed (503, store untouched — a displaced stopped loop survives); a refused add forgets the pre-written entry if it cannot. A boolean true forged into the agent-writable autonudge.json has no trust entry and refuses. Entries are pruned to live loop ids on every write.
  • A ratchet test fails if any module other than session_directive_apply.py ever supplies initiator_slot_key=; the structured-update path passes the SESSION's binding, not the loop's own key echoed back.
  • Round 4: revocation of the trust entry now happens only after the store has committed the removal (a failed save leaves a still-stored loop with the entry it needs to fire). The GPT finding that the trust record does not authenticate the loop payload is real but pre-existing and store-wide (any loop's message in the agent-writable autonudge.json can 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.
  • Round 3: the fire-time boundary applies to EVERY dashboard loop (prompt loops too, not only the structured/gated ones the completion hook covers) and a refusal is SEL-audited (monitor_fire / denied); the trust entry is revoked when its loop is removed or replaced (remove_syncforget_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_start keeps 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_stop share 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 compared is True, never by truthiness (gate already had this; self_armed gained it only on review).

Tests (test/test_autonudge_member_self_arm.py, written, not run locally per repo rule): (a) self-arm admitted for monitor_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.

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

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

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

  • The legacy monitor_update mode gate lives in session_directive_apply.py:672-688 ("the mode rule has to be applied HERE"), not in authorize_and_update_nudge. The chokepoint can resolve the loop's slot and mode itself, so it could enforce the rule with the same initiator_slot_key pattern the two arm chokepoints use; as shipped, any future caller of authorize_and_update_nudge (workflow, app handler) can rewrite a member loop's message — the instruction every future wake executes — with no provenance check and nothing going red. The ratchet test pins who may claim self-arm, not who must apply the mode rule.

Suggestions

  • Move the legacy update mode check into authorize_and_update_nudge (loop → slot mode → is_self_arm), letting the REST handler pass its own human-provenance marker; that restores the enforced-at-the-chokepoint symmetry the arm paths have.

[DESIGN-REVIEWED] 447595b

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 447595bd84878adff8284256b7c0cc67ba62d2c7 — 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.

All evidence is gathered: the full 2980-line patch, the intent file, and repository verification of consumers (initiator_slot_key supplied only by the directive consumer, self_armed consumers, no frontend consumer of the notice prefixes, the tool_result overwrite broadcast at chat_runner.py:8462, and the pre-change success string visible in the removed hunk).

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 ships

Intent: let a crew/member session schedule its own wake (real incident: conductor never woken) and make a refused arm visible — a FIX.

  1. A member/crew session's own monitor_start/monitor_watch now arms; outsiders still 409 — justified
  2. Refused arm writes a ⚠️ notice row into the session transcript — justified
  3. Successful arm also writes a ✅ notice row — rides along
  4. Applier result gains first-wake time and [status N] — justified
  5. MCP tool ack reworded to say where the outcome lands — justified
  6. The slot's own delivered wake may re-arm/revise its loop — justified (Admit a self-armed loop's own wake as self-arm provenance (follow-up to #8919) #9142, owner-ruled)
  7. A member slot's legacy loop revisable only by its own turn — justified (closes the surface item 1 opens)
  8. Every dashboard loop mode-checked at fire time; refusal SEL-audited — justified (agent-untrusted-ceiling boundary)
  9. Persisted self_armed bit + keystone trust/autonudge-self-armed.json — justified (same boundary; forgeable-store constraint)
  10. Spec updated in the same commit — mandated

Watch

  • Item 3's zero option is the pre-PR status quo: the removed hunk already returned "Monitor loop {id} started on this session: {cadence}…", overwriting the transcript tool_result row (broadcast at chat_runner.py:8462), and nobody reported successes as invisible. "No more bare 'requested'" overstates — the bare ack was only the in-band tool return, which remains (reworded). Only the refusal side removes the named harm.
  • Declared partial is honest: 3 counted siblings (monitor_update/monitor_stop/autonudge_stop) share the ack'd-before-applied shape; a refused update/stop leaves a visible loop, so deferral is grounded.

Subtractions

  • Drop _surface_arm_success, ARM_SUCCESS_NOTICE_PREFIX, and the two call sites in _monitor_start/_monitor_watch (session_directive_apply.py) — keep the refusal notice, whose harm is the incident.

[FIRST-PRINCIPLES-REVIEWED] 447595b

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 447595b

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

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

@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 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 447595bd84878adff8284256b7c0cc67ba62d2c7 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 447595b

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

@CrysisDeu
CrysisDeu force-pushed the fix/autonudge-member-self-loop branch from 41dfd38 to 3332652 Compare September 6, 2026 07:45
@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 6, 2026
@CrysisDeu CrysisDeu changed the title fix(autonudge): let crew/member slots arm their own loops fix(autonudge): let members arm their own loops; report arm outcome Sep 6, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition — review round 1 (head 333265227 → next push)

GPT 5.6 BLOCKING (gateway.py:6212, unvalidated persisted self_armed) — accepted, fixed in the protective direction: _load() normalises a non-boolean persisted self_armed to False with a warning (mirrors gate), and the fire-time guard now compares getattr(loop, "self_armed", False) is not True, never truthiness. Pinned by test_load_normalises_a_non_boolean_self_armed_to_false and TestFireTimeGuardIsNotTruthinessBased (forged "false", "true", 1 all refuse).

GPT 5.6 FINDING (authz.py:735, derive self_armed before the mode branch) — declined; it loosens a security boundary. Setting self_armed=True for every self-arm would let a default-mode session's loop follow that session INTO crew mode and keep firing there, which is precisely the case the fire-time re-check exists to stop (test_monitor_rechecks_dashboard_mode_before_provider_entry). The bit means "this slot was crew/member when its own turn armed the loop", nothing wider. A default-mode session that later becomes a member re-arms from its own turn and is admitted then.

Design Review — provenance is honor-system — accepted: added a ratchet test (test_only_the_session_directive_consumer_passes_initiator_slot_key) that scans src/kiro_crew and fails on any initiator_slot_key= call site outside dashboard/session_directive_apply.py; the structured-update site now passes the SESSION's binding handed down from _monitor_update, not loop.slot_key echoed back.

Design Review — self-armed fires after a later mode change — decision stated in the spec: the fire-time guard still requires the same slot object and persistent memory mode, member slots have their mode pinned at every writer, and a crew slot that armed itself is still the session that asked to be woken. The arm-time "mode unchanged" rule closes a millisecond TOCTOU window, not a policy on future mode changes.

Design Review + First Principles — fire_immediately default-on is a fleet-wide default change — kept, now declared as such in the PR body ("a changed default for every monitor_start caller, deliberately"). This is the owner's product decision for this fix: an armed loop must prove it is alive on cycle one, because a silent arm was the incident. Cost is one turn per arm, none per later cycle. Simplification taken: first_fire_delay_secs: int | None threaded through three layers is now a fire_immediately: bool with the constant applied inside AutoNudgeService.add.

First Principles — drop _surface_arm_success — declined: the owner asked for the outcome on two channels (transcript row + notice) on both success and failure; the tool_result row alone is folded away in the dashboard's collapsed view, the notice row is not.

First Principles — refusal notice covers 2 of 9 directive kinds — declared as partial in the PR body: the arming directives are the ones whose silent refusal leaves a session never woken; a refused update/stop leaves an already-visible loop in place. Follow-up, not this 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 6, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/autonudge-member-self-loop branch from 3332652 to c80021e Compare September 6, 2026 08:27
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/autonudge-member-self-loop branch from c80021e to 6ba1e3c Compare September 6, 2026 09:08
@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 6, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition — review round 2 (head c80021efd6ba1e3c35)

Both GPT 5.6 BLOCKING findings accepted and fixed in the protective direction; nothing was loosened.

F1 — session_directive_apply.py:371, headless turns treated as self-arms. Accepted. The consumer now supplies initiator_slot_key only when producer_is_user_facing is set — the same authenticated-human provenance flag the set_project / reset_conversation gate already uses, so the two boundaries cannot drift apart. A cron injection, a sub-agent sharing the slot, an app-driven or nudge-driven turn in a member session passes an empty initiator, is_self_arm returns False, and the crew/member refusal stands (visibly, via the notice row). Pinned by test_a_headless_turn_in_a_member_session_gets_no_self_arm_provenance (both arming kinds) and test_structured_update_provenance_also_requires_a_human_turn.

F2 — autonudge.py:852, a forged boolean true in the agent-writable store survives _load. Accepted. The persisted bit is now a hint, not authorization: the fire-time guard requires loop.self_armed is True AND a matching entry in a new keystone-gated trust record trust/autonudge-self-armed.json (autonudge_selfarm.py; trust/ is already on security._SENSITIVE_HOME_DIRS as a whole directory, so agent file tools can neither read nor write it — the same fence as member DM bindings and member rules). The authorizer writes the entry right after svc.add returns for a self-arm and fails closed if it cannot (removes the loop, denies 503). A forged true has no trust entry and refuses; entries are pruned to live loop ids on every write. Pinned by TestSelfArmTrustRecord (round-trip, id+slot both required, pruning, total readers), TestFireTimeGuardRequiresTheTrustRecord (bit True + no entry ⇒ refused) and test_self_arm_fails_closed_when_the_trust_record_cannot_be_written.

Same-span note for the stall rule: round 1 was gateway.py:6212; round 2 is two new spans (session_directive_apply.py:371, autonudge.py:852). No span has repeated.

@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Sep 6, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/autonudge-member-self-loop branch from b916a2c to a4521c1 Compare September 6, 2026 17:39
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

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

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

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

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.

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

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/autonudge-member-self-loop branch from a4521c1 to 9c07790 Compare September 6, 2026 19:12
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

fire_immediately dropped per maintainer request

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 6, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/autonudge-member-self-loop branch from 9c07790 to a66671d Compare September 6, 2026 19:59
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 6, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition — review round 5 (head 9c077907ea66671de7)

GPT 5.6 BLOCKING — autonudge_authz.py:924, a failed trust write deletes the displaced loop. Accepted, fixed by reordering rather than by adding a restore path. The authorizer now mints the loop id itself (uuid4().hex[:8]), writes the keystone-gated self-arm entry before svc.add, and hands the id to the service (add / add_monitor gained an optional loop_id; _mint_loop_id refuses an id already in use as a conflict rather than silently re-minting, since the record would then name the wrong loop). Consequences:

  • A failed trust write denies 503 with the store untouched — the stopped loop this arm would have displaced is still there. No remove, no rollback needed.
  • If the add itself refuses (NudgeAdmissionRefused, MonitorUpdateConflict, or an unexpected error), the pre-written entry is forgotten best-effort.
  • The previous post-add liveness re-check is gone because its race no longer exists: a concurrent removal of the new loop now runs after the entry exists, so remove_sync's revoke finds and drops it.

Pinned by test_self_arm_fails_closed_before_the_store_is_touched (nothing added, nothing removed), test_a_refused_add_forgets_the_pre_written_trust_entry (both refusal types), test_a_successful_add_keeps_the_pre_written_trust_entry, test_service_refuses_a_pre_minted_id_already_in_use, and the self-arm admitted test now asserts the recorded id equals the id the service was handed.

Also on this head: fire_immediately (immediate first wake) was removed at the maintainer's request in the previous push; the spec and PR body no longer describe it.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition — review round 6 (head a66671de7fcd012727)

GPT 5.6 BLOCKING — autonudge_authz.py:868, an id collision would revoke an existing loop's trust entry. Accepted and fixed rather than overridden, even though the lane itself judged the condition extreme (a 2^-32 event per arm): the authorizer now reserves a collision-free id before writing the trust entry — it re-mints until svc.get_by_id answers absent (bounded at 8 attempts; exhausting them or a store that cannot resolve ids denies 503 with nothing written). An existing loop's entry is therefore never overwritten and the conflict-refusal cleanup can only ever forget the entry this arm wrote. Pinned by test_self_arm_id_reservation_skips_an_id_a_live_loop_already_holds and test_self_arm_denies_when_no_free_id_can_be_reserved.

GPT 5.6 FINDING — spec still described the post-add upsert and a get_by_id liveness re-check. Accepted: that sentence was stale text from an earlier round; it now documents the pre-add ordering, the reserved id, and the refusal cleanup.

Also: the fire_immediately removal (maintainer request) landed clean — all 55 non-review checks were green on a66671de7.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

@bolichen97 — thanks; taking the three items in order. Your review pre-dates two pushes, so the state has moved:

  1. fire_immediately fleet-wide default — removed entirely at the maintainer's request (head 9c077907e, comment "fire_immediately dropped per maintainer request"). monitor_start keeps its historical first-fire timing (a full interval after arming); schema, validation, service path, applier kwargs, tests, spec and PR body no longer carry it. Nothing to sign off any more.

  2. feat: expose session monitors to agents #5184's author@kyleseaman, this PR narrows the crew/member guard your feat: expose session monitors to agents #5184 introduced in autonudge_authz.py (both sites). The guard's intent — no cron, other session or app may inject automation turns into a member's thread — is kept verbatim and hardened (keystone-gated trust record, human-turn provenance, provenance ratchet). The one admitted exception is a member arming a loop on itself from a human-started turn in its own thread, which the guard was refusing and which left the conductor member thread never woken. The PR body's "Where the guard comes from" section and the spec paragraph state this; happy to adjust if it misreads your intent.

  3. Autonomous re-arm gap — filed as Admit a self-armed loop's own wake as self-arm provenance (follow-up to #8919) #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.

Current head fcd012727: 57/57 checks green, MERGEABLE, GPT/Opus/Design/UX/FP all clear. Re-review requested.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

@bolichen97 — responding to review 5125901086 point by point, then the owner decision it asked for.

The self-arm scoping itself is correctly implemented: initiator_slot_key is always the caller's own session binding, cross-member arming is structurally impossible, and there's a ratchet test pinning the single call site. That part is sound.

Agreed, and since your review it was tightened further at the GPT lane's request: the persisted self_armed bit is now a hint only — the fire-time guard also requires the keystone-gated trust record (autonudge_selfarm.py, under the agent-unreachable trust/ root), the entry is written before svc.add under a reserved collision-free id, revoked after the store commits a removal, and provenance is granted only to human-started turns.

  1. fire_immediately defaults to true for every monitor_start caller fleet-wide (one extra agent turn per arm, forever). … Please split this into its own PR/decision, or get explicit maintainer sign-off recorded on-thread.

Resolved by removal, not sign-off: the owner asked for fire_immediately to be dropped and it is gone from this PR entirely (head 9c077907e, comment "fire_immediately dropped per maintainer request"). monitor_start keeps its historical first-fire timing — a full interval after arming. Nothing fleet-wide changes any more.

  1. This relaxes a trust boundary added 3 days ago in feat: expose session monitors to agents #5184, whose original author was not consulted. Please loop them in before this proceeds.

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

Separately, Design Review names an unresolved functional gap: since self-arm requires producer_is_user_facing, an autonomous member re-arming its own loop still gets a 409 … please link the follow-up issue in the PR body.

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. fire_immediately was dropped at owner request.

Current head fcd012727: 57/57 checks green, MERGEABLE, all AI review lanes clear. Not merging from this thread; that remains the owner's action.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

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

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 521aad01c66e1a199330f36f2275ac1449297314.

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.

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

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Follow-up to the owner decision above (comment 5565825909) — the review's third point is now fixed in this PR, not deferred:

Design Review names an unresolved functional gap: since self-arm requires producer_is_user_facing, an autonomous member re-arming its own loop still gets a 409 — so the incident this PR targets (never-wakes) becomes visible rather than fixed for autonomous turns.

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:

  • _fire_dashboard_nudge marks the wake turn it starts with _run_chat(_directive_self_wake=True); on a crew/member slot that wake only exists because the fire-time guard already proved the loop self-armed and trust-recorded.
  • apply_session_directive admits self-arm provenance when producer_is_user_facing or producer_is_self_wake; the wake mark never unlocks set_project / reset_conversation.
  • Cron, app and sub-agent turns carry neither mark → empty initiator → 409 with the visible ⚠️ Automation loop NOT armed notice, as before.
  • Keystone trust-record semantics unchanged (id + slot bound, written before svc.add, revoked after a committed removal).
  • Tests: the slot's own wake re-arms monitor_start / monitor_watch / structured monitor_update; headless injection still gets no provenance; human turn unchanged; the gateway marks the wake turn; self-wake does not unlock set_project. Spec and PR body updated. This resolves Admit a self-armed loop's own wake as self-arm provenance (follow-up to #8919) #9142.

@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 521aad01c: 56/56 checks green, MERGEABLE, all AI review lanes clear (GPT lane's "trust record agent-forgeable" finding overridden as a false positive — trust/ is already in sandbox._CREW_HIDDEN_LEAVES and security._CREW_SECRET_LEAVES). Branch protection now wants an approving review; the author cannot self-approve.

chenmingwei23
chenmingwei23 previously approved these changes Sep 7, 2026
… 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).
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.

4 participants