diff --git a/docs/feature-map/README.md b/docs/feature-map/README.md index 4afcf3d5f94..380570cfa27 100644 --- a/docs/feature-map/README.md +++ b/docs/feature-map/README.md @@ -130,7 +130,7 @@ Settings → Overview; the graph visualizer is a Developer internals view. |---|---|---|---|---|---| | Schedule | Cron jobs: recurring agent turns, scripts, commands | `/schedule` — rail **Schedule**; also created inline from the crew editor's "What wakes this crew" section (`/capabilities?tab=crews`) | `pages/SchedulePage.tsx`, `components/CrewWakeSection.tsx` | `handlers/cron.py` | `GET,POST /api/crons`, `DELETE /api/crons/{job_id}`, `GET /api/crons/history` | | Cron secret grants | Owner-approved vault-secret env grants for script crons: agent requests via `cron_secret_request`, the owner approves/denies/revokes on the job's Secrets panel | `/schedule` → job → **Secrets** | `pages/SchedulePage.tsx` (`JobSecretsPanel`) | `handlers/cron.py` | `PUT /api/crons/{job_id}/secrets` | -| Monitor loops | Same-session bounded monitors and legacy nudge loops watching an external thing; a member's loop is also surfaced read-only on the Crew Members drawer | Agent/API for bounded monitors; Chat header → legacy loop popover; Crew Members → member drawer → Auto patrol | `components/AutoNudgePopover.tsx`, `components/autoNudgeLoop.ts`, `pages/members/MembersPage.tsx` (read-only) | `handlers/autonudge.py` | `GET,POST /api/monitors`, `PATCH /api/monitors/{id}`, `GET /api/monitors/slot/{slot_key}`, `POST /api/monitors/{id}/stop`, `POST /api/monitors/{id}/restart`, `GET,POST /api/autonudge`, `PATCH,DELETE /api/autonudge/{loop_id}` | +| Monitor loops | Same-session bounded monitors and legacy nudge loops watching an external thing; a member's loop is also surfaced read-only on the Crew Members drawer | Agent/API for bounded monitors; Chat header → legacy loop popover; Crew Members → member drawer → Auto patrol | `components/AutoNudgePopover.tsx`, `components/autoNudgeLoop.ts`, `pages/members/MembersPage.tsx` (read-only) | `handlers/autonudge.py` | `GET,POST /api/monitors`, `PATCH /api/monitors/{id}`, `GET /api/monitors/slot/{slot_key}`, `POST /api/monitors/{id}/stop`, `POST /api/monitors/{id}/restart`, `GET,POST /api/autonudge`, `PATCH,DELETE /api/autonudge/{loop_id}`, `POST /api/autonudge/{loop_id}/fire` | | Session ledger | Durable per-session work state surviving compaction | Agent-written; no dashboard page | — | `handlers/session_ledger.py` | `GET /api/session-ledger`, `POST /api/session-ledger/record` | | Session control | Create / stop / send-to a session from outside it | Agent and app callers, not a UI | — | `session_control.py` | `POST /api/session-control/create`, `.../stop`, `.../send`, `GET .../read` | diff --git a/docs/system-specs/modules/learn-cron-dashboard.md b/docs/system-specs/modules/learn-cron-dashboard.md index 580364651df..074273b19af 100644 --- a/docs/system-specs/modules/learn-cron-dashboard.md +++ b/docs/system-specs/modules/learn-cron-dashboard.md @@ -1333,7 +1333,7 @@ off a non-object, which has no `.get` and no string key lookup and would surface **Cron Folders**: GET `/api/cron-folders` (list), POST `/api/cron-folders` (create `{name}` → `{id, name, order}`), PATCH `/api/cron-folders/{folder_id}` (rename `{name}`), DELETE `/api/cron-folders/{folder_id}` (delete folder, clears `folder_id` on assigned jobs) **Messaging**: POST `/api/send-message` (send to Slack DM + dashboard notification; body: `{text, title?, blocks?}`; when `blocks` provided, sends Block Kit message via `post_blocks()` with `text` as fallback; used by `send_message` MCP tool in `kirocrew-core`) -**AutoNudge** (`autonudge.py`, `autonudge_authz.py`, `dashboard/handlers/autonudge.py`): GET `/api/autonudge` (list loops — EVERY prompt-loop record the service holds, active or stopped, as `asdict(NudgeLoop)`, so `stopped_reason`, `next_due_ts` and `banner` ride the list even though the `autonudge_state` websocket frame carries only the live counters; the Crew Members drawer's per-member patrol block is a reader of this list, filtered by the member's `member-` slot key, and the frontend re-reads the list on every frame and on every reconnect rather than merging frames. **Reserved, not yet emitted:** the response MAY carry `denied: [{slot_key, code, reason, ts}]` — at most one entry per slot, the most recent REFUSED arm, and a SUCCESSFUL arm on that slot clears its entry, so a refusal can never mask a later stop; `code` is machine-readable in the same convention as non-2xx error bodies, drawn from the arm chokepoint's refusal branches — `member_mode` (a crew/member slot), `memory_mode` (incognito/temporary session), `session_gone` (owning slot no longer exists), `message_too_long`, `sentinel_path_sensitive`, `audit_unavailable` — and `reason` is that branch's own error text. Today refusals are recorded only in the SEL audit and the field is absent; a reader treats an absent field as "no refusal recorded", never as an error, and the frontend renders no refusal verdict until a producer ships), GET `/api/autonudge/slot/{slot_key}` (loop for a slot), POST `/api/autonudge` (start/replace a loop), PATCH `/api/autonudge/{loop_id}` (update), DELETE `/api/autonudge/{loop_id}` (stop). Loop-by-id resolution — the DELETE handler's pre-remove audit capture and the `PATCH`-path channel-banner refusal — shares ONE accessor, `svc.get_by_id(loop_id)` (returns the live loop or `None`), alongside `get_by_slot`/`list_all`, rather than two inline id-scans. Loop creation goes through the single chokepoint `authorize_and_add_nudge(svc, state, slot_key, message, …, source)` — a transport-agnostic security module at `autonudge_authz.py` (NOT the HTTP handler file; `state` is a narrow structural Protocol, and `dashboard/handlers/autonudge.py` is a thin HTTP mapping that re-exports it) — shared by the REST handler AND the workflow `ctx.nudge` bridge, so both enforce identical authorization before `svc.add`: dashboard `slot_key in state._slots`; Slack session routable; **Discord deny-by-default** (transport up, DM session only, user in `allowed_user_ids`, and `slot_key` == the user's *current* session key — blocks spoofing another `discord:` session); 8000-char message limit; sensitive `stop_sentinel_path` refused. **Load-time sentinel repair (`repair_sentinel_path`)**: the kill-switch path is resolved once at arm time and persisted verbatim, so the loop store can outlive the one-time `~/.kiro/crew`→`~/.kiro/crew` data-home migration and `start()` would re-arm a loop whose sentinel points at a directory that no longer exists — `_timer` only tests `Path(stop_sentinel_path).exists()`, so that loop's kill switch is **dead** (only `max_cycles` or an explicit remove can stop it). `_load()` therefore re-homes any legacy-rooted path onto the resolved current home and re-applies the arm-time sensitivity refusal (a persisted path that is sensitive *now* is dropped to `""` rather than stat'ed on a timer); a repair sets `_store_dirty` so `start()` flushes it once via an executor-offloaded write instead of re-deriving it every boot. The check deliberately does NOT require the path to live under the data home — an absolute `workspaces..dir` is a legitimate configuration, and filtering on containment would clear working kill switches. **`PATCH /api/autonudge/{loop_id}` applies the SAME message redaction as the arm chokepoint** (`redact_exfiltration_urls` + `redact_credentials`) — the field it mutates is the one that gets persisted and re-injected/posted on every fire, so redacting only on arm would make update a trivial bypass — and it SEL-audits `denied` outcomes (non-string/oversized message, non-integer `idle_secs`/`max_cycles`, non-boolean `active` — `bool("false")` is `True`, so a string would turn a pause into a resume — unknown loop), not just `success`. Both the redaction and the audit live in the transport-agnostic `authorize_and_update_nudge` chokepoint beside the arm one, not in the HTTP handler, so no future non-HTTP caller can bypass them; it too is **audit-or-deny** (critical `invoked` event before the mutation, 503 if unwritable). **Update/fire write serialization** — one protocol, two entry points: every writer must snapshot the store *while holding* `_lock` and then await an executor-offloaded `_write_state` (fsync must never run on the loop). `_update_locked` and `_add_locked` do that inline because they already hold the lock for their mutation; the post-fire bookkeeping, which holds no lock, calls the `_persist_locked` helper to acquire it first. A writer that snapshotted and *then* released could otherwise land a stale payload over newer `cycle_count`/`active` state and resurrect it after a restart, so `update()` is shielded like `add()` to keep the lock held across its own offloaded write. **One timer-cancellation policy (`_cancel_timer`)**: two conditions make a cancel wrong rather than redundant, and both live in that one method so no caller has to remember either — the currently running timer task (a self-re-arm from inside `_timer`) is about to return on its own, and a task whose event loop has already CLOSED must be dropped rather than cancelled. `Task.cancel` schedules through `loop.call_soon`, so cancelling a task on a closed loop raises `RuntimeError: Event loop is closed`, which escaped `remove`/`remove_sync` and made the dashboard handler above it answer **500**. The service is a process-global singleton (`_INSTANCE`, published by `start()`), so its `_timers` outlive the loop that created them whenever one loop replaces another: the gateway's own shutdown, and — the way this surfaced — a test driving a handler after an earlier test's loop closed. The condition is asked positively (`get_loop().is_closed()`) rather than by catching the `RuntimeError`, because a closed loop is the one state where cancelling is a no-op by definition and catching would also swallow a genuine scheduling fault. `stop()` routes through it for the same reason rather than looping over bare `t.cancel()` — shutdown is the likeliest moment for a timer's loop to be closing already. The closed-loop question is asked BEFORE the current-task one because it needs no running loop of its own: `stop()` is reached from synchronous callers (gateway shutdown, test teardown) where `asyncio.current_task()` raises `RuntimeError: no running event loop` rather than answering None, which `_current_task_or_none` turns into the None the caller means. The test-side floor for the singleton is `_restore_autonudge_singleton` (see [testing-conventions](../common/testing-conventions.md)). **Mid-fire updates never cancel the timer** (`_firing` tracks the fire window): channel loops run the unattended turn inline in `_on_fire`, so cancelling would kill the turn — the re-arm is deferred to the running timer, and the undelivered path refuses to re-arm a loop that was deactivated mid-fire so an explicit pause is not silently resumed. The window spans delivery, the bookkeeping persist, and the re-arm decision (`_run_fire_cycle`), and `notify_turn_complete` observes it too: a dashboard turn that completes mid-window records the re-arm in `_rearm_pending` and it is applied when the window closes, because arming immediately would cancel the firing task while it is writing the delivered cycle. **`monitor_update` MCP tool**: revises the loop bound to the calling session in place (message / idle_secs / max_cycles) so a stale instruction can be corrected without tearing the loop down and losing its `cycle_count`. It resolves the loop id from the caller's own binding key rather than accepting a caller-supplied id, so a cross-session rewrite is unrepresentable. Since issue #755 it is STATELESS (see `session.md` → "Stateless session-directive tools"): the tool validates its patch and returns a directive, and the session-aware consumer resolves the loop from ITS OWN binding key via `svc.get_by_slot(binding)` (`session_directive_apply.py`), never via `GET /api/autonudge/slot/{key}`. The tool still calls `_resolve_session_key_strict()` (no PID walk), but only as a context guard that rejects un-appliable sessions (cron/hook/subagent) — not to bind the effect — for the same reason `monitor_start`/`autonudge_stop` do. **`monitor_start` cycle cap**: omitting `max_cycles` now yields a bounded default (`_MONITOR_DEFAULT_MAX_CYCLES = 24`) instead of unlimited, because an unbounded loop stops only when the model volunteers `autonudge_stop` and observed loop stores show that is unreliable (real babysit loops ran to 24/24 and 20/20 delivered cycles, stopping only on the cap); explicit `0` still means unlimited. **Wall-clock budget (`max_runtime_secs`)**: a second, opt-in terminal bound (0 = unlimited, ceiling `MAX_RUNTIME_SECS_CEILING` = 604800s/7 days — enforced at BOTH authz chokepoints, not just the MCP schemas, so REST/workflow callers cannot exceed it, and the REST handler refuses non-integral floats rather than truncating them) alongside the cycle cap, because a cycle cap alone cannot bound COST — a loop with slow turns or a long idle gap can run for days within its cycle budget. Measured from the persisted `created_ts` (not arm time), so a gateway restart re-arms the loop but never resets its clock; a store entry with no `created_ts` never trips (no anchor — guessing one could kill a healthy loop on its first post-upgrade cycle). Enforced in `_timer` via the shared `runtime_budget_exceeded` predicate, checked AFTER the cycle cap (both exhausted → the cap wins) and BEFORE the fire so a spent budget never buys one more unattended turn, **and re-checked immediately post-delivery** in `_run_fire_cycle` — a budget that expired during a slow turn deactivates the loop the moment that turn ends instead of arming another full idle cycle. The service never cancels an IN-FLIGHT turn (the mid-fire contracts above forbid it — cancelling kills channel turns), so the declared bound can overshoot by at most one turn, itself capped by the transport's per-turn ceiling (`constants.CHAT_TURN_TIMEOUT`); terminal treatment matches the cap — deactivate (inspectable/restartable, not removed) + `expired` event, and the notifier (`_notify_nudge_expired`) words the notification per bound using the same predicate. **A terminal subject outranks every bound in that wording, and an OWED terminal turn counts as one**: a channel-bound loop does not settle on observation (it learns its watch finished from a delivered turn), so the probe records the owed turn in `monitor.terminal_pending` and leaves the loop active with no `outcome` and no `monitor_terminal` reason — and if that final turn is refused and the retry finds a bound spent, `_timer` deactivates on the bound before the settlement that would promote the debt ever runs. The notifier therefore derives `terminal` from `stopped_reason == monitor_terminal` **or** an outstanding `terminal_pending`, and takes the merged-vs-closed-unmerged distinction from the settled `outcome` falling back to the debt (both use `success`/`blocked`), so a watch whose subject merged is never announced with the signal of one that ran out of cycles. This is WORDING only: the bound that stopped the loop keeps its own `stopped_reason`, so the spent cap stays observable and the `monitor_update` revival affordance keyed on `cycle_cap` is unchanged. Whether an owed terminal turn should instead outrank the cap in `_timer` and be DELIVERED is a separate, open question (issue #8060), not settled here. **Approval-stall stop (`approval_stalled`)**: a third terminal condition, evaluated in `_timer` LAST of the three so a loop also out of cycles or budget still reports the bound it historically would have. It is **reactive by construction** — it fires only on recorded evidence that a cycle's tool approval went unanswered, never on a reading of whether an auto-approve grant is in force. The evidence is written by `notify_approval_stalled(slot_key)`, a sync slot-keyed hook alongside `notify_turn_complete`/`notify_user_input`, called from the approval-timeout branch of ALL THREE paths a nudge cycle can stall in, matching the three fire paths `_run_fire_cycle` dispatches: `chat_runner`'s per-slot wait (dashboard-slot loops), `_interactive_approval`'s raced wait in the Slack gateway (Slack loops, whose turns are approved there rather than through the dashboard runner -- the fire path threads the loop's own `slot_key` in as `nudge_key`, and the guard keeps cron/taskrunner/subagent consumers of that same callback unaffected), and `DiscordApprovalDecider.__call__`'s button wait (Discord loops, keyed on the decider's own `session_key`). Only the timeout branch records; an explicit human rejection is a decision, not evidence that nobody is present. The hook sets the persisted `approval_stalled` flag and returns rather than stopping inline, because stopping there would cancel a possibly-mid-fire timer -- the one thing the fire-window contracts forbid -- and race the turn that produced the evidence. The predictive alternative (test the grant before dispatching) was rejected: a loop whose cycles only touch auto-approved tools needs no grant, so it would be stopped for nothing, and such a loop can never reach an interactive approval wait, which is what makes the reactive test free of that false positive rather than merely tuned against it. Cost is bounded at the one cycle already in flight. `monitor_update` deliberately has **no revival affordance** for this reason (raising a bound does not restore an authorization), but its paused-loop denial names the real remedy — re-enable auto-approve, then `monitor_start` — and every revival clears the flag so a re-granted loop is not stopped by spent evidence on its first wake. The clear is keyed on an actual revival (`not was_active`), not on any `active=True`: a still-active loop also receives one from an ordinary settings save, and treating that as an answer would erase evidence recorded moments earlier and let one more doomed cycle fire. Before this existed, a loop whose grant lapsed kept waking, dispatching, being declined and spending its cap on cycles that were never able to act, with `cycle_count` making a capped-out run indistinguishable from a finished one; merged reporting (an operator notice on grant expiry) explained that after the fact but never stopped it, and being edge-triggered on the expiry event could not cover a loop that started after the grant was already gone. **`stopped_reason`** (persisted on the loop: `""`/`"manual"`/`"autonudge_stop"`/`"cycle_cap"`/`"runtime_budget"`/`"approval_stalled"`) records WHY the last deactivation happened — `_timer`'s terminal bounds tag themselves, any other `update(active=False)` defaults to `"manual"`, and every revival clears it. The `autonudge_stop` session-directive applier persists `"autonudge_stop"` only for `research-*` loops, whose watchdog consumes that source-owned evidence; ordinary dashboard and channel loops retain the historical remove-on-stop behavior instead of leaving a paused record with no consumer. A Research Lab stop may replace an earlier manual pause from an app-disable race, while cycle/runtime-bound writers still cannot overwrite a deactivation that landed first. The caller’s free-form explanation is deliberately not persisted because the watchdog needs only the deterministic source and model-authored text may contain sensitive content. The Research Lab record is restart-durable until consumed. Its watchdog checks the tombstone before trust-expiry handling or reviving loops suspended by an app disable: on the first watchdog poll after any in-flight worker turn exits, it prefers the tombstone over `worker_done.json`, preserves the existing verified-finding-first verdict, requires at least one readable finding before reporting STOPPED, then removes the consumed loop record. `worker_done.json` remains the conservative fallback at the normal idle deadline when no tombstone exists, while mere loop absence remains untrusted because unreachable-session cleanup also removes loops. Revival via `monitor_update` keys on a paused record's source, NOT on elapsed-time inference: wall-clock keeps growing after a manual pause, so "budget looks spent" cannot distinguish a pause from an expiry, and a budget raise must never resume a loop the user paused (the cycle-count heuristic survives only as a legacy fallback for stores written before the field). A budget-stopped loop revives when the budget is raised above its elapsed age (or 0), matching the cap-raise affordance; the paused-loop denial names the bound that actually stopped the loop. **Arm-failure reporting**: the stateless applier (`session_directive_apply.py::_monitor_start`) returns an explicit human-readable outcome for the model — a `Monitor loop … started …` confirmation, `Monitor loop NOT armed: auto-nudge is disabled on this host.`, or `Failed to start monitor loop: {error}` — rather than the former undifferentiated string that was indistinguishable from the transient MCP reconnects agents are instructed to retry through. Auditing is preserved in intent: the applier emits a **SEL event for every outcome** tagged `source="mcp-directive"` (`success` / `denied` / `error`), and the `POST /api/autonudge` REST handler and the workflow `ctx.nudge` bridge keep their own `source="dashboard"` / `source="workflow"` audits through the shared `authorize_and_add_nudge` chokepoint. **AUDIT-OR-DENY availability policy (deliberate contract change)**: a CRITICAL `invoked` event is written (synchronously on a worker thread, awaited) *before* `svc.add` arms the loop; if that write fails, the arm is **denied** — `POST /api/autonudge` returns **503** ("audit log unavailable — nudge loop not armed") and the workflow path reports "NOT armed" into the run. Previously SEL unavailability could not prevent arming a loop; now a wedged SEL trades availability for a guaranteed audit trail (no loop may ever exist unaudited), matching the repo's fail-closed security posture. The terminal `success` event is best-effort — an armed loop is already covered by the `invoked` record. **`add()` ordering semantics (observable by all callers)**: `add()` awaits an executor-offloaded persist and arms the timer under a shielded task, so the loop's FIRST timer cycle may complete before `add()` returns — callers must not assume post-`add()` state predates the first fire. A caller cancelled mid-`add()` gets shielded "mutation may have already landed" semantics: it receives `CancelledError`, but the arm+persist completes (writes stay strictly serialized under the service lock). `binding_key_for(session_key)` (in `autonudge.py`) is the single source of truth for "nudge-able" (`dashboard:chat-N-TS`→`chat-N-TS`; `slack:`/`discord:`/`webex:` pass through; `cron:`/`hook:`/`subagent:`/empty→`None`), shared with the `monitor_start` directive applier (via `_binding()` in `session_directive_apply.py`). **Workflow `ctx.nudge`**: dynamic-workflow scripts arm a loop on their originating session — `WorkflowService` threads the launching `session_key` (`start`→`run`→`_exec_validated`→`_RunContext`) and wires a `nudge` port to a gateway-injected `nudge_authorizer` that maps the (caller-influenced) key via `binding_key_for` and calls `authorize_and_add_nudge(…, source="workflow")`; the workflows package never touches `AutoNudgeService`/`state` directly. Best-effort: an unwired authorizer, a non-nudge-able session, an authz rejection, or `svc.add`/no-loop failure degrades to a logged no-op (the arm runs as a `create_task` whose ref is held in `WorkflowService._nudge_tasks` to avoid mid-await GC) — a monitoring convenience never crashes a completed run. This fixes the prior `RuntimeError("ctx.nudge is not available for this run (no nudge port wired)")`. The workflow authoring prompt (`_AUTHOR_SYSTEM` in `workflows/service.py`) advertises ONLY ctx primitives production actually wires; a parity contract test (`test_workflows_nudge_wiring.py::test_author_prompt_advertises_only_wired_primitives`) fails if a primitive is added to the prompt without a wired port (or vice versa), so the advertised-but-unwired crash class cannot silently reappear. The same class is closed at the ENFORCEMENT layer too: the runner calls `validate.check_ctx_surface(source, CORE_CTX_SURFACE | )` at the exec boundary, so a hand-written or rerun script referencing a primitive the executing host did not wire fails validation with a clear per-line error (`run_failed`, `where="validate"`) instead of starting and dying mid-run — host-aware by construction, so test/companion runners that wire additional ports keep their full surface. +**AutoNudge** (`autonudge.py`, `autonudge_authz.py`, `dashboard/handlers/autonudge.py`): GET `/api/autonudge` (list loops — EVERY prompt-loop record the service holds, active or stopped, as `asdict(NudgeLoop)`, so `stopped_reason`, `next_due_ts` and `banner` ride the list even though the `autonudge_state` websocket frame carries only the live counters; the Crew Members drawer's per-member patrol block is a reader of this list, filtered by the member's `member-` slot key, and the frontend re-reads the list on every frame and on every reconnect rather than merging frames. **Reserved, not yet emitted:** the response MAY carry `denied: [{slot_key, code, reason, ts}]` — at most one entry per slot, the most recent REFUSED arm, and a SUCCESSFUL arm on that slot clears its entry, so a refusal can never mask a later stop; `code` is machine-readable in the same convention as non-2xx error bodies, drawn from the arm chokepoint's refusal branches — `member_mode` (a crew/member slot), `memory_mode` (incognito/temporary session), `session_gone` (owning slot no longer exists), `message_too_long`, `sentinel_path_sensitive`, `audit_unavailable` — and `reason` is that branch's own error text. Today refusals are recorded only in the SEL audit and the field is absent; a reader treats an absent field as "no refusal recorded", never as an error, and the frontend renders no refusal verdict until a producer ships), GET `/api/autonudge/slot/{slot_key}` (loop for a slot), POST `/api/autonudge` (start/replace a loop), PATCH `/api/autonudge/{loop_id}` (update), DELETE `/api/autonudge/{loop_id}` (stop), POST `/api/autonudge/{loop_id}/fire` (run the loop's next cycle NOW, out of band from the idle countdown). **The fire route arms rather than delivers, and writes nothing**: `AutoNudgeService.fire_now` only re-arms through `_arm_timer(delay=0.0)`, so the cycle runs inside the ordinary `_timer` body and the stop sentinel, the cycle cap, the wall-clock budget, the approval-stall stop and the probe gate all still apply -- delivery goes through the one `_on_fire` path, and a manual cycle therefore advances `cycle_count` exactly as a scheduled one does. `_timer` sleeps the delay it is given and fires WITHOUT consulting `next_due_ts`, which is why arming alone is sufficient. `delay=0.0` rather than `_arm_from_deadline`'s `_OVERDUE_REARM_SECS` beat, because that beat exists so an elapsed deadline does not ambush a user mid-conversation and a manual press IS the user asking. **`next_due_ts` is deliberately NOT written, and the method has no `await` at all.** An earlier revision wrote the deadline and awaited a durable persist so a restart between the press and the fire would resume overdue; that await was a suspension window, and several writers to `next_due_ts` in this module hold no lock while writing it (the quiet-tick re-arm on the gated-wake branch is one), so each guard closing one writer's window exposed the next -- a concurrent `remove` arming a stale object, a cancelled caller abandoning the write, a countdown entering `_firing` mid-persist, a quiet tick overwriting the deadline, then the refused path leaving its own value committed. The window is not closable at that call site, so the write was removed instead: the cost is that a restart between the press and the fire resumes on the loop's own schedule rather than overdue, the same degradation `_persist_soon` already documents for every other deadline assignment here. The popover supplies the "due" reading locally so a press still shows its one confirmation, reconciled by the next `autonudge_state` frame. The interval reset is INHERITED, not implemented: a delivered cycle clears `next_due_ts` and the re-arm starts a fresh full interval, so the next automatic nudge lands one `idle_secs` after the manual turn ENDS, and `notify_user_input` still defers rather than cancels. Refuses a loop it does not hold (404 -- the stop sentinel lands here too, since it removes the loop), an INACTIVE loop (409 -- the non-removing terminal bounds all leave the loop registered but inactive, so one condition covers them and a press cannot buy a turn past a bound the user armed), a loop MID-FIRE (409 -- `_arm_timer` would cancel a task that may be parked on `_persist_locked()` writing the delivered cycle, the same window `notify_turn_complete` defers around), a structured monitor (409 `structured_monitor_requires_monitor_api`, as `PATCH` does), and a session with a turn in flight (409 `session_busy`). That last one is REFUSED RATHER THAN QUEUED, which is the fire path's own recorded decision -- queueing "would stack identical 3KB+ nudges and blow up the context window" -- read through the canonical two-term predicate `slot.running or slot._in_stage_execution` (`slot.running` alone is False between the stages of a multi-stage plan). The busy pre-check is an AFFORDANCE, not a guarantee: a turn starting between it and the fire is still refused by the fire path, so the check exists to turn that silence into a 409 the goal popover can show. The fire itself is AUDIT-OR-DENY (`autonudge_fire`, `critical=True`, off-loop) because a delivered cycle spends a model turn; the refusals are audited best-effort, since the request is refused either way. Loop-by-id resolution — the DELETE handler's pre-remove audit capture and the `PATCH`-path channel-banner refusal — shares ONE accessor, `svc.get_by_id(loop_id)` (returns the live loop or `None`), alongside `get_by_slot`/`list_all`, rather than two inline id-scans. Loop creation goes through the single chokepoint `authorize_and_add_nudge(svc, state, slot_key, message, …, source)` — a transport-agnostic security module at `autonudge_authz.py` (NOT the HTTP handler file; `state` is a narrow structural Protocol, and `dashboard/handlers/autonudge.py` is a thin HTTP mapping that re-exports it) — shared by the REST handler AND the workflow `ctx.nudge` bridge, so both enforce identical authorization before `svc.add`: dashboard `slot_key in state._slots`; Slack session routable; **Discord deny-by-default** (transport up, DM session only, user in `allowed_user_ids`, and `slot_key` == the user's *current* session key — blocks spoofing another `discord:` session); 8000-char message limit; sensitive `stop_sentinel_path` refused. **Load-time sentinel repair (`repair_sentinel_path`)**: the kill-switch path is resolved once at arm time and persisted verbatim, so the loop store can outlive the one-time `~/.kiro/crew`→`~/.kiro/crew` data-home migration and `start()` would re-arm a loop whose sentinel points at a directory that no longer exists — `_timer` only tests `Path(stop_sentinel_path).exists()`, so that loop's kill switch is **dead** (only `max_cycles` or an explicit remove can stop it). `_load()` therefore re-homes any legacy-rooted path onto the resolved current home and re-applies the arm-time sensitivity refusal (a persisted path that is sensitive *now* is dropped to `""` rather than stat'ed on a timer); a repair sets `_store_dirty` so `start()` flushes it once via an executor-offloaded write instead of re-deriving it every boot. The check deliberately does NOT require the path to live under the data home — an absolute `workspaces..dir` is a legitimate configuration, and filtering on containment would clear working kill switches. **`PATCH /api/autonudge/{loop_id}` applies the SAME message redaction as the arm chokepoint** (`redact_exfiltration_urls` + `redact_credentials`) — the field it mutates is the one that gets persisted and re-injected/posted on every fire, so redacting only on arm would make update a trivial bypass — and it SEL-audits `denied` outcomes (non-string/oversized message, non-integer `idle_secs`/`max_cycles`, non-boolean `active` — `bool("false")` is `True`, so a string would turn a pause into a resume — unknown loop), not just `success`. Both the redaction and the audit live in the transport-agnostic `authorize_and_update_nudge` chokepoint beside the arm one, not in the HTTP handler, so no future non-HTTP caller can bypass them; it too is **audit-or-deny** (critical `invoked` event before the mutation, 503 if unwritable). **Update/fire write serialization** — one protocol, two entry points: every writer must snapshot the store *while holding* `_lock` and then await an executor-offloaded `_write_state` (fsync must never run on the loop). `_update_locked` and `_add_locked` do that inline because they already hold the lock for their mutation; the post-fire bookkeeping, which holds no lock, calls the `_persist_locked` helper to acquire it first. A writer that snapshotted and *then* released could otherwise land a stale payload over newer `cycle_count`/`active` state and resurrect it after a restart, so `update()` is shielded like `add()` to keep the lock held across its own offloaded write. **One timer-cancellation policy (`_cancel_timer`)**: two conditions make a cancel wrong rather than redundant, and both live in that one method so no caller has to remember either — the currently running timer task (a self-re-arm from inside `_timer`) is about to return on its own, and a task whose event loop has already CLOSED must be dropped rather than cancelled. `Task.cancel` schedules through `loop.call_soon`, so cancelling a task on a closed loop raises `RuntimeError: Event loop is closed`, which escaped `remove`/`remove_sync` and made the dashboard handler above it answer **500**. The service is a process-global singleton (`_INSTANCE`, published by `start()`), so its `_timers` outlive the loop that created them whenever one loop replaces another: the gateway's own shutdown, and — the way this surfaced — a test driving a handler after an earlier test's loop closed. The condition is asked positively (`get_loop().is_closed()`) rather than by catching the `RuntimeError`, because a closed loop is the one state where cancelling is a no-op by definition and catching would also swallow a genuine scheduling fault. `stop()` routes through it for the same reason rather than looping over bare `t.cancel()` — shutdown is the likeliest moment for a timer's loop to be closing already. The closed-loop question is asked BEFORE the current-task one because it needs no running loop of its own: `stop()` is reached from synchronous callers (gateway shutdown, test teardown) where `asyncio.current_task()` raises `RuntimeError: no running event loop` rather than answering None, which `_current_task_or_none` turns into the None the caller means. The test-side floor for the singleton is `_restore_autonudge_singleton` (see [testing-conventions](../common/testing-conventions.md)). **Mid-fire updates never cancel the timer** (`_firing` tracks the fire window): channel loops run the unattended turn inline in `_on_fire`, so cancelling would kill the turn — the re-arm is deferred to the running timer, and the undelivered path refuses to re-arm a loop that was deactivated mid-fire so an explicit pause is not silently resumed. The window spans delivery, the bookkeeping persist, and the re-arm decision (`_run_fire_cycle`), and `notify_turn_complete` observes it too: a dashboard turn that completes mid-window records the re-arm in `_rearm_pending` and it is applied when the window closes, because arming immediately would cancel the firing task while it is writing the delivered cycle. **`monitor_update` MCP tool**: revises the loop bound to the calling session in place (message / idle_secs / max_cycles) so a stale instruction can be corrected without tearing the loop down and losing its `cycle_count`. It resolves the loop id from the caller's own binding key rather than accepting a caller-supplied id, so a cross-session rewrite is unrepresentable. Since issue #755 it is STATELESS (see `session.md` → "Stateless session-directive tools"): the tool validates its patch and returns a directive, and the session-aware consumer resolves the loop from ITS OWN binding key via `svc.get_by_slot(binding)` (`session_directive_apply.py`), never via `GET /api/autonudge/slot/{key}`. The tool still calls `_resolve_session_key_strict()` (no PID walk), but only as a context guard that rejects un-appliable sessions (cron/hook/subagent) — not to bind the effect — for the same reason `monitor_start`/`autonudge_stop` do. **`monitor_start` cycle cap**: omitting `max_cycles` now yields a bounded default (`_MONITOR_DEFAULT_MAX_CYCLES = 24`) instead of unlimited, because an unbounded loop stops only when the model volunteers `autonudge_stop` and observed loop stores show that is unreliable (real babysit loops ran to 24/24 and 20/20 delivered cycles, stopping only on the cap); explicit `0` still means unlimited. **Wall-clock budget (`max_runtime_secs`)**: a second, opt-in terminal bound (0 = unlimited, ceiling `MAX_RUNTIME_SECS_CEILING` = 604800s/7 days — enforced at BOTH authz chokepoints, not just the MCP schemas, so REST/workflow callers cannot exceed it, and the REST handler refuses non-integral floats rather than truncating them) alongside the cycle cap, because a cycle cap alone cannot bound COST — a loop with slow turns or a long idle gap can run for days within its cycle budget. Measured from the persisted `created_ts` (not arm time), so a gateway restart re-arms the loop but never resets its clock; a store entry with no `created_ts` never trips (no anchor — guessing one could kill a healthy loop on its first post-upgrade cycle). Enforced in `_timer` via the shared `runtime_budget_exceeded` predicate, checked AFTER the cycle cap (both exhausted → the cap wins) and BEFORE the fire so a spent budget never buys one more unattended turn, **and re-checked immediately post-delivery** in `_run_fire_cycle` — a budget that expired during a slow turn deactivates the loop the moment that turn ends instead of arming another full idle cycle. The service never cancels an IN-FLIGHT turn (the mid-fire contracts above forbid it — cancelling kills channel turns), so the declared bound can overshoot by at most one turn, itself capped by the transport's per-turn ceiling (`constants.CHAT_TURN_TIMEOUT`); terminal treatment matches the cap — deactivate (inspectable/restartable, not removed) + `expired` event, and the notifier (`_notify_nudge_expired`) words the notification per bound using the same predicate. **A terminal subject outranks every bound in that wording, and an OWED terminal turn counts as one**: a channel-bound loop does not settle on observation (it learns its watch finished from a delivered turn), so the probe records the owed turn in `monitor.terminal_pending` and leaves the loop active with no `outcome` and no `monitor_terminal` reason — and if that final turn is refused and the retry finds a bound spent, `_timer` deactivates on the bound before the settlement that would promote the debt ever runs. The notifier therefore derives `terminal` from `stopped_reason == monitor_terminal` **or** an outstanding `terminal_pending`, and takes the merged-vs-closed-unmerged distinction from the settled `outcome` falling back to the debt (both use `success`/`blocked`), so a watch whose subject merged is never announced with the signal of one that ran out of cycles. This is WORDING only: the bound that stopped the loop keeps its own `stopped_reason`, so the spent cap stays observable and the `monitor_update` revival affordance keyed on `cycle_cap` is unchanged. Whether an owed terminal turn should instead outrank the cap in `_timer` and be DELIVERED is a separate, open question (issue #8060), not settled here. **Approval-stall stop (`approval_stalled`)**: a third terminal condition, evaluated in `_timer` LAST of the three so a loop also out of cycles or budget still reports the bound it historically would have. It is **reactive by construction** — it fires only on recorded evidence that a cycle's tool approval went unanswered, never on a reading of whether an auto-approve grant is in force. The evidence is written by `notify_approval_stalled(slot_key)`, a sync slot-keyed hook alongside `notify_turn_complete`/`notify_user_input`, called from the approval-timeout branch of ALL THREE paths a nudge cycle can stall in, matching the three fire paths `_run_fire_cycle` dispatches: `chat_runner`'s per-slot wait (dashboard-slot loops), `_interactive_approval`'s raced wait in the Slack gateway (Slack loops, whose turns are approved there rather than through the dashboard runner -- the fire path threads the loop's own `slot_key` in as `nudge_key`, and the guard keeps cron/taskrunner/subagent consumers of that same callback unaffected), and `DiscordApprovalDecider.__call__`'s button wait (Discord loops, keyed on the decider's own `session_key`). Only the timeout branch records; an explicit human rejection is a decision, not evidence that nobody is present. The hook sets the persisted `approval_stalled` flag and returns rather than stopping inline, because stopping there would cancel a possibly-mid-fire timer -- the one thing the fire-window contracts forbid -- and race the turn that produced the evidence. The predictive alternative (test the grant before dispatching) was rejected: a loop whose cycles only touch auto-approved tools needs no grant, so it would be stopped for nothing, and such a loop can never reach an interactive approval wait, which is what makes the reactive test free of that false positive rather than merely tuned against it. Cost is bounded at the one cycle already in flight. `monitor_update` deliberately has **no revival affordance** for this reason (raising a bound does not restore an authorization), but its paused-loop denial names the real remedy — re-enable auto-approve, then `monitor_start` — and every revival clears the flag so a re-granted loop is not stopped by spent evidence on its first wake. The clear is keyed on an actual revival (`not was_active`), not on any `active=True`: a still-active loop also receives one from an ordinary settings save, and treating that as an answer would erase evidence recorded moments earlier and let one more doomed cycle fire. Before this existed, a loop whose grant lapsed kept waking, dispatching, being declined and spending its cap on cycles that were never able to act, with `cycle_count` making a capped-out run indistinguishable from a finished one; merged reporting (an operator notice on grant expiry) explained that after the fact but never stopped it, and being edge-triggered on the expiry event could not cover a loop that started after the grant was already gone. **`stopped_reason`** (persisted on the loop: `""`/`"manual"`/`"autonudge_stop"`/`"cycle_cap"`/`"runtime_budget"`/`"approval_stalled"`) records WHY the last deactivation happened — `_timer`'s terminal bounds tag themselves, any other `update(active=False)` defaults to `"manual"`, and every revival clears it. The `autonudge_stop` session-directive applier persists `"autonudge_stop"` only for `research-*` loops, whose watchdog consumes that source-owned evidence; ordinary dashboard and channel loops retain the historical remove-on-stop behavior instead of leaving a paused record with no consumer. A Research Lab stop may replace an earlier manual pause from an app-disable race, while cycle/runtime-bound writers still cannot overwrite a deactivation that landed first. The caller’s free-form explanation is deliberately not persisted because the watchdog needs only the deterministic source and model-authored text may contain sensitive content. The Research Lab record is restart-durable until consumed. Its watchdog checks the tombstone before trust-expiry handling or reviving loops suspended by an app disable: on the first watchdog poll after any in-flight worker turn exits, it prefers the tombstone over `worker_done.json`, preserves the existing verified-finding-first verdict, requires at least one readable finding before reporting STOPPED, then removes the consumed loop record. `worker_done.json` remains the conservative fallback at the normal idle deadline when no tombstone exists, while mere loop absence remains untrusted because unreachable-session cleanup also removes loops. Revival via `monitor_update` keys on a paused record's source, NOT on elapsed-time inference: wall-clock keeps growing after a manual pause, so "budget looks spent" cannot distinguish a pause from an expiry, and a budget raise must never resume a loop the user paused (the cycle-count heuristic survives only as a legacy fallback for stores written before the field). A budget-stopped loop revives when the budget is raised above its elapsed age (or 0), matching the cap-raise affordance; the paused-loop denial names the bound that actually stopped the loop. **Arm-failure reporting**: the stateless applier (`session_directive_apply.py::_monitor_start`) returns an explicit human-readable outcome for the model — a `Monitor loop … started …` confirmation, `Monitor loop NOT armed: auto-nudge is disabled on this host.`, or `Failed to start monitor loop: {error}` — rather than the former undifferentiated string that was indistinguishable from the transient MCP reconnects agents are instructed to retry through. Auditing is preserved in intent: the applier emits a **SEL event for every outcome** tagged `source="mcp-directive"` (`success` / `denied` / `error`), and the `POST /api/autonudge` REST handler and the workflow `ctx.nudge` bridge keep their own `source="dashboard"` / `source="workflow"` audits through the shared `authorize_and_add_nudge` chokepoint. **AUDIT-OR-DENY availability policy (deliberate contract change)**: a CRITICAL `invoked` event is written (synchronously on a worker thread, awaited) *before* `svc.add` arms the loop; if that write fails, the arm is **denied** — `POST /api/autonudge` returns **503** ("audit log unavailable — nudge loop not armed") and the workflow path reports "NOT armed" into the run. Previously SEL unavailability could not prevent arming a loop; now a wedged SEL trades availability for a guaranteed audit trail (no loop may ever exist unaudited), matching the repo's fail-closed security posture. The terminal `success` event is best-effort — an armed loop is already covered by the `invoked` record. **`add()` ordering semantics (observable by all callers)**: `add()` awaits an executor-offloaded persist and arms the timer under a shielded task, so the loop's FIRST timer cycle may complete before `add()` returns — callers must not assume post-`add()` state predates the first fire. A caller cancelled mid-`add()` gets shielded "mutation may have already landed" semantics: it receives `CancelledError`, but the arm+persist completes (writes stay strictly serialized under the service lock). `binding_key_for(session_key)` (in `autonudge.py`) is the single source of truth for "nudge-able" (`dashboard:chat-N-TS`→`chat-N-TS`; `slack:`/`discord:`/`webex:` pass through; `cron:`/`hook:`/`subagent:`/empty→`None`), shared with the `monitor_start` directive applier (via `_binding()` in `session_directive_apply.py`). **Workflow `ctx.nudge`**: dynamic-workflow scripts arm a loop on their originating session — `WorkflowService` threads the launching `session_key` (`start`→`run`→`_exec_validated`→`_RunContext`) and wires a `nudge` port to a gateway-injected `nudge_authorizer` that maps the (caller-influenced) key via `binding_key_for` and calls `authorize_and_add_nudge(…, source="workflow")`; the workflows package never touches `AutoNudgeService`/`state` directly. Best-effort: an unwired authorizer, a non-nudge-able session, an authz rejection, or `svc.add`/no-loop failure degrades to a logged no-op (the arm runs as a `create_task` whose ref is held in `WorkflowService._nudge_tasks` to avoid mid-await GC) — a monitoring convenience never crashes a completed run. This fixes the prior `RuntimeError("ctx.nudge is not available for this run (no nudge port wired)")`. The workflow authoring prompt (`_AUTHOR_SYSTEM` in `workflows/service.py`) advertises ONLY ctx primitives production actually wires; a parity contract test (`test_workflows_nudge_wiring.py::test_author_prompt_advertises_only_wired_primitives`) fails if a primitive is added to the prompt without a wired port (or vice versa), so the advertised-but-unwired crash class cannot silently reappear. The same class is closed at the ENFORCEMENT layer too: the runner calls `validate.check_ctx_surface(source, CORE_CTX_SURFACE | )` at the exec boundary, so a hand-written or rerun script referencing a primitive the executing host did not wire fails validation with a clear per-line error (`run_failed`, `where="validate"`) instead of starting and dying mid-run — host-aware by construction, so test/companion runners that wire additional ports keep their full surface. Research Lab worker slot keys have the canonical shape `research-` plus an eight-character lowercase hexadecimal campaign id. Creation and recognition share `apps/builtins/auto_research/session_keys.py`. A matching name alone is diff --git a/src/kiro_crew/autonudge.py b/src/kiro_crew/autonudge.py index 5924ffac894..9b6b3fd6174 100644 --- a/src/kiro_crew/autonudge.py +++ b/src/kiro_crew/autonudge.py @@ -4366,6 +4366,95 @@ async def _run_fire_cycle(self, loop: NudgeLoop) -> None: if is_channel_key(loop.slot_key) and loop.active and loop.id in self._loops: self._arm_from_deadline(loop) + async def fire_now(self, loop_id: str) -> tuple["NudgeLoop | None", str, int]: + """Bring one loop's next cycle forward to now, out of band from its countdown. + + Returns ``(loop, "", 200)`` once the cycle is armed to run, or + ``(None, reason, status)`` on refusal — the ``(obj, error, status)`` + shape the authz chokepoints in :mod:`kiro_crew.autonudge_authz` already + use, so the HTTP handler stays a thin mapping. + + WHAT THIS DELIBERATELY DOES NOT DO: it does not deliver the nudge + itself. It re-arms through :meth:`_arm_timer`, so the cycle runs inside + the ordinary :meth:`_timer` body — the stop sentinel, the cycle cap, the + wall-clock budget, the approval-stall stop and the probe gate all apply + exactly as they do on a scheduled tick, and the delivery goes through the + one ``_on_fire`` path. Calling :meth:`_run_fire_cycle` directly would have + needed that whole ladder restated here, and a second copy of a + five-condition gate is a divergence waiting to happen. + + ``delay=0.0`` rather than :meth:`_arm_from_deadline`'s + ``_OVERDUE_REARM_SECS`` beat. That beat exists so an elapsed deadline + does not ambush a user mid-conversation — they keep deferring it simply + by typing. A manual trigger IS the user asking, so the condition the beat + protects against is not present. + + Three refusals, and each one is load-bearing rather than defensive: + + * **Not registered** -> 404. Nothing to fire. This is also where the stop + SENTINEL lands: it goes through ``remove``, so the loop is gone rather + than merely inactive. + * **Not active** -> 409. The non-removing terminal bounds — the cycle + cap, the wall-clock budget and the approval stall — all leave the loop + registered but inactive, so this ONE condition covers them without + restating the list. A manual press must not buy a turn past a bound the + user armed. + * **Mid-fire** -> 409. :meth:`_arm_timer` cancels the existing timer + task, and during the fire window that task may be parked on + ``_persist_locked()`` writing the delivered cycle; cancelling it there + loses the ``cycle_count`` bump. This is the same window + ``notify_turn_complete``/``notify_user_input`` defer around, and the + same answer the sibling immediate-trigger route gives for a run + already in flight (``POST /api/crons/{id}/run`` -> 409). + + NO SUSPENSION POINT, and that is the design rather than an omission. + ``async def`` for the caller's convenience, but nothing inside awaits, so + the guards and the arm are atomic with respect to the event loop: between + reading ``loop`` and arming it, no other coroutine can run. + + This shape was arrived at the hard way and the history is worth keeping. + An earlier revision wrote ``next_due_ts = time.time()`` and awaited a + DURABLE persist before arming, so a restart between the press and the + fire would resume overdue. That await was a suspension window, and this + module has several writers to ``next_due_ts`` that hold NO lock while + writing it — the quiet-tick re-arm on the gated-wake branch is one. Each + guard added to close one writer's window exposed the next: a concurrent + ``remove`` arming a stale object, a cancelled caller abandoning the write, + a countdown entering ``_firing`` mid-persist, a quiet tick overwriting the + deadline, then the refused path leaving its own value durably committed. + Five rounds, each caused by the fix before it. The window is not closable + at this call site, because the racing writers take no lock at all. + + SO THE WRITE IS GONE. ``_arm_timer(delay=0.0)`` is what brings the cycle + forward: :meth:`_timer` sleeps the delay it is given and fires WITHOUT + consulting ``next_due_ts``. The write was only ever for restart cosmetics, + and that is exactly what is given up — a gateway restart between the press + and the fire resumes on the loop's own schedule instead of overdue, and + the operator presses again. That is the same degradation + :meth:`_persist_soon` documents as acceptable for every other deadline + assignment in this class ("a lost write degrades to a fresh full countdown + after restart, never a premature or dropped fire"), and a far better trade + than a sixth guard on an uncloseable window. + + The countdown reset issue #8212 asks for is UNAFFECTED, because it never + came from this write: a delivered cycle clears ``next_due_ts`` in + :meth:`_run_fire_cycle` and the re-arm then starts a fresh full interval. + """ + loop = self.get_by_id(loop_id) + if loop is None: + return None, "loop not found", 404 + if not loop.active: + return None, "loop is not active", 409 + if loop_id in self._firing: + return None, "loop is already firing", 409 + self._arm_timer(loop, delay=0.0) + logger.info( + "AutoNudge: loop %s brought forward by hand — cycle %d armed to run now", + loop.id, + loop.cycle_count + 1, + ) + return loop, "", 200 + class _AutoNudgeMaintenanceView: """Store operations that are safe inside ``maintenance_service``'s lock.""" diff --git a/src/kiro_crew/dashboard/handlers/autonudge.py b/src/kiro_crew/dashboard/handlers/autonudge.py index 2c6126d8b9e..ec56ca3993c 100644 --- a/src/kiro_crew/dashboard/handlers/autonudge.py +++ b/src/kiro_crew/dashboard/handlers/autonudge.py @@ -690,3 +690,203 @@ async def api_autonudge_delete(request: web.Request) -> web.Response: metadata={"loop_id": loop_id, "caller": request.remote or ""}, ) return web.json_response({"ok": True}) + + +async def api_autonudge_fire(request: web.Request) -> web.Response: + """POST /api/autonudge/{loop_id}/fire — run this loop's next cycle now. + + The manual counterpart of the idle timer, for the case the loop's interval + cannot serve: the operator already knows the thing being waited on has + changed, so the remaining gap buys nothing. Spelled ``fire`` rather than the + cron sibling's ``run`` because ``fire`` is this subsystem's own verb + (``_on_fire``, ``_run_fire_cycle``, ``last_fire_ts``, the ``fired`` event). + + Thin HTTP mapping, as everywhere else in this file: ``svc.fire_now`` owns the + schedule semantics and the not-registered / not-active / mid-fire refusals, + and documents why each is load-bearing. Two refusals belong here instead, + because they are about the transport's own subject rather than the loop: + + * A **structured monitor** is refused with the same 409 code ``PATCH`` uses. + Those records are driven by ``_on_monitor_tick`` and owned by the monitor + API; the goal popover never sees one, since ``api_autonudge_get`` filters + them out. + * A **busy session** is refused rather than queued, and that is not a fresh + product decision — the fire path this route arms already made it, with its + reason written down at the site: queueing "would stack identical 3KB+ + nudges and blow up the context window" (``_fire_dashboard_nudge``). The + predicate is the repository's canonical one, ``slot.running or + slot._in_stage_execution``, read here exactly as the cron-injection + handler reads it (``handlers/messaging.py``) — ``slot.running`` alone is + False between the stages of a multi-stage plan, so it would let this land + a concurrent turn on top of the plan. Note the two consumers of that + predicate diverge deliberately: the cron path QUEUES, this one REFUSES, + and the nudge path's stated reason is the one that applies here. + + This check is an AFFORDANCE, not a guarantee: a turn that starts between + it and the fire is still refused by the fire path, which then re-arms with + backoff. Its whole job is to turn that silence into a 409 the popover can + show. A loop bound to a channel key has no dashboard slot, so the check + is skipped and that transport's own busy guard answers. + + Authentication is the same as its ``POST`` / ``PATCH`` / ``DELETE`` siblings + on this path, deliberately: no new trust boundary, and this route is + strictly LESS powerful than the ``POST`` beside it, which arms a loop that + can spend turns until a bound stops it. + + **Every outcome is audited, and the FIRE is audit-or-deny.** The split is the + one this repository already draws, not a new policy: + + * The **fire** is gated on a ``critical=True`` write that lands BEFORE + ``fire_now`` arms anything. The default path only ENQUEUES, and on the + event loop an enqueue failure drops the event with a warning + (``sel.py``), so a best-effort pre-audit would still let a model turn run + unrecorded -- the audit would be a hope, not a gate. ``critical`` writes + synchronously and re-raises, and the write is awaited through + ``asyncio.to_thread`` because a synchronous flush on the loop would freeze + every session's turn (``no-blocking-call-on-event-loop``). This is the same + shape this subsystem's own ``autonudge_authz`` uses for ``monitor_update`` + and ``monitor_stop``, and the 503 mirrors ``handlers/cron.py``'s + ``audit_unavailable`` refusal for a grant it could not record. + * The **refusals** stay best-effort. An earlier revision audited only after + ``fire_now`` returned, so the four guards below denied requests and left no + SEL event at all -- that was a real hole and is fixed. But making them + critical would trade an audit-sink problem for a different failure while + preventing nothing: the request is refused either way, so availability must + not hinge on SEL disk health. That is the disposition + ``messaging/identity`` states for a deny and ``azure_client`` states for a + post-action outcome. + * The **terminal** event after ``fire_now`` is best-effort for the same + reason: by then the timer is armed and the ``invoked`` record has landed, + so raising would replace a real result with a logging error. + + Routing every exit through these two helpers is what makes the property + structural rather than a habit: a guard added later cannot silently skip the + record, because there is no un-audited way out. + """ + # Read before the service check so the audit helpers can name the subject + # even on the disabled path. Pure ``match_info`` read; no service needed. + loop_id = request.match_info["loop_id"] + + async def _audit(outcome: str, session_key: str, error: str) -> None: + """Best-effort record for an outcome that did NOT start a turn. + + OFF THE LOOP and failure-swallowing, both for stated reasons. The default + SEL path only enqueues, which is cheap -- but ``sel()`` itself may lazily + initialize the log, and on a degraded sink that initialization is + filesystem work that would run on the gateway's event loop and stall + every session (``no-blocking-call-on-event-loop``). And because this + record accompanies a request that is being REFUSED, its own failure must + not turn a clean 409 into a 500: the caller already learns the outcome + from the status, so the audit is best-effort by contract here, exactly as + the post-action outcome events are elsewhere in the codebase. The + write-ahead ``invoked`` record is the one that fails closed. + """ + try: + await asyncio.to_thread( + lambda: sel().log_tool_invocation( + session_key=session_key, + source="dashboard", + tool_name="autonudge_fire", + outcome=outcome, + metadata={ + "loop_id": loop_id, + "caller": request.remote or "", + "error": error, + }, + ) + ) + except Exception: + logger.warning( + "autonudge fire: refusal audit unavailable (outcome=%s)", + outcome, + exc_info=True, + ) + + async def _audit_or_deny(session_key: str) -> bool: + """Write-ahead audit for the fire. False = do not fire. + + ``sel()`` is resolved INSIDE the worker: on a fresh gateway the lookup + lazily initializes the log, which is itself filesystem work that must not + run on the event loop. + """ + try: + await asyncio.to_thread( + lambda: sel().log_tool_invocation( + session_key=session_key, + source="dashboard", + tool_name="autonudge_fire", + outcome="invoked", + critical=True, + metadata={"loop_id": loop_id, "caller": request.remote or ""}, + ) + ) + except Exception: + logger.error("autonudge fire denied: SEL audit unavailable", exc_info=True) + return False + return True + + svc = _autonudge_get() + if svc is None: + await _audit("denied", "", "autonudge_disabled") + return web.json_response( + {"error": "auto-nudge disabled", "code": "autonudge_disabled"}, + status=503, + ) + existing = svc.get_by_id(loop_id) + if existing is None: + await _audit("denied", "", "autonudge_not_found") + return web.json_response( + {"error": "loop not found", "code": "autonudge_not_found"}, status=404 + ) + if is_structured_monitor_loop(existing): + await _audit("denied", existing.slot_key, "structured_monitor_requires_monitor_api") + return _monitor_error( + "structured monitors must use the monitor update API", + "structured_monitor_requires_monitor_api", + status=409, + ) + state: DashboardState = request.app["state"] + slot = state.get_slot(existing.slot_key) + if slot is not None and (slot.running or slot._in_stage_execution): + # Names the OUTCOME and the NEXT STEP, not just the condition. "a turn is + # in flight" leaves a reader unable to tell a refusal from a delay, and + # the distinction is the whole point here: the press was refused, not + # queued, so trying again later is the action. "still working" rather + # than "mid-turn": a usability reader could only guess at the latter, + # which is jargon from this codebase's vocabulary and not the user's. + # Lowercase-first because all 13 error messages in this file are, and + # this body is rendered verbatim beside them. + await _audit("denied", existing.slot_key, "session_busy") + return web.json_response( + { + "error": "nudge not sent: the agent is still working, so try again when it finishes", + "code": "session_busy", + }, + status=409, + ) + if not await _audit_or_deny(existing.slot_key): + # Fail closed, with nothing armed: the deadline has not moved and no + # timer was re-armed, so the loop is exactly as the operator left it. + return web.json_response( + { + "error": "audit log unavailable: the nudge was NOT sent, " + "so fix the audit store and press again", + "code": "audit_unavailable", + }, + status=503, + ) + loop, error, status = await svc.fire_now(loop_id) + await _audit("success" if error == "" else "denied", existing.slot_key, error) + if error: + # Each arm carries a LITERAL status beside its code, rather than passing + # ``status=status`` through. The error-code contract caps dynamic + # statuses for a stated reason — computing one is how the coded-response + # ratchet gets defeated while looking like ordinary refactoring — so the + # pairing is written out where a reader and a static check can both see + # it. Both arms are kept even though this route answers ``not found`` + # itself above: relying on the 404 being unreachable would make a later + # edit to that guard silently change this response's status. + if status == 404: + return web.json_response({"error": error, "code": "autonudge_not_found"}, status=404) + return web.json_response({"error": error, "code": "autonudge_not_fired"}, status=409) + return web.json_response({"ok": True, "loop": _serialize(loop)}) diff --git a/src/kiro_crew/dashboard/server.py b/src/kiro_crew/dashboard/server.py index a084f2516fb..aefc488a23d 100644 --- a/src/kiro_crew/dashboard/server.py +++ b/src/kiro_crew/dashboard/server.py @@ -1483,6 +1483,7 @@ def _register_mcp_routes(app: web.Application) -> None: # Auto-nudge (feature-flagged — returns 503 when KIROCREW_AUTONUDGE unset) from kiro_crew.dashboard.handlers.autonudge import ( api_autonudge_delete, + api_autonudge_fire, api_autonudge_get, api_autonudge_list, api_autonudge_start, @@ -1502,6 +1503,7 @@ def _register_mcp_routes(app: web.Application) -> None: app.router.add_get("/api/autonudge/slot/{slot_key}", api_autonudge_get) app.router.add_patch("/api/autonudge/{loop_id}", api_autonudge_update) app.router.add_delete("/api/autonudge/{loop_id}", api_autonudge_delete) + app.router.add_post("/api/autonudge/{loop_id}/fire", api_autonudge_fire) app.router.add_get("/api/monitors", api_monitors_list) app.router.add_post("/api/monitors", api_monitor_create) app.router.add_get("/api/monitors/slot/{slot_key}", api_monitor_slot_get) diff --git a/temp-screenshots/goal-trigger-nudge-8212/01-armed-loop-trigger-offered.png b/temp-screenshots/goal-trigger-nudge-8212/01-armed-loop-trigger-offered.png new file mode 100644 index 00000000000..f1972e1e690 Binary files /dev/null and b/temp-screenshots/goal-trigger-nudge-8212/01-armed-loop-trigger-offered.png differ diff --git a/temp-screenshots/goal-trigger-nudge-8212/02-paused-loop-trigger-absent.png b/temp-screenshots/goal-trigger-nudge-8212/02-paused-loop-trigger-absent.png new file mode 100644 index 00000000000..fa73af9d97d Binary files /dev/null and b/temp-screenshots/goal-trigger-nudge-8212/02-paused-loop-trigger-absent.png differ diff --git a/temp-screenshots/goal-trigger-nudge-8212/03-refused-while-a-turn-is-in-flight.png b/temp-screenshots/goal-trigger-nudge-8212/03-refused-while-a-turn-is-in-flight.png new file mode 100644 index 00000000000..2b108c9fd55 Binary files /dev/null and b/temp-screenshots/goal-trigger-nudge-8212/03-refused-while-a-turn-is-in-flight.png differ diff --git a/temp-screenshots/goal-trigger-nudge-8212/04-pressed-schedule-line-reads-due.png b/temp-screenshots/goal-trigger-nudge-8212/04-pressed-schedule-line-reads-due.png new file mode 100644 index 00000000000..7e46b5f0cec Binary files /dev/null and b/temp-screenshots/goal-trigger-nudge-8212/04-pressed-schedule-line-reads-due.png differ diff --git a/temp-screenshots/goal-trigger-nudge-8212/05-narrow-320px-nothing-clipped.png b/temp-screenshots/goal-trigger-nudge-8212/05-narrow-320px-nothing-clipped.png new file mode 100644 index 00000000000..3d0db571d26 Binary files /dev/null and b/temp-screenshots/goal-trigger-nudge-8212/05-narrow-320px-nothing-clipped.png differ diff --git a/test/test_autonudge_manual_fire.py b/test/test_autonudge_manual_fire.py new file mode 100644 index 00000000000..7f799c7a05b --- /dev/null +++ b/test/test_autonudge_manual_fire.py @@ -0,0 +1,744 @@ +"""Tests for the manual goal-loop trigger (issue #8212). + +The gap these pin: the loop interval is an IDLE gap, so an operator who already +knows the thing being waited on has changed had no way to tell the loop to look +now. There was no out-of-band fire anywhere on the surface — the autonudge HTTP +API was list / get / start / update / delete, and no MCP tool fired either. + +Two properties matter more than the button, and both are asserted here rather +than described in prose: + +* **The schedule.** A manual fire must not silently shift the interval. It does + not, and the reason is that it reuses the delivered-fire bookkeeping: a + delivered cycle clears ``next_due_ts`` and the re-arm then starts a fresh full + interval, so the next automatic nudge lands one ``idle_secs`` after the manual + turn ENDS. Pinned by + ``test_the_next_automatic_nudge_is_a_full_interval_after_the_manual_turn_ends``. +* **The bounds.** A manual press must not buy a turn past a bound the user + armed. ``fire_now`` arms the ordinary ``_timer`` body rather than calling + ``_run_fire_cycle`` directly, so every terminal gate still runs. Pinned per + gate — the cycle cap and the stop sentinel each get their own test, because a + single test would only be proven to the first gate that fires. + +The refusals are pinned too, and the mid-fire one is load-bearing rather than +defensive: ``_arm_timer`` cancels the existing timer task, and inside the fire +window that task may be parked on ``_persist_locked()`` writing the delivered +cycle. +""" + +from __future__ import annotations + +import asyncio +import json +from typing import Any +from unittest.mock import MagicMock + +import pytest +from aiohttp import web +from aiohttp.test_utils import make_mocked_request + +from kiro_crew.autonudge import AutoNudgeService, NudgeLoop +from kiro_crew.dashboard.handlers import autonudge as h +from kiro_crew.monitoring.models import MonitorState + + +@pytest.fixture(autouse=True) +def _enable(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("KIROCREW_AUTONUDGE", "1") + + +@pytest.fixture(scope="session") +def svc_base_dir(tmp_path_factory: pytest.TempPathFactory): + """A SESSION-scoped store directory for every service these tests build. + + Not each test's ``tmp_path``. ``_persist_soon`` dispatches its write through + ``run_in_executor``, and a thread already inside ``_write_state`` cannot be + stopped -- cancelling the awaiting task does not reach it. So a late write can + land after a per-test directory is torn down and RECREATE it, a side effect + that outlives the test. + + An earlier revision drained those writers at teardown instead. That narrows + the window but cannot close it, because the cancel branch leaves the thread + running; the review that pressed on it was right. Giving the writes a + directory nobody deletes mid-run removes the hazard rather than racing it. + """ + return tmp_path_factory.mktemp("autonudge-manual-fire") + + +def _loop(**over: Any) -> NudgeLoop: + fields: dict[str, Any] = { + "id": "lp-1", + "slot_key": "chat-1-111", + "message": "check the PR", + "idle_secs": 300, + "max_cycles": 24, + "cycle_count": 3, + # A live loop mid-countdown: the value the press must move. + "next_due_ts": 9_999_999_999.0, + } + fields.update(over) + return NudgeLoop(**fields) + + +async def _run_armed_cycle(svc: AutoNudgeService, loop_id: str) -> None: + """Await the timer task ``fire_now`` just armed, to completion. + + The suite's established shape (``await svc._timers[loop.id]`` in + ``test_autonudge.py``). Captured by REFERENCE before any await, because the + gates inside ``_timer`` reach ``update``/``remove``, which pop the entry from + ``_timers`` — reading the dict afterwards would raise KeyError on exactly + the paths this file is here to test. Draining "until no tasks remain" + instead is wrong: ``_persist_soon`` schedules a supervised background write, + so that condition is never reached. + """ + task = svc._timers[loop_id] + await asyncio.gather(task, return_exceptions=True) + + +# --------------------------------------------------------------------------- # +# AutoNudgeService.fire_now +# --------------------------------------------------------------------------- # + + +@pytest.mark.asyncio +async def test_a_manual_trigger_delivers_the_cycle_through_the_ordinary_fire_path( + svc_base_dir, monkeypatch +) -> None: + """The press runs the SAME delivery the timer runs, and counts as a turn. + + ``cycle_count`` is documented as counting DELIVERED TURNS, and a manual + nudge delivers one and spends a model turn exactly as a scheduled one does, + so it advances the counter. That falls out of reusing ``_run_fire_cycle`` + rather than being decided here — there is no manual-vs-scheduled branch to + get wrong. + """ + fired: list[NudgeLoop] = [] + + async def on_fire(loop: NudgeLoop) -> bool: + fired.append(loop) + return True + + svc = AutoNudgeService(base_dir=svc_base_dir, on_fire=on_fire) + loop = _loop() + svc._loops[loop.id] = loop + + # The press must not TOUCH the deadline. An earlier revision wrote + # ``next_due_ts = now`` and awaited a durable persist so a restart would + # resume overdue; that await was a suspension window, and this module has + # several writers to that field which hold no lock, so five successive races + # came out of it. The write is gone and the arm is what brings the cycle + # forward, so the field must read exactly as the loop left it. + due_at_arm: list[float] = [] + real_arm = svc._arm_timer + + def _arm(lp: NudgeLoop, **kw: Any) -> None: + due_at_arm.append(lp.next_due_ts) + real_arm(lp, **kw) + + monkeypatch.setattr(svc, "_arm_timer", _arm) + + armed, error, status = await svc.fire_now(loop.id) + + assert (error, status) == ("", 200) + assert armed is loop + assert due_at_arm == [9_999_999_999.0], ( + "fire_now moved the deadline; the write was removed because it cannot be " + "made durable here without a suspension point" + ) + await _run_armed_cycle(svc, loop.id) + assert [lp.id for lp in fired] == ["lp-1"] + assert loop.cycle_count == 4 + # Cleared by the delivered-fire bookkeeping, which is what makes the re-arm + # below start a fresh full interval instead of resuming a stale deadline. + assert loop.next_due_ts == 0.0 + svc.stop() + + +@pytest.mark.asyncio +async def test_the_next_automatic_nudge_is_a_full_interval_after_the_manual_turn_ends( + svc_base_dir, monkeypatch: pytest.MonkeyPatch +) -> None: + """Issue #8212's second requirement, and the one a defect would hide in. + + "Reset the countdown to zero, so the next automatic nudge is a full interval + away from the manual one." Measured from the end of the manual TURN, which + is when ``notify_turn_complete`` fires — not from the button press, because + the turn is what the interval is an idle gap between. + """ + + async def on_fire(_loop: NudgeLoop) -> bool: + return True + + svc = AutoNudgeService(base_dir=svc_base_dir, on_fire=on_fire) + loop = _loop(idle_secs=300) + svc._loops[loop.id] = loop + + await svc.fire_now(loop.id) + await _run_armed_cycle(svc, loop.id) + assert loop.next_due_ts == 0.0 # delivered + + # Freeze the clock only for the re-arm, so the assertion is an equality + # rather than a tolerance window a slow host could widen past. + monkeypatch.setattr("kiro_crew.autonudge.time.time", lambda: 1_700_000_000.0) + svc.notify_turn_complete(loop.slot_key) + + assert loop.next_due_ts == 1_700_000_000.0 + 300 + svc.stop() + + +@pytest.mark.asyncio +async def test_a_manual_trigger_cannot_buy_a_turn_past_the_cycle_cap(svc_base_dir) -> None: + """The cap gate still runs, because the press arms ``_timer`` not the fire. + + An active loop already at its cap is reachable — the cap is checked on the + tick BEFORE the fire, so a store written at ``cycle_count == max_cycles`` + (or a loop whose last delivery reached it) is live until its next tick. A + manual press on it must deactivate, exactly as that tick would, rather than + deliver one more turn. + """ + fired: list[NudgeLoop] = [] + + async def on_fire(loop: NudgeLoop) -> bool: + fired.append(loop) + return True + + svc = AutoNudgeService(base_dir=svc_base_dir, on_fire=on_fire) + loop = _loop(max_cycles=4, cycle_count=4) + svc._loops[loop.id] = loop + + _armed, error, status = await svc.fire_now(loop.id) + assert (error, status) == ("", 200) + await _run_armed_cycle(svc, loop.id) + + assert fired == [] + assert loop.active is False + assert loop.stopped_reason == "cycle_cap" + svc.stop() + + +@pytest.mark.asyncio +async def test_a_manual_trigger_still_honours_the_stop_sentinel(tmp_path, svc_base_dir) -> None: + """Second gate, second test: the kill switch is not bypassed by the button. + + Its own test rather than an extra assertion on the cap one, because a test + is only proven to the first gate that fires — the cap returns before the + sentinel check would ever be reached, so one test cannot cover both. + """ + fired: list[NudgeLoop] = [] + + async def on_fire(loop: NudgeLoop) -> bool: + fired.append(loop) + return True + + sentinel = tmp_path / "STOP" + sentinel.write_text("halt", encoding="utf-8") + svc = AutoNudgeService(base_dir=svc_base_dir, on_fire=on_fire) + loop = _loop(stop_sentinel_path=str(sentinel)) + svc._loops[loop.id] = loop + + _armed, error, status = await svc.fire_now(loop.id) + assert (error, status) == ("", 200) + await _run_armed_cycle(svc, loop.id) + + assert fired == [] + assert loop.id not in svc._loops + svc.stop() + + +@pytest.mark.asyncio +async def test_fire_now_refuses_an_inactive_loop(svc_base_dir) -> None: + """One condition covers every terminal bound, so none is restated. + + A cap, a spent runtime budget, an approval stall and a sentinel removal all + leave the loop inactive, so refusing on ``active`` refuses all of them + without a second copy of the list to drift. + """ + fired: list[NudgeLoop] = [] + + async def on_fire(loop: NudgeLoop) -> bool: + fired.append(loop) + return True + + svc = AutoNudgeService(base_dir=svc_base_dir, on_fire=on_fire) + loop = _loop(active=False) + svc._loops[loop.id] = loop + + armed, error, status = await svc.fire_now(loop.id) + + assert armed is None + assert status == 409 + assert error == "loop is not active" + assert loop.id not in svc._timers + assert fired == [] + svc.stop() + + +@pytest.mark.asyncio +async def test_fire_now_refuses_a_loop_that_is_already_firing(svc_base_dir) -> None: + """Load-bearing, not defensive: arming would cancel the firing task. + + ``_arm_timer`` cancels the existing timer before it creates a new one, and + inside the fire window that task may be parked on ``_persist_locked()`` + writing the delivered cycle — cancelling it there loses the ``cycle_count`` + bump. The assertion is therefore that the IN-FLIGHT TASK SURVIVES, not + merely that a 409 came back. + """ + svc = AutoNudgeService(base_dir=svc_base_dir) + loop = _loop() + svc._loops[loop.id] = loop + + started = asyncio.Event() + release = asyncio.Event() + + async def in_flight() -> None: + started.set() + await release.wait() + + task = asyncio.create_task(in_flight()) + await started.wait() + svc._timers[loop.id] = task + svc._firing.add(loop.id) + + armed, error, status = await svc.fire_now(loop.id) + + assert armed is None + assert status == 409 + assert error == "loop is already firing" + assert svc._timers[loop.id] is task + assert not task.cancelled() + assert not task.done() + + release.set() + await task + svc._firing.discard(loop.id) + svc.stop() + + +@pytest.mark.asyncio +async def test_fire_now_refuses_a_loop_the_service_does_not_hold(svc_base_dir) -> None: + """404, resolved through the shared ``get_by_id`` accessor.""" + svc = AutoNudgeService(base_dir=svc_base_dir) + + armed, error, status = await svc.fire_now("no-such-loop") + + assert armed is None + assert status == 404 + assert error == "loop not found" + svc.stop() + + +# --------------------------------------------------------------------------- # +# POST /api/autonudge/{loop_id}/fire +# --------------------------------------------------------------------------- # + + +class _FakeSvc: + """Only what the fire route calls.""" + + def __init__(self, loops: list[NudgeLoop]) -> None: + self.loops = loops + self.fired: list[str] = [] + self.result: tuple[NudgeLoop | None, str, int] | None = None + + def get_by_id(self, loop_id: str) -> NudgeLoop | None: + return next((lp for lp in self.loops if lp.id == loop_id), None) + + async def fire_now(self, loop_id: str) -> tuple[NudgeLoop | None, str, int]: + self.fired.append(loop_id) + if self.result is not None: + return self.result + return self.get_by_id(loop_id), "", 200 + + +def _svc_with_loop(svc_base_dir: Any) -> tuple[AutoNudgeService, NudgeLoop]: + """A real service holding one live loop, for the service-level assertions. + + Built inline the way every other service test here builds it, rather than as + a fixture: these tests patch instance methods (``_persist_locked``, + ``_arm_timer``) and a shared fixture would hide which one each test replaced. + """ + + async def _on_fire(loop: NudgeLoop) -> bool: + return True + + svc = AutoNudgeService(base_dir=svc_base_dir, on_fire=_on_fire) + loop = _loop() + svc._loops[loop.id] = loop + return svc, loop + + +@pytest.fixture(autouse=True) +def sel_mock(monkeypatch: pytest.MonkeyPatch) -> MagicMock: + sink = MagicMock() + monkeypatch.setattr(h, "sel", lambda: sink) + return sink + + +def _mk(loop_id: str, *, slot: Any = None) -> web.Request: + app = web.Application() + state = MagicMock() + state.get_slot = MagicMock(return_value=slot) + app["state"] = state + req = make_mocked_request( + "POST", + f"/api/autonudge/{loop_id}/fire", + app=app, + match_info={"loop_id": loop_id}, + ) + req["user"] = "local-app" + return req + + +def _body(response: web.StreamResponse) -> dict: + assert isinstance(response, web.Response) + raw = response.body + assert isinstance(raw, bytes) + return json.loads(raw.decode("utf-8")) + + +def _slot(*, running: bool = False, in_stage: bool = False) -> MagicMock: + slot = MagicMock() + slot.running = running + # Modelled explicitly: a bare MagicMock attribute is truthy and would trip + # the busy guard on every test. + slot._in_stage_execution = in_stage + return slot + + +@pytest.mark.asyncio +async def test_route_fires_and_returns_the_updated_loop(monkeypatch) -> None: + loop = NudgeLoop(id="lp-1", slot_key="chat-1-111", message="check", idle_secs=300) + svc = _FakeSvc([loop]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=_slot())) + + assert resp.status == 200 + body = _body(resp) + assert body["ok"] is True + assert body["loop"]["id"] == "lp-1" + assert svc.fired == ["lp-1"] + + +@pytest.mark.asyncio +async def test_route_refuses_when_the_session_already_has_a_turn_in_flight(monkeypatch) -> None: + """Refused, not queued — and the fire path already decided that. + + Its own comment states the reason: queueing "would stack identical 3KB+ + nudges and blow up the context window". So this is the repository's recorded + answer being surfaced as a 409, not a new product decision. + """ + loop = NudgeLoop(id="lp-1", slot_key="chat-1-111", message="check", idle_secs=300) + svc = _FakeSvc([loop]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=_slot(running=True))) + + assert resp.status == 409 + assert _body(resp)["code"] == "session_busy" + assert svc.fired == [] + + +@pytest.mark.asyncio +async def test_route_refuses_between_the_stages_of_a_multi_stage_plan(monkeypatch) -> None: + """``slot.running`` alone reads False in that window. + + Which is exactly why the canonical predicate is two-term. Without the + ``_in_stage_execution`` half this press would land a concurrent turn on top + of a plan that is mid-flight. + """ + loop = NudgeLoop(id="lp-1", slot_key="chat-1-111", message="check", idle_secs=300) + svc = _FakeSvc([loop]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=_slot(running=False, in_stage=True))) + + assert resp.status == 409 + assert _body(resp)["code"] == "session_busy" + assert svc.fired == [] + + +@pytest.mark.asyncio +async def test_route_refuses_a_structured_monitor(monkeypatch) -> None: + """Same 409 code ``PATCH`` gives: those records belong to the monitor API.""" + loop = NudgeLoop(id="mon-1", slot_key="chat-1-111", message="check", idle_secs=300) + loop.monitor = MonitorState( + kind="github_pull_request", + target="https://github.com/acme/widgets/pull/7", + objective="review_ready", + created_ts=1.0, + ) + svc = _FakeSvc([loop]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + resp = await h.api_autonudge_fire(_mk("mon-1", slot=_slot())) + + assert resp.status == 409 + assert _body(resp)["code"] == "structured_monitor_requires_monitor_api" + assert svc.fired == [] + + +@pytest.mark.asyncio +async def test_route_reports_404_for_an_unknown_loop(monkeypatch) -> None: + svc = _FakeSvc([]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + resp = await h.api_autonudge_fire(_mk("nope", slot=_slot())) + + assert resp.status == 404 + assert _body(resp)["code"] == "autonudge_not_found" + + +@pytest.mark.asyncio +async def test_route_reports_503_when_the_feature_is_off(monkeypatch) -> None: + monkeypatch.setattr(h, "_autonudge_get", lambda: None) + + resp = await h.api_autonudge_fire(_mk("lp-1")) + + assert resp.status == 503 + assert _body(resp)["code"] == "autonudge_disabled" + + +@pytest.mark.asyncio +async def test_route_surfaces_a_service_refusal_and_audits_it_as_denied( + monkeypatch, sel_mock: MagicMock +) -> None: + """A denied press is audited too, not only a successful one. + + A delivered cycle spends a model turn, so both outcomes belong in the audit + for the same reason ``DELETE`` records its own. + """ + loop = NudgeLoop(id="lp-1", slot_key="chat-1-111", message="check", idle_secs=300) + svc = _FakeSvc([loop]) + svc.result = (None, "loop is already firing", 409) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=_slot())) + + assert resp.status == 409 + assert _body(resp)["error"] == "loop is already firing" + kwargs = sel_mock.log_tool_invocation.call_args.kwargs + assert kwargs["tool_name"] == "autonudge_fire" + assert kwargs["outcome"] == "denied" + assert kwargs["session_key"] == "chat-1-111" + + +@pytest.mark.asyncio +async def test_route_skips_the_busy_check_for_a_channel_bound_loop(monkeypatch) -> None: + """No dashboard slot exists for a ``slack:`` key, so that transport answers. + + Reading a missing slot as "not busy" is the correct fallthrough here: the + channel fire paths carry their own ``is_busy`` guard, and refusing on a slot + lookup that can never succeed would make the route permanently unusable for + those loops. + """ + loop = NudgeLoop(id="lp-1", slot_key="slack:C1.123", message="check", idle_secs=300) + svc = _FakeSvc([loop]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=None)) + + assert resp.status == 200 + assert svc.fired == ["lp-1"] + + +@pytest.mark.asyncio +async def test_every_early_refusal_is_audited_as_denied(monkeypatch, sel_mock: MagicMock) -> None: + """No exit from this route is un-audited, including the four early guards. + + The first revision audited only after ``fire_now`` returned, so a request + denied by an earlier guard left no SEL event at all -- a denied permission + decision invisible to the audit trail, which is the one thing that trail + exists to record. Asserted as a LOOP over every refusing guard rather than + one test per guard on purpose: the property is "there is no un-audited way + out", and a guard added later that forgets the event should fail an existing + test rather than wait for someone to remember to write a new one. + + ``sel_mock`` is reset between cases so each assertion is about its own + request; a cumulative call count would pass even if one case emitted two + events and another emitted none. + """ + disabled = object() # sentinel: the 503 arm never touches the service + + cases: list[tuple[str, Any, int, str, str]] = [ + # (label, service, expected status, expected code, expected audited key) + ("feature disabled", disabled, 503, "autonudge_disabled", ""), + ("unknown loop", _FakeSvc([]), 404, "autonudge_not_found", ""), + ( + "structured monitor", + _FakeSvc( + [ + NudgeLoop( + id="lp-1", + slot_key="chat-1-111", + message="check", + idle_secs=300, + monitor=MonitorState( + kind="github_pull_request", + target="https://github.com/acme/widgets/pull/7", + objective="review_ready", + created_ts=1.0, + ), + ) + ] + ), + 409, + "structured_monitor_requires_monitor_api", + "chat-1-111", + ), + ] + + for label, svc, want_status, want_code, want_session in cases: + sel_mock.reset_mock() + monkeypatch.setattr( + h, "_autonudge_get", (lambda: None) if svc is disabled else (lambda s=svc: s) + ) + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=_slot())) + + assert resp.status == want_status, label + assert _body(resp)["code"] == want_code, label + assert sel_mock.log_tool_invocation.call_count == 1, f"{label}: one event" + kwargs = sel_mock.log_tool_invocation.call_args.kwargs + assert kwargs["tool_name"] == "autonudge_fire", label + assert kwargs["outcome"] == "denied", label + assert kwargs["session_key"] == want_session, label + # The loop id is recorded even on the disabled path, which is why it is + # read before the service lookup rather than after it. + assert kwargs["metadata"]["loop_id"] == "lp-1", label + assert kwargs["metadata"]["error"] == want_code, label + + +@pytest.mark.asyncio +async def test_the_busy_refusal_is_audited_as_denied(monkeypatch, sel_mock: MagicMock) -> None: + """The busy guard specifically -- it is the one a reviewer flagged. + + Kept separate from the loop above because it needs a slot in a distinct + state rather than a distinct service, and because it is the refusal an + operator actually hits: a mid-turn agent is a common state, not an edge one, + so this is the audit event most likely to matter in a real trail. + """ + loop = NudgeLoop(id="lp-1", slot_key="chat-1-111", message="check", idle_secs=300) + svc = _FakeSvc([loop]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=_slot(running=True))) + + assert resp.status == 409 + assert _body(resp)["code"] == "session_busy" + assert svc.fired == [], "a refused press must not fire" + assert sel_mock.log_tool_invocation.call_count == 1 + kwargs = sel_mock.log_tool_invocation.call_args.kwargs + assert kwargs["outcome"] == "denied" + assert kwargs["session_key"] == "chat-1-111" + assert kwargs["metadata"]["error"] == "session_busy" + + +@pytest.mark.asyncio +async def test_a_press_that_cannot_be_audited_does_not_fire( + monkeypatch, sel_mock: MagicMock +) -> None: + """AUDIT-OR-DENY: no record, no turn. The load-bearing half of the gate. + + A delivered cycle spends a model turn with nobody watching, so "which press + started this turn, and when" is the only evidence it happened. The default + SEL path merely ENQUEUES, and on the event loop an enqueue failure drops the + event with a warning -- so auditing before the fire is not enough on its own. + ``critical=True`` is what makes it a gate rather than a hope, and this test + fails if the pre-fire write is ever downgraded to best-effort. + + Modelled as an ASYMMETRIC sink, exactly as the repository's other + audit-or-deny tests do: the queued (non-critical) refusal writes succeed and + only the critical one raises. A stub that failed every write would pass even + against a route that never asked for durability. + """ + loop = NudgeLoop(id="lp-1", slot_key="chat-1-111", message="check", idle_secs=300) + svc = _FakeSvc([loop]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + + def _log(**kw: Any) -> None: + if kw.get("critical"): + raise OSError("audit sink is unwritable") + + sel_mock.log_tool_invocation.side_effect = _log + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=_slot())) + + assert resp.status == 503 + assert _body(resp)["code"] == "audit_unavailable" + # The whole point: the loop is exactly as the operator left it. + assert svc.fired == [], "fired despite an unwritable audit sink" + + +@pytest.mark.asyncio +async def test_the_pre_fire_audit_is_critical_and_lands_before_the_fire( + monkeypatch, sel_mock: MagicMock +) -> None: + """Ordering AND durability, asserted together rather than assumed. + + Two separate ways this could be wrong, so both are pinned: a critical write + placed AFTER ``fire_now`` would satisfy "is critical" while still arming the + timer first, and a correctly-placed write that is not critical would satisfy + "is first" while dropping the record on a full disk. The ``order`` list + interleaves the audit calls with the fire, so the assertion reads the real + sequence instead of trusting the source layout. + """ + loop = NudgeLoop(id="lp-1", slot_key="chat-1-111", message="check", idle_secs=300) + order: list[str] = [] + + class _Svc(_FakeSvc): + async def fire_now(self, loop_id: str): # type: ignore[override] + order.append("fire") + return await super().fire_now(loop_id) + + svc = _Svc([loop]) + monkeypatch.setattr(h, "_autonudge_get", lambda: svc) + sel_mock.log_tool_invocation.side_effect = lambda **kw: order.append( + f"audit:{kw['outcome']}:critical={bool(kw.get('critical'))}" + ) + + resp = await h.api_autonudge_fire(_mk("lp-1", slot=_slot())) + + assert resp.status == 200 + assert order == [ + "audit:invoked:critical=True", + "fire", + "audit:success:critical=False", + ], order + + +@pytest.mark.asyncio +async def test_fire_now_never_suspends_which_is_what_makes_it_race_free(svc_base_dir) -> None: + """THE invariant. Five race classes were closed by deleting the await, not by + guarding it, so this asserts the absence directly. + + A coroutine with no ``await`` inside completes on its FIRST ``send`` -- it + raises ``StopIteration`` carrying the return value rather than yielding a + future to the loop. So this is a mechanical check that no suspension point + exists between reading the loop and arming it, which is precisely why a + concurrent ``remove``, a cancelled caller, a countdown entering ``_firing``, + a quiet-tick reschedule and a half-committed deadline are all impossible + here rather than merely handled. + + Written against the raw coroutine on purpose: ``await svc.fire_now(...)`` + would pass whether or not it suspends, so it cannot distinguish the two. + Anyone reintroducing an ``await`` in this method fails this test with a + message that says what it costs. + """ + svc, loop = _svc_with_loop(svc_base_dir) + coro = svc.fire_now(loop.id) + try: + coro.send(None) + except StopIteration as done: + result, error, status = done.value + assert (error, status) == ("", 200) + assert result is loop + else: + coro.close() + raise AssertionError( + "fire_now suspended. Any await between the guards and the arm reopens " + "the remove race, the cancellation window, the mid-persist _firing " + "window and the quiet-tick deadline overwrite -- none of which can be " + "closed here, because those writers take no lock." + ) + svc.stop() diff --git a/website/scripts/capture-goal-trigger-nudge-8212.mjs b/website/scripts/capture-goal-trigger-nudge-8212.mjs new file mode 100644 index 00000000000..4ce91ce7aa5 --- /dev/null +++ b/website/scripts/capture-goal-trigger-nudge-8212.mjs @@ -0,0 +1,353 @@ +/** + * Screenshot harness + assertions for the GOAL-LOOP MANUAL TRIGGER (#8212). + * + * The goal popover gains one button. Three frames, because a still of a button + * proves the least interesting third of the change: + * + * 1. armed loop -> "Trigger nudge" sits on the SCHEDULE line, beside the + * countdown it acts on, and the Stop/Save action row below + * is back to two controls (website/AUTOSDE.yaml:230 holds a + * row to two and names "leaves the row" as the escape). + * 2. PAUSED loop -> the button is ABSENT. The control frame: without it, + * frame 1 proves only that a button can render, not that it + * renders when it should. It is gated on `active` because + * every terminal bound leaves the loop inactive and the + * server refuses to fire one, so a button there could only + * ever produce a 409. + * 3. refusal -> the fire route answers 409 `session_busy` and the popover + * shows the reason inline and STAYS OPEN. This is the frame + * that carries the answer to the issue's queue-vs-disable + * question: a press during a live turn is refused visibly + * rather than queued silently. No still of a button can show + * that, which is why it is photographed rather than argued. + * + * This ASSERTS as well as photographs, because a PNG cannot fail. It drives the + * REAL built SPA (website/dist) behind `serveDist` with every /api/** call + * answered from fixtures by `stubDashboardApi` -- no gateway, no dashboard auth, + * no kiro-cli -- and exits non-zero unless each frame renders what the PR + * claims. A stale bundle therefore reds instead of quietly photographing the old + * copy, and a blank frame cannot be committed as evidence. + * + * Labels are read from the CATALOG, so a key rename breaks the capture loudly + * instead of silently screenshotting the wrong element. + * + * Usage: node scripts/capture-goal-trigger-nudge-8212.mjs [outDir] + */ +import { chromium } from 'playwright' +import { mkdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' + +import { json } from './lib/boot-api.mjs' +import { serveDist } from './lib/serve-dist.mjs' +import { logPageProblems, stubDashboardApi } from './lib/stub-dashboard-api.mjs' + +const OUT = process.argv[2] || '../temp-screenshots/goal-trigger-nudge-8212' +const SLOT = 'chat-loop' +const PROJECT = '/home/user/workspace/uploader' +const LOOP_ID = 'loop-8212' + +mkdirSync(OUT, { recursive: true }) + +const LOCALES = fileURLToPath(new URL('../src/i18n/locales/', import.meta.url)) +const manual = JSON.parse(readFileSync(LOCALES + 'en.manual.json', 'utf-8')) +const gen = JSON.parse(readFileSync(LOCALES + 'en.json', 'utf-8')) +const TRIGGER = manual.components.autoNudgePopover.trigger_nudge +const SAVE = manual.components.autoNudgePopover.save +const STOP = gen.components.autoNudgePopover.stop_loop +if (!TRIGGER || !SAVE || !STOP) { + throw new Error('components.autoNudgePopover trigger/save/stop keys missing -- renamed?') +} + +const NOW = Math.floor(Date.now() / 1000) +/** + * Fixed wall-clock instant for the loop's timestamps so the popover's "Last + * fire" line renders the same string on every run. A now-relative value would + * rewrite the committed PNG's bytes on every re-capture, turning a re-pin after + * a rebase into a pointless binary diff. + */ +const FIXED_FIRE_TS = Date.UTC(2026, 8, 6, 9, 40, 0) / 1000 + +const slots = [{ + key: SLOT, + title: 'Drive PR 8188 to green', + running: false, + last_message: 'Cycle 3 done -- waiting on the Windows shard.', + messages: 4, + agent: 'kirocrew', + memory_mode: 'persistent', + project: PROJECT, + folder_id: '', + modified: NOW, + source_links: [], + source_links_total: 0, +}] + +const detail = { + running: false, + has_more: false, + total: 2, + queue: [], + project: PROJECT, + messages: [ + { role: 'user', ts: NOW - 900, content: 'Keep checking PR 8188 until the board settles.' }, + { role: 'assistant', ts: NOW - 120, content: 'Cycle 3 done -- the Windows shard is still running.' }, + ], +} + +/** + * One loop, shaped like the backend's `asdict(loop)`. + * + * `next_due_ts` is 0 on purpose: a live countdown puts a per-second value in the + * popover's own footer line, so two runs would never produce the same bytes. + */ +const makeLoop = over => ({ + id: LOOP_ID, + slot_key: SLOT, + message: 'Check https://github.com/kirodotdev/KiroCrew/pull/8188 for new CI results and review comments.', + idle_secs: 300, + max_cycles: 24, + cycle_count: 3, + active: true, + last_fire_ts: FIXED_FIRE_TS, + next_due_ts: 0, + ...over, +}) + +const { srv, base } = await serveDist() +const browser = await chromium.launch() +const context = await browser.newContext({ + viewport: { width: 1500, height: 950 }, + // The action row is 12px type; 1x renders the button label soft enough on + // GitHub that a reviewer cannot read it. + deviceScaleFactor: 2, +}) + +/** + * Boot the chat page with `loop` seeded as this slot's loop. + * + * `fireReply` is the answer the fire route gives. Null means the route is not + * stubbed at all, which is the right default: a frame that does not press the + * button must not depend on a response that would never be requested. + */ +async function load(loop, fireReply = null) { + const page = await context.newPage() + logPageProblems(page) + + const extra = async (path, route) => { + if (fireReply && path === `/api/autonudge/${LOOP_ID}/fire`) { + await json(route, fireReply.body, fireReply.status) + return true + } + if (path === `/api/autonudge/slot/${SLOT}`) { await json(route, { loop }); return true } + if (path === '/api/autonudge') { await json(route, { enabled: true, loops: [loop] }); return true } + if (path.startsWith('/api/chat/slots/')) { await json(route, detail); return true } + return false + } + + await stubDashboardApi(page, { + slots, + extra, + // Pin the locale: without it the SPA negotiates one from the environment and + // the frame comes out in whatever language the runner happens to pick. + localStorageEntries: { 'mc-active-slot': SLOT, 'mc-lang': 'en' }, + }) + await page.goto(base + '/', { waitUntil: 'domcontentloaded' }) + await page.waitForTimeout(2500) + return page +} + +/** Open the goal popover from the composer chip and return it. */ +async function openPopover(page) { + // Addressed by accessible name, which differs by armed state: an active loop + // reads "Goal active (cycle N/M)", an inactive one falls back to "Set a goal". + const chip = page + .getByRole('button', { name: /^(Goal active \(cycle |Set a goal$)/ }) + .first() + await chip.waitFor({ state: 'visible', timeout: 15000 }) + await chip.click() + const popover = page.getByRole('dialog').filter({ hasText: 'Set a goal' }).first() + await popover.waitFor({ state: 'visible', timeout: 10000 }) + // Radix plays a zoom/fade entry animation; shoot after it settles. + await page.waitForTimeout(700) + return popover +} + +const results = [] +const check = (name, ok, detailText) => { + results.push({ name, ok, detail: detailText }) + if (!ok) console.error(`FAIL ${name}: ${detailText}`) +} + +async function shoot(popover, name) { + const out = join(OUT, name) + await popover.screenshot({ path: out }) + console.log('wrote', out) +} + +// 1 -- armed loop: the button is offered, beside Stop loop and Save. +{ + const page = await load(makeLoop()) + const popover = await openPopover(page) + const trigger = popover.getByRole('button', { name: TRIGGER }) + const count = await trigger.count() + await shoot(popover, '01-armed-loop-trigger-offered.png') + check('01 button present', count === 1, `found ${count} "${TRIGGER}" button(s), want exactly 1`) + // Its neighbours, so the frame is proven to show the whole popover foot rather + // than a button floating on its own. + check('01 row shows Stop loop', (await popover.getByRole('button', { name: STOP }).count()) === 1, + `"${STOP}" not found in the same frame`) + check('01 row shows Save', (await popover.getByRole('button', { name: SAVE }).count()) === 1, + `"${SAVE}" not found in the same frame`) + // The blocking rule is about SIBLINGS IN ONE horizontal group, so assert the + // action row itself rather than the popover's total button count. + const rowLabels = await popover.evaluate(el => { + const save = Array.from(el.querySelectorAll('button')).find(b => b.textContent === 'Save') + return Array.from(save.parentElement.querySelectorAll('button')).map(b => b.textContent) + }) + check('01 action row holds two', rowLabels.length === 2, + `action row holds ${rowLabels.length} buttons (${rowLabels.join(', ')}), cap is 2`) + await page.close() +} + +// 2 -- paused loop: the control frame. The button must be absent, and the +// schedule line must SAY why rather than leaving an unexplained gap. +{ + const page = await load(makeLoop({ active: false, stopped_reason: 'manual' })) + const popover = await openPopover(page) + const count = await popover.getByRole('button', { name: TRIGGER }).count() + await shoot(popover, '02-paused-loop-trigger-absent.png') + check('02 button absent', count === 0, `found ${count} "${TRIGGER}" button(s) on a paused loop, want 0`) + // Complement: absent anywhere in the popover, not merely under that exact + // accessible name -- a stale render could leave it somewhere else. + check('02 absent anywhere', !(await popover.innerText()).includes(TRIGGER), + `popover text still contains "${TRIGGER}"`) + // And the popover really did render the loop, so the negative is about the + // gating rather than about an empty frame. + check('02 popover shows the loop', (await popover.getByRole('button', { name: STOP }).count()) === 1, + `"${STOP}" missing -- the frame did not render a loop at all, so the negative proves nothing`) + // The absence must be EXPLAINED. Without this the frame is a mystery: a + // reader cannot tell a paused loop from a broken render. + check('02 the paused state is named', + (await popover.getByTestId('auto-nudge-loop-paused').count()) === 1, + 'nothing on the schedule line says the loop is paused, so the missing button has no reason') + await page.close() +} + +// 3 -- refusal: a press during a live turn is shown, not swallowed. +{ + const REFUSAL = 'nudge not sent: the agent is still working, so try again when it finishes' + const page = await load(makeLoop(), { + status: 409, + body: { error: REFUSAL, code: 'session_busy' }, + }) + const popover = await openPopover(page) + // Type into the goal box first: a press must not be a silent way to lose an + // unsaved edit, so the frame has to show the edit still there afterwards. + await popover.getByLabel('Goal description').fill('edited but not saved') + await popover.getByRole('button', { name: TRIGGER }).click() + await page.waitForTimeout(900) + await shoot(popover, '03-refused-while-a-turn-is-in-flight.png') + const text = await popover.innerText() + check('03 refusal is visible', text.includes(REFUSAL), + `popover does not show the refusal; it reads ${JSON.stringify(text.slice(0, 200))}`) + check('03 popover stayed open', await popover.isVisible(), + 'popover closed on a refusal -- it holds unsaved fields and must not be torn down') + check('03 the unsaved edit survived', + (await popover.getByLabel('Goal description').inputValue()) === 'edited but not saved', + 'the typed goal was lost -- a refusal must not discard the edit') + await page.close() +} + +// 4 -- the SUCCESS state, which the PR claims and nothing photographed. A press +// that works flips the schedule line to "due" in place, and the popover stays +// open so the change is visible where the press happened. Without this frame the +// feedback claim rests on prose; with it, a reviewer can see what a working press +// looks like. `next_due_ts` is a FIXED past timestamp, not `Date.now()`, so the +// line renders the settled "due" wording rather than a per-second countdown that +// would make every run produce different bytes. +{ + const DUE = 'Next cycle due' + // The route returns the loop UNCHANGED -- it no longer moves the deadline -- so + // this fixture must not hand the frame a moved one. If the line still reads + // "due" below, the CLIENT produced that, which is the behaviour under test. + const page = await load(makeLoop(), { status: 200, body: { ok: true, loop: makeLoop() } }) + const popover = await openPopover(page) + // Same unsaved edit as frame 3: on SUCCESS the edit must survive too, which is + // the whole reason this press does not close the popover. + await popover.getByLabel('Goal description').fill('edited but not saved') + const before = await popover.innerText() + check('04 started from a non-due line', !before.includes(DUE), + `the line already read "${DUE}" before the press, so the change proves nothing`) + await popover.getByRole('button', { name: TRIGGER }).click() + await page.waitForTimeout(900) + await shoot(popover, '04-pressed-schedule-line-reads-due.png') + const after = await popover.innerText() + check('04 the line now reads due', after.includes(DUE), + `after a successful press the line reads ${JSON.stringify(after.slice(0, 200))}`) + check('04 popover stayed open', await popover.isVisible(), + 'the popover closed on success -- the feedback would be invisible and the edit lost') + check('04 the unsaved edit survived', + (await popover.getByLabel('Goal description').inputValue()) === 'edited but not saved', + 'a successful press discarded the typed goal') + check('04 no error is shown', !after.includes('nudge not sent'), + 'a successful press rendered a refusal notice') + // The press must ACKNOWLEDGE itself. Before this the button re-enabled unchanged + // and a reader could not tell whether pressing again would double the nudge. + check('04 the trigger is now disabled', + await popover.getByRole('button', { name: TRIGGER }).isDisabled(), + 'the trigger is still pressable after a successful press, so nothing says the cycle is already armed') + await page.close() +} + +// 5 -- the 320px floor. MEASURED, not asserted from class names: the rule is +// about what renders, and a class-string check would pass on a layout that still +// overflows. Reads real bounding boxes and fails if the popover or the new action +// extends past the viewport's right edge. +{ + const NARROW = 320 + const page = await context.newPage() + logPageProblems(page) + await page.setViewportSize({ width: NARROW, height: 900 }) + const extra = async (path, route) => { + if (path === `/api/autonudge/slot/${SLOT}`) { await json(route, { loop: makeLoop() }); return true } + if (path === '/api/autonudge') { await json(route, { enabled: true, loops: [makeLoop()] }); return true } + if (path.startsWith('/api/chat/slots/')) { await json(route, detail); return true } + return false + } + await stubDashboardApi(page, { slots, extra }) + await page.goto(base) + const popover = await openPopover(page) + await shoot(popover, '05-narrow-320px-nothing-clipped.png') + + const box = await popover.boundingBox() + check('05 popover fits the viewport', + box !== null && box.x >= 0 && box.x + box.width <= NARROW + 1, + `popover spans x=${box && box.x}..${box && (box.x + box.width)} in a ${NARROW}px viewport`) + + const btn = popover.getByRole('button', { name: TRIGGER }) + check('05 the trigger is present at 320px', (await btn.count()) === 1, + 'the trigger vanished at 320px -- wrapping must move it, not remove it') + const bb = await btn.boundingBox() + check('05 the trigger fits the viewport', + bb !== null && bb.x >= 0 && bb.x + bb.width <= NARROW + 1, + `trigger spans x=${bb && bb.x}..${bb && (bb.x + bb.width)} in a ${NARROW}px viewport`) + // Clipping is not only horizontal overflow: a zero-width or zero-height box is + // an element the user cannot press either. + check('05 the trigger is actually pressable', + bb !== null && bb.width > 8 && bb.height > 8, + `trigger box is ${bb && bb.width}x${bb && bb.height}`) + await page.close() +} + +await browser.close() +srv.close() + +console.log('--- assertions (each frame must render what the PR claims) ---') +for (const r of results) console.log(JSON.stringify(r)) + +if (!results.every(r => r.ok)) { + console.error('FAIL: a frame did not render the trigger affordance -- fix the fixture, do not commit the PNG') + process.exit(1) +} +console.log('OK') diff --git a/website/src/components/AutoNudgePopover.tsx b/website/src/components/AutoNudgePopover.tsx index f4a5e2f4d21..3f5a5eba698 100644 --- a/website/src/components/AutoNudgePopover.tsx +++ b/website/src/components/AutoNudgePopover.tsx @@ -1,5 +1,5 @@ import { useEffect, useMemo, useRef, useState } from 'react' -import { useQuery } from '@tanstack/react-query' +import { useQuery, useQueryClient } from '@tanstack/react-query' import { Goal, X } from 'lucide-react' import { Popover, PopoverTrigger, PopoverContent } from './ui/popover' import ErrorNotice from './ErrorNotice' @@ -10,7 +10,7 @@ import { DRAFT_SAVE_DEBOUNCE_MS } from '../utils/draftConstants' import { i18nT } from '../i18n/t' import { fmtTimeNumeric } from '../i18n/format' -import { type AutoNudgeLoop, cycleText as loopCycleText, nextCycleText } from './autoNudgeLoop' +import { type AutoNudgeLoop, cycleText as loopCycleText, nextCycleText, AUTONUDGE_LOOPS_QUERY_KEY } from './autoNudgeLoop' export type { AutoNudgeLoop } from './autoNudgeLoop' interface Props { @@ -60,6 +60,7 @@ export default function AutoNudgePopover({ slotKey, loop, open, onOpenChange, on // lingering until it is reopened -- and the request dedupes with the other // consumer of the same key. `enabled: open` keeps a zero-token watch from // costing a request on every chat render just to say "still nothing". + const queryClient = useQueryClient() const { data: cronJobs, isError: watchesFailed, refetch: refetchWatches } = useQuery({ queryKey: ['cron-jobs'], queryFn: () => api.crons().then(r => r.jobs || []), @@ -208,6 +209,58 @@ export default function AutoNudgePopover({ slotKey, loop, open, onOpenChange, on } } + /** Run the loop's next cycle now instead of waiting out the remaining gap. + * + * Sends NO body: the nudge fired is whatever the loop currently holds, read + * server-side, so the button stays correct after a `monitor_update` revises + * the instruction and a stale popover field can never be delivered as the + * prompt. The consequence is that a user who edited the message and pressed + * this gets the ARMED message, not the edited one. + * + * WHICH IS WHY THIS DOES NOT CLOSE THE POPOVER, unlike `save` and `stop`. + * Closing would drop that unsaved edit with no dirty guard (drafts are not + * persisted while a loop exists), so a press after an edit would cost the + * user their text as well as spending a turn on the old prompt. Leaving the + * popover open keeps the edit, keeps Save reachable, and makes the outcome + * visible in place: the schedule line beside the button flips to "due", and + * the header's cycle readout advances a moment later when the delivered fire + * broadcasts (`autonudge_state`), which is also where the press's cost + * against the cycle cap becomes observable. + * + * Refusals (409 for a mid-fire loop or a session with a turn in flight, 404 + * for a loop the server no longer holds) land in the same inline + * `ErrorNotice` as `save` and `stop`. */ + async function triggerNow() { + if (!loop) return + setSaving(true) + setError('') + try { + const resp = await fetch(`/api/autonudge/${loop.id}/fire`, { method: 'POST' }) + const data = await resp.json().catch(() => ({})) + if (!resp.ok) throw new Error(data.error || `HTTP ${resp.status}`) + // The route returns the loop UNCHANGED: the server-side deadline write was + // removed because it could not be made durable without a suspension point + // that raced several lock-free writers. Rendering the response verbatim + // would therefore leave the countdown showing the very cycle this press + // superseded -- the one visible confirmation a press has. So the armed + // deadline is set here instead. Not a fiction: the cycle IS armed to run + // now, and the delivery's `autonudge_state` frame reconciles the shared + // cache moments later. + onChange({ ...data.loop, next_due_ts: Date.now() / 1000 }) + // Keep the SHARED registry consistent with the local view. `onChange` only + // updates this popover, so a reader of the full registry -- the Crew Members + // patrol block -- would otherwise keep its cached copy until the delivery's + // `autonudge_state` frame arrives. Nothing about the deadline changes here + // any more, so this is about the two views never disagreeing rather than + // about a stale countdown. + void queryClient.invalidateQueries({ queryKey: AUTONUDGE_LOOPS_QUERY_KEY }) + } catch (e: unknown) { + setError(e instanceof Error ? e.message : String(e)) + } finally { + setSaving(false) + } + } + // ── Countdown to the next trigger (#6482) ── // The 1s ticker runs only while the popover is OPEN (review finding: a // closed-but-armed loop must not re-render the toolbar button every second @@ -235,6 +288,11 @@ export default function AutoNudgePopover({ slotKey, loop, open, onOpenChange, on * the existing strings, so no catalogue text changes. Unlike the countdown * this is safe in aria-label: it changes once per cycle, not once per * second. */ + /** Whether a cycle is ALREADY armed to run. Derived from the same countdown + * the schedule line renders, so the button and the text can never disagree. */ + const cycleAlreadyDue = + countdownText === i18nT('components.autoNudgePopover.next_cycle_due') + const cycleText = loopCycleText(loop) return ( @@ -259,7 +317,15 @@ export default function AutoNudgePopover({ slotKey, loop, open, onOpenChange, on {loop?.active && loop.cycle_count > 0 ? cycleText : null} - +
@@ -351,10 +417,73 @@ export default function AutoNudgePopover({ slotKey, loop, open, onOpenChange, on
+ {/* The trigger sits on the SCHEDULE line, not in the action row below. + Two reasons, and they point the same way. `max-two-buttons-per-row` + (website/AUTOSDE.yaml:230, blocking) holds a row to two controls and + names this exact escape -- "the third action ... goes into an + overflow DropdownMenu, or LEAVES THE ROW" -- and leaving is cheaper + than a menu for one action. And it belongs here on the merits: this + button changes the countdown printed beside it, so the control and + the state it acts on read as one thing, while Stop/Save act on the + loop's configuration. + A one-button group, so the cap is satisfied structurally rather than + by being under it today. Button classes are the popover's existing + small-button spelling (the watches Retry above). + Gated on `active`, not merely on `loop`: a paused record still opens + this popover, and every terminal bound leaves the loop inactive, so + the server refuses to fire one -- a button there could only ever + produce a 409. */} {loop && ( -
- {i18nT('components.autoNudgePopover.last_fire')} {loop.last_fire_ts ? fmtTimeNumeric(loop.last_fire_ts) : i18nT('components.autoNudgePopover.never')} - {countdownText && · {countdownText}} + /* `flex-wrap` is for STRING LENGTH, not for 320px: the width cap on the + shell is what keeps this row inside the viewport, and measurement says + so -- pinning the shell back to 420px reddens the narrow frame while + removing this wrap does not. It is kept because `shrink-0` protects the + button, so a longer localized countdown ("Next cycle due, fires after + the current turn" is materially longer in several of the twelve + catalogues) has only this row to give. Defensive, and labelled as such + rather than claimed as the fix. */ + /* STACKED in every state, not a wrapping row. When the countdown flips to + the longer "due" wording, a wrapping row moved the button from beside the + text onto its own line -- relocating a control directly under the cursor + that just pressed it. One layout at every width also means the narrow + frame and the desktop frame agree, instead of the 320px case being a + second shape to keep in sync. */ +
+
+ {i18nT('components.autoNudgePopover.last_fire')} {loop.last_fire_ts ? fmtTimeNumeric(loop.last_fire_ts) : i18nT('components.autoNudgePopover.never')} + {countdownText && · {countdownText}} +
+ {loop.active ? ( + + ) : ( + /* Says WHY the button is not here, rather than leaving a gap. A + blind reader of the paused screenshot could not tell it was the + same loop at all, and an inactive loop otherwise looks identical + to an active one whose button failed to render -- the state is + the reason for the absence, so it belongs in the space the + absence leaves. Text, not a disabled button: the server refuses + to fire an inactive loop, so there is no press to offer. */ + + {i18nT('components.autoNudgePopover.loop_paused')} + + )}
)} @@ -382,7 +511,15 @@ export default function AutoNudgePopover({ slotKey, loop, open, onOpenChange, on disabled={saving || !message.trim()} className="px-3 py-1 rounded bg-accent text-accent-fg border-none cursor-pointer disabled:opacity-50 hover:bg-accent/90" > - {loop ? i18nT('components.autoNudgePopover.save') : i18nT('components.autoNudgePopover.start_loop')} + {/* A paused loop's way out was invisible: this button silently PATCHes + `active: true`, so on an inactive loop it must SAY so. A usability + reader found no resume control at all and called both "Paused" and + "Stop loop" risky as a result. Gated on `active`, not on existence, + which is the bug -- and it reuses the `start_loop` key the no-loop + case already uses, so no catalogue gains a string. */} + {loop?.active + ? i18nT('components.autoNudgePopover.save') + : i18nT('components.autoNudgePopover.start_loop')}
diff --git a/website/src/i18n/locales/bn.json b/website/src/i18n/locales/bn.json index f29d50ee882..ee5b8e12733 100644 --- a/website/src/i18n/locales/bn.json +++ b/website/src/i18n/locales/bn.json @@ -5473,6 +5473,7 @@ "goal_description": "লক্ষ্যের বিবরণ", "goal_interrupted_cycle": "লক্ষ্য লুপ প্রস্তুত (সাইকেল {{cycle}}) — শেষ টার্ন বিঘ্নিত হয়েছে; চ্যাট আবার চালু করো বা পরের সাইকেলের জন্য অপেক্ষা করো", "last_fire": "সর্বশেষ চালনা:", + "loop_paused": "পজ করা", "max_cycles_0": "সর্বোচ্চ চক্র (0 = ∞)", "max_cycles_0_infinite": "সর্বোচ্চ চক্র (0 = অসীম)", "never": "কখনো নয়", @@ -5485,6 +5486,7 @@ "set_a_goal": "একটি লক্ষ্য নির্ধারণ করুন", "start_loop": "লুপ শুরু করো", "stop_loop": "লুপ বন্ধ করুন", + "trigger_nudge": "নাজ ট্রিগার করুন", "watches_load_failed": "এই সেশনের ওয়াচগুলো লোড করা যায়নি।", "watches_next": "পরবর্তী", "watches_note": "প্রকৃত সংকেত পেলেই এজেন্টকে জাগায়। সময়সূচি পৃষ্ঠায় পরিচালনা করুন।", diff --git a/website/src/i18n/locales/de.json b/website/src/i18n/locales/de.json index aa7470169d6..f757e3ad171 100644 --- a/website/src/i18n/locales/de.json +++ b/website/src/i18n/locales/de.json @@ -5473,6 +5473,7 @@ "goal_description": "Zielbeschreibung", "goal_interrupted_cycle": "Zielschleife bereit (Zyklus {{cycle}}) – letzter Zug wurde unterbrochen; Chat fortsetzen oder auf den nächsten Zyklus warten", "last_fire": "Letzte Auslösung:", + "loop_paused": "Pausiert", "max_cycles_0": "Max. Zyklen (0 = ∞)", "max_cycles_0_infinite": "Max. Zyklen (0 = unbegrenzt)", "never": "nie", @@ -5485,6 +5486,7 @@ "set_a_goal": "Ein Ziel festlegen", "start_loop": "Schleife starten", "stop_loop": "Schleife stoppen", + "trigger_nudge": "Anstoß auslösen", "watches_load_failed": "Die Watches dieser Sitzung konnten nicht geladen werden.", "watches_next": "nächste", "watches_note": "Sie wecken den Agenten nur bei einem echten Signal. Verwaltung auf der Seite „Zeitplan“.", diff --git a/website/src/i18n/locales/en-XA.json b/website/src/i18n/locales/en-XA.json index ba10bbe9c08..11b4973adf1 100644 --- a/website/src/i18n/locales/en-XA.json +++ b/website/src/i18n/locales/en-XA.json @@ -5280,7 +5280,9 @@ "retry": "[Ŕèţŕý ········]", "save": "[Şàṽè ······]", "start_loop": "[Şţàŕţ ĺøøþ ···············]", - "watches_load_failed": "[Çøùĺðñ'ţ ĺøàð ţĥìş şèşşìøñ'ş ẁàţçĥèş. ···················]" + "trigger_nudge": "[Ţŕìğğèŕ ñùðğè ············]", + "watches_load_failed": "[Çøùĺðñ'ţ ĺøàð ţĥìş şèşşìøñ'ş ẁàţçĥèş. ···················]", + "loop_paused": "[Þàùşèð ·········]" }, "bottomTerminalPanel": { "terminal_is_in_its_own_window": "[Ţèŕɱìñàĺ ìş ìñ ìţş øẁñ ẁìñðøẁ ····················]", diff --git a/website/src/i18n/locales/en.manual.json b/website/src/i18n/locales/en.manual.json index 0bc065feffc..213c66fe285 100644 --- a/website/src/i18n/locales/en.manual.json +++ b/website/src/i18n/locales/en.manual.json @@ -1517,7 +1517,9 @@ "retry": "Retry", "save": "Save", "start_loop": "Start loop", - "watches_load_failed": "Couldn't load this session's watches." + "trigger_nudge": "Trigger nudge", + "watches_load_failed": "Couldn't load this session's watches.", + "loop_paused": "Paused" }, "bottomTerminalPanel": { "close_failed": "Couldn't stop this terminal's shell on the server. It will be cleaned up automatically.", diff --git a/website/src/i18n/locales/es.json b/website/src/i18n/locales/es.json index cb9da263102..098f2019305 100644 --- a/website/src/i18n/locales/es.json +++ b/website/src/i18n/locales/es.json @@ -5563,6 +5563,7 @@ "goal_description": "Descripción del objetivo", "goal_interrupted_cycle": "Bucle de objetivo armado (ciclo {{cycle}}) — el último turno se interrumpió; reanuda el chat o espera al siguiente ciclo", "last_fire": "Última ejecución:", + "loop_paused": "En pausa", "max_cycles_0": "Ciclos máximos (0 = ∞)", "max_cycles_0_infinite": "Ciclos máximos (0 = infinito)", "never": "nunca", @@ -5575,6 +5576,7 @@ "set_a_goal": "Establecer un objetivo", "start_loop": "Iniciar el bucle", "stop_loop": "Detener el bucle", + "trigger_nudge": "Activar el empujón", "watches_load_failed": "No se pudieron cargar las vigilancias de esta sesión.", "watches_next": "siguiente", "watches_note": "Solo despiertan al agente ante una señal real. Gestiónalas en la página Programación.", diff --git a/website/src/i18n/locales/fr.json b/website/src/i18n/locales/fr.json index 64962a50819..a424caa9403 100644 --- a/website/src/i18n/locales/fr.json +++ b/website/src/i18n/locales/fr.json @@ -5563,6 +5563,7 @@ "goal_description": "Description de l’objectif", "goal_interrupted_cycle": "Boucle d’objectif armée (cycle {{cycle}}) — le dernier tour a été interrompu ; reprends le chat ou attends le prochain cycle", "last_fire": "Dernier déclenchement :", + "loop_paused": "En pause", "max_cycles_0": "Cycles max (0 = ∞)", "max_cycles_0_infinite": "Cycles max (0 = infini)", "never": "jamais", @@ -5575,6 +5576,7 @@ "set_a_goal": "Définir un objectif", "start_loop": "Démarrer la boucle", "stop_loop": "Arrêter la boucle", + "trigger_nudge": "Déclencher la relance", "watches_load_failed": "Impossible de charger les veilles de cette session.", "watches_next": "prochaine", "watches_note": "Elles ne réveillent l'agent qu'en cas de signal réel. Gérez-les depuis la page Planification.", diff --git a/website/src/i18n/locales/hi.json b/website/src/i18n/locales/hi.json index 48a1b0c289b..71ece04490b 100644 --- a/website/src/i18n/locales/hi.json +++ b/website/src/i18n/locales/hi.json @@ -5473,6 +5473,7 @@ "goal_description": "लक्ष्य विवरण", "goal_interrupted_cycle": "लक्ष्य लूप तैयार (चक्र {{cycle}}) — पिछला टर्न बाधित हुआ; चैट फिर से शुरू करो या अगले चक्र का इंतज़ार करो", "last_fire": "अंतिम बार चला:", + "loop_paused": "रोका गया", "max_cycles_0": "अधिकतम चक्र (0 = ∞)", "max_cycles_0_infinite": "अधिकतम चक्र (0 = अनंत)", "never": "कभी नहीं", @@ -5485,6 +5486,7 @@ "set_a_goal": "लक्ष्य सेट करें", "start_loop": "लूप शुरू करें", "stop_loop": "लूप रोकें", + "trigger_nudge": "नज ट्रिगर करें", "watches_load_failed": "इस सत्र के वॉच लोड नहीं किए जा सके।", "watches_next": "अगला", "watches_note": "ये वास्तविक संकेत मिलने पर ही एजेंट को जगाते हैं। शेड्यूल पेज पर प्रबंधित करें।", diff --git a/website/src/i18n/locales/it.json b/website/src/i18n/locales/it.json index 22fa5faa7d9..98d36f9a54b 100644 --- a/website/src/i18n/locales/it.json +++ b/website/src/i18n/locales/it.json @@ -5563,6 +5563,7 @@ "goal_description": "Descrizione dell'obiettivo", "goal_interrupted_cycle": "Loop obiettivo armato (ciclo {{cycle}}) — l'ultimo turno è stato interrotto; riprendi la chat o attendi il prossimo ciclo", "last_fire": "Ultima attivazione:", + "loop_paused": "In pausa", "max_cycles_0": "Numero massimo di cicli (0 = ∞)", "max_cycles_0_infinite": "Numero massimo di cicli (0 = infiniti)", "never": "mai", @@ -5575,6 +5576,7 @@ "set_a_goal": "Imposta un obiettivo", "start_loop": "Avvia il ciclo", "stop_loop": "Arresta il ciclo", + "trigger_nudge": "Attiva il sollecito", "watches_load_failed": "Impossibile caricare le watch di questa sessione.", "watches_next": "prossima", "watches_note": "Svegliano l'agente solo con un segnale reale. Gestiscili nella pagina Pianificazione.", diff --git a/website/src/i18n/locales/ja.json b/website/src/i18n/locales/ja.json index 60f7aaf7185..24ce8705d9b 100644 --- a/website/src/i18n/locales/ja.json +++ b/website/src/i18n/locales/ja.json @@ -5383,6 +5383,7 @@ "goal_description": "目標説明", "goal_interrupted_cycle": "ゴールループ待機中（サイクル {{cycle}}）— 前のターンが中断されました。チャットを再開するか、次のサイクルをお待ちください", "last_fire": "最後の実行:", + "loop_paused": "一時停止中", "max_cycles_0": "最大周期 (0 = ∞)", "max_cycles_0_infinite": "最大周期 (0 = 無制限)", "never": "なし", @@ -5395,6 +5396,7 @@ "set_a_goal": "目標を設定", "start_loop": "ループ開始", "stop_loop": "ループ停止", + "trigger_nudge": "ナッジ実行", "watches_load_failed": "このセッションのウォッチを読み込めませんでした。", "watches_next": "次回", "watches_note": "実際のシグナルがあったときだけエージェントを起こします。管理は「スケジュール」ページから。", diff --git a/website/src/i18n/locales/ko.json b/website/src/i18n/locales/ko.json index 74fcc1bf970..6b2d19a47ff 100644 --- a/website/src/i18n/locales/ko.json +++ b/website/src/i18n/locales/ko.json @@ -5383,6 +5383,7 @@ "goal_description": "목표 설명", "goal_interrupted_cycle": "목표 루프 대기 중 (주기 {{cycle}}) — 이전 턴이 중단되었습니다. 채팅을 재개하거나 다음 주기를 기다리세요", "last_fire": "마지막 실행:", + "loop_paused": "일시중지됨", "max_cycles_0": "최대 주기 (0 = ∞)", "max_cycles_0_infinite": "최대 주기 (0 = 무제한)", "never": "없음", @@ -5395,6 +5396,7 @@ "set_a_goal": "목표 설정", "start_loop": "루프 시작", "stop_loop": "루프 중지", + "trigger_nudge": "넛지 실행", "watches_load_failed": "이 세션의 워치를 불러올 수 없습니다.", "watches_next": "다음", "watches_note": "실제 신호가 있을 때만 에이전트를 깨웁니다. 스케줄 페이지에서 관리하세요.", diff --git a/website/src/i18n/locales/pt.json b/website/src/i18n/locales/pt.json index fad079aa92c..5456931f4df 100644 --- a/website/src/i18n/locales/pt.json +++ b/website/src/i18n/locales/pt.json @@ -5563,6 +5563,7 @@ "goal_description": "Descrição do objetivo", "goal_interrupted_cycle": "Loop de objetivo armado (ciclo {{cycle}}) — o último turno foi interrompido; retome o chat ou aguarde o próximo ciclo", "last_fire": "Última execução:", + "loop_paused": "Pausado", "max_cycles_0": "Máx. de ciclos (0 = ∞)", "max_cycles_0_infinite": "Máx. de ciclos (0 = infinito)", "never": "nunca", @@ -5575,6 +5576,7 @@ "set_a_goal": "Definir um objetivo", "start_loop": "Iniciar loop", "stop_loop": "Parar o loop", + "trigger_nudge": "Acionar o empurrão", "watches_load_failed": "Não foi possível carregar as vigilâncias desta sessão.", "watches_next": "próxima", "watches_note": "Só acordam o agente diante de um sinal real. Gerencie-os na página Agendamento.", diff --git a/website/src/i18n/locales/ru.json b/website/src/i18n/locales/ru.json index 6043f7d14a9..815ea56dfe5 100644 --- a/website/src/i18n/locales/ru.json +++ b/website/src/i18n/locales/ru.json @@ -5653,6 +5653,7 @@ "goal_description": "Описание цели", "goal_interrupted_cycle": "Цикл цели наготове (цикл {{cycle}}) — последний ход был прерван; возобнови чат или дождись следующего цикла", "last_fire": "Последний запуск:", + "loop_paused": "Приостановлено", "max_cycles_0": "Макс. циклов (0 = ∞)", "max_cycles_0_infinite": "Макс. циклов (0 = бесконечно)", "never": "никогда", @@ -5665,6 +5666,7 @@ "set_a_goal": "Задать цель", "start_loop": "Запустить цикл", "stop_loop": "Остановить цикл", + "trigger_nudge": "Запустить подталкивание", "watches_load_failed": "Не удалось загрузить наблюдения этой сессии.", "watches_next": "далее", "watches_note": "Будят агента только при реальном сигнале. Управление на странице «Расписание».", diff --git a/website/src/i18n/locales/zh-CN.json b/website/src/i18n/locales/zh-CN.json index 1bff9900b71..6dc1eb1bd7b 100644 --- a/website/src/i18n/locales/zh-CN.json +++ b/website/src/i18n/locales/zh-CN.json @@ -5383,6 +5383,7 @@ "goal_description": "目标描述", "goal_interrupted_cycle": "目标循环待命（第 {{cycle}} 轮）— 上一轮被中断；恢复聊天或等待下一轮", "last_fire": "上次触发：", + "loop_paused": "已暂停", "max_cycles_0": "最大轮次（0 = ∞）", "max_cycles_0_infinite": "最大轮次（0 = 无限）", "never": "从不", @@ -5395,6 +5396,7 @@ "set_a_goal": "设定目标", "start_loop": "启动循环", "stop_loop": "停止循环", + "trigger_nudge": "触发推动", "watches_load_failed": "无法加载此会话的监视项。", "watches_next": "下次", "watches_note": "只在出现真实信号时唤醒智能体。可在“计划”页面管理。", diff --git a/website/src/test/AutoNudgePopover.test.tsx b/website/src/test/AutoNudgePopover.test.tsx index ec41bca47fd..aaf24e00570 100644 --- a/website/src/test/AutoNudgePopover.test.tsx +++ b/website/src/test/AutoNudgePopover.test.tsx @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' -import { render, screen, fireEvent, act } from '@testing-library/react' +import { useState } from 'react' +import { render, screen, fireEvent, act, cleanup } from '@testing-library/react' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import AutoNudgePopover, { type AutoNudgeLoop } from '../components/AutoNudgePopover' import { __resetForTests, loadGoalDraft, saveGoalDraft } from '../utils/goalDrafts' @@ -511,3 +512,198 @@ describe('AutoNudgePopover cycle cap readout', () => { expect(trigger.getAttribute('aria-label')).not.toMatch(/Next cycle/i) }) }) + +describe('AutoNudgePopover Trigger nudge (#8212)', () => { + beforeEach(() => { + localStorage.clear() + __resetForTests() + vi.stubGlobal('fetch', vi.fn(() => Promise.resolve({ ok: true, json: () => Promise.resolve({ loop: null }) })) as unknown as typeof fetch) + }) + afterEach(() => { vi.useRealTimers(); vi.unstubAllGlobals() }) + + /** Local render helper: the shared one hardcodes no-op callbacks, and these + * tests are about what the press DOES to them. + * + * `open` is CONTROLLED here, mirroring the real parent (ChatInput owns the + * flag and feeds it back). A fixed `open={true}` would make the harness pin + * the popover's presence, so "the edit survives" could not fail even if the + * code closed it -- the assertion would be about the fixture rather than the + * component. */ + const renderWith = (loop: AutoNudgeLoop | null, onChange = vi.fn()) => { + const onOpenChange = vi.fn() + const Harness = () => { + const [open, setOpen] = useState(true) + return ( + { onOpenChange(v); setOpen(v) }} + onChange={onChange} + /> + ) + } + const qc = new QueryClient({ defaultOptions: { queries: { retry: false, gcTime: 0 } } }) + render( + + + , + ) + return { onChange, onOpenChange } + } + + const triggerButton = () => screen.queryByRole('button', { name: 'Trigger nudge' }) + + it('offers the button while a loop is active', () => { + renderWith(makeLoop()) + expect(triggerButton()).toBeTruthy() + }) + + it('offers it NOWHERE when no loop is running, so the affordance never appears without a subject', () => { + renderWith(null) + // Complement assertion rather than a bare negative on one node: a stale + // render could leave the button somewhere else in the tree, and "the + // button I looked for is absent" would still pass. + expect(triggerButton()).toBeNull() + expect(screen.queryAllByRole('button', { name: /Trigger/i })).toHaveLength(0) + }) + + it('offers it NOWHERE for a paused loop, because the server refuses to fire one', () => { + // Gated on `active`, not on `loop`: every terminal bound leaves the loop + // inactive, so a button here could only ever produce a 409. + renderWith(makeLoop({ active: false })) + expect(triggerButton()).toBeNull() + expect(screen.queryAllByRole('button', { name: /Trigger/i })).toHaveLength(0) + }) + + it('disables itself once a cycle is due, so a press visibly acknowledges itself', () => { + // The press used to leave the button re-enabled and unchanged, so a reader + // could not tell whether pressing again would double the nudge. It would not: + // the cycle is already armed. Both directions asserted -- a loop that is NOT + // due must stay pressable, or this would disable the feature it guards. + renderWith(makeLoop({ next_due_ts: 1_700_000_000 })) + expect(triggerButton()).toBeTruthy() + expect((triggerButton() as HTMLButtonElement).disabled).toBe(true) + cleanup() + renderWith(makeLoop({ next_due_ts: Math.floor(Date.now() / 1000) + 300 })) + expect((triggerButton() as HTMLButtonElement).disabled).toBe(false) + }) + + it('names the way OUT of a paused loop instead of leaving Save to do it silently', () => { + // The primary button PATCHes `active: true`, so on a paused loop it is the + // resume control -- and it used to read "Save", which said nothing. A blind + // reader found no resume path at all and called "Stop loop" risky as a + // result. Both directions asserted: an active loop must still read Save, or + // this would just move the confusion. + renderWith(makeLoop({ active: false })) + expect(screen.getByRole('button', { name: 'Start loop' })).toBeTruthy() + expect(screen.queryByRole('button', { name: 'Save' })).toBeNull() + cleanup() + renderWith(makeLoop({ active: true })) + expect(screen.getByRole('button', { name: 'Save' })).toBeTruthy() + expect(screen.queryByRole('button', { name: 'Start loop' })).toBeNull() + }) + + it('says the loop is paused where the button would be, so the absence has a reason', () => { + // Absence alone is ambiguous: an inactive loop looked identical to an active + // one whose button failed to render, and a usability reader could not tell + // the paused screenshot was even the same loop. The state is the reason for + // the absence, so it occupies the space the absence leaves. + renderWith(makeLoop({ active: false })) + expect(screen.getByTestId('auto-nudge-loop-paused')).toBeTruthy() + // And it is genuinely conditional, not always-on decoration. + cleanup() + renderWith(makeLoop({ active: true })) + expect(screen.queryByTestId('auto-nudge-loop-paused')).toBeNull() + expect(triggerButton()).toBeTruthy() + }) + + it('posts to the loop-scoped fire route with NO body, so the ARMED message is what fires', async () => { + const fired = makeLoop({ next_due_ts: 1_700_000_000 }) + vi.stubGlobal('fetch', vi.fn((url: string) => + Promise.resolve({ + ok: true, + json: () => Promise.resolve(String(url).endsWith('/fire') ? { ok: true, loop: fired } : { loop: null }), + }), + ) as unknown as typeof fetch) + const { onChange, onOpenChange } = renderWith(makeLoop()) + + await act(async () => { fireEvent.click(triggerButton()!) }) + + // Selected by URL, not by index: opening the popover also reads /api/crons. + const calls = (fetch as unknown as { mock: { calls: [string, { method?: string, body?: string }?][] } }).mock.calls + const fire = calls.find(c => String(c[0]) === '/api/autonudge/l1/fire') + expect(fire, 'no POST to the fire route was issued').toBeTruthy() + expect(fire![1]?.method).toBe('POST') + // Load-bearing: a body would let a stale popover field become the prompt. + // The nudge fired must be whatever the loop currently holds, read server-side. + expect(fire![1]?.body).toBeUndefined() + // The server no longer moves the deadline, so the component supplies the + // armed one. Asserted field-wise rather than by identity: the loop's own + // data must be passed through untouched, and only `next_due_ts` replaced. + const passed = onChange.mock.calls.at(-1)?.[0] + expect(passed).toMatchObject({ ...fired, next_due_ts: expect.any(Number) }) + expect(passed.next_due_ts).toBeGreaterThan(Date.now() / 1000 - 5) + // And it must NOT close: closing would drop an unsaved edit in the textarea + // 40px above, with no dirty guard, so a press after an edit would cost the + // user their text on top of spending a turn on the old prompt. + expect(onOpenChange).not.toHaveBeenCalled() + }) + + it('keeps a typed-but-unsaved goal edit after a successful press', async () => { + // The complement of the assertion above, stated as the user-visible fact + // rather than as a callback that was not invoked: a press must never be a + // silent way to lose work. + vi.stubGlobal('fetch', vi.fn((url: string) => + Promise.resolve({ + ok: true, + json: () => Promise.resolve(String(url).endsWith('/fire') ? { ok: true, loop: makeLoop() } : { loop: null }), + }), + ) as unknown as typeof fetch) + renderWith(makeLoop()) + const box = screen.getByLabelText('Goal description') as HTMLTextAreaElement + fireEvent.change(box, { target: { value: 'edited but not saved' } }) + + await act(async () => { fireEvent.click(triggerButton()!) }) + + expect((screen.getByLabelText('Goal description') as HTMLTextAreaElement).value) + .toBe('edited but not saved') + }) + + it('surfaces a refusal inline and keeps the popover open, because it holds unsaved fields', async () => { + // The refusal names the outcome and the next step, not just the condition: + // a reader must be able to tell a refusal from a delay, and the press was + // refused rather than queued. + const REFUSAL = 'nudge not sent: the agent is still working, so try again when it finishes' + vi.stubGlobal('fetch', vi.fn((url: string) => + String(url).endsWith('/fire') + ? Promise.resolve({ ok: false, status: 409, json: () => Promise.resolve({ error: REFUSAL, code: 'session_busy' }) }) + : Promise.resolve({ ok: true, json: () => Promise.resolve({ loop: null }) }), + ) as unknown as typeof fetch) + const { onChange, onOpenChange } = renderWith(makeLoop()) + + await act(async () => { fireEvent.click(triggerButton()!) }) + + expect(screen.getByText(REFUSAL)).toBeTruthy() + // A refusal must not report success by tearing the popover down. + expect(onChange).not.toHaveBeenCalled() + expect(onOpenChange).not.toHaveBeenCalled() + }) + + it('sits on the schedule line, not in the Stop/Save action row (max-two-buttons-per-row)', async () => { + // `website/AUTOSDE.yaml:230` holds a row to two controls and names this + // escape itself: the third action "leaves the row". Asserted structurally + // rather than by counting the whole popover, because the rule is about + // SIBLINGS IN ONE horizontal group. + renderWith(makeLoop()) + const save = screen.getByRole('button', { name: 'Save' }) + const row = save.parentElement! + const rowButtons = Array.from(row.querySelectorAll('button')) + expect(rowButtons).toHaveLength(2) + expect(rowButtons.map(b => b.textContent)).toEqual(['Stop loop', 'Save']) + // And the trigger is a sibling of the schedule text instead. + const trigger = triggerButton()! + expect(trigger.parentElement).not.toBe(row) + expect(trigger.parentElement!.textContent).toMatch(/Last fire:/) + }) +})