Skip to content

feat: expose session monitors to agents - #5175

Closed
kyleseaman wants to merge 1 commit into
token-monitors-github-probefrom
codex/token-monitors-tools
Closed

feat: expose session monitors to agents#5175
kyleseaman wants to merge 1 commit into
token-monitors-github-probefrom
codex/token-monitors-tools

Conversation

@kyleseaman

@kyleseaman kyleseaman commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Stack: #5171#5172#5173#5174#5175#5176#5177

Position: 5 of 7. Base: #5174. Next: #5176.

Adds stateless session monitor tools, the live controller, authenticated directives and routes, typed cross-surface dispatch, and durable accounting.

Verification: controller, MCP, directive, auth, route, session, and channel suites.

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 3820dfd18059ce6b7fb6025ade6fbaebabd6f65b.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/dashboard/handlers/autonudge.py:184 -- Monitor routes omit app-slot ownership checks
svc.list_all() and caller-selected slot_key/monitor_id are used without checking request["app"].
App token with /api/monitors scope -> monitor routes -> foreign session monitors are exposed or mutated.
Anchor: backend-security-controls
Fix: Reject or filter app callers unless slot._app == request["app"].

BLOCKING -- src/kiro_crew/slack/gateway.py:200 -- Disabled monitoring is eagerly imported on boot
from kiro_crew.monitoring.controller import MonitorController
KIROCREW_AUTONUDGE=0 -> gateway import -> monitoring controller/provider load -> disabled-subsystem boot work delays every launch.
Anchor: no-new-work-on-gateway-boot-path
Fix: Gate structured-monitor imports behind the enabled check in _init_autonudge().

[BLOCK-MERGE] 3820dfd
[GPT-REVIEWED] 3820dfd
False positive or not applicable? A repository writer can comment:
/ai-review override gpt 3820dfd18059ce6b7fb6025ade6fbaebabd6f65b: <one-sentence reason>

@kyleseaman kyleseaman closed this Aug 23, 2026
@kyleseaman
kyleseaman deleted the codex/token-monitors-tools branch August 23, 2026 05:18
@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Aug 23, 2026
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Replaced by #5184 after renaming the branch to remove the prefix.

@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

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

Repository evidence gathered; counts run (/api/monitors consumers, MONITOR_WAKE_PREFIX, wake_count, website API usage, RFC PR4/PR5 scope). Emitting the review.

First-Principles-Verdict: CONCERNS

Six new /api/monitors browser routes ship with zero consumers — the RFC assigns their UI, including the restart control, to the next PR.

What this change ships

Intent: let an agent arm, inspect, edit, and stop a cheap server-probed PR monitor that wakes its own session only on actionable change — an ADDITION (RFC PR 4 of 7).

  1. monitor_watch arms a bounded structured monitor from one tool call — justified
  2. monitor_inspect reads only the strict-authenticated session's monitor — justified
  3. monitor_stop (and legacy autonudge_stop) retains a durable terminal outcome — justified
  4. monitor_update edits structured targets/budgets; identity edits refused mid-wake — justified
  5. Monitors now live-probe and inject a redacted, capped [Monitor wake] turn — justified
  6. Busy sessions get typed BUSY/DISPATCHED/UNAVAILABLE handoff, never steer/queue — justified; authoritative claim boundary is Discord-only
  7. Six /api/monitors routes incl. restart — zero consumers
  8. Legacy /api/autonudge PATCH/DELETE fail closed for structured ids — justified
  9. Session close retires the monitor terminally, with close-failure rollback — justified
  10. Durable wake_count plus monitor fields on the websocket payload — declared accounting

Watch

  • /api/monitors* real consumers: 0 (grepped api/monitors repo-wide: registration, handlers, docs, tests only; website/src has no hit). The RFC puts "create, inspect, stop, and explicit restart controls" in PR 5 (feat: show bounded monitors in the dashboard #5176). Only GET /api/autonudge/session-monitor has a consumer (monitor_inspect).
  • The advisory-busy-then-blocking-claim race fixed for Discord (wait_if_busy=False) has 2 unfixed siblings: _fire_slack_nudge and _fire_dashboard_nudge still decide BUSY from advisory checks, then block on the real claim. The spec declares the Discord boundary only; nothing marks the siblings deferred.

Subtractions

  • Defer the /api/monitors route family — most clearly POST /api/monitors/{id}/restart — to PR 5, where their only consumer lands (0 consumers today).
  • Drop GET /api/monitors and GET /api/monitors/slot/{slot_key}: this same diff makes GET /api/autonudge and GET /api/autonudge/slot/{slot_key} return the identical monitor_state_public_dict projection, so the reads are a second spelling that must be kept in sync.
  • Drop _serialize_monitor (dashboard/handlers/autonudge.py) — a bare alias of _serialize; call _serialize directly.

[FIRST-PRINCIPLES-REVIEWED] 3820dfd

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound probe-first design, but the new tool must now win an LLM tool-selection race against monitor_start, whose description still claims PR watching.

Watch

  • The agent-facing surface now offers two tools for the same user intent: monitor_start still says "Use when the user asks to babysit / monitor / keep checking something (a PR, CI run…)" while monitor_watch covers exactly the GitHub-PR case. An LLM that picks the legacy loop burns a full agent turn per cycle — the precise cost the token-efficient-monitors RFC exists to remove — and nothing goes red when it does. If steering text lands in a later PR of the stack, fine; otherwise update monitor_start's description in this PR to route PR targets to monitor_watch.
  • monitor_update is now one tool whose accepted field set depends on hidden record type (legacy vs structured), with mutual rejection errors as the only guide. Acceptable, but every future monitor field addition must be classified into one of the two halves — worth a comment or spec line naming that rule where the field lists live (session_directive_apply.py).

[DESIGN-REVIEWED] 3820dfd

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant