refactor: account for completed monitor turns

[kyleseaman]

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

Position: 3 of 7. Base: #5172. Next: #5174.

Charges monitor budgets from correlated raw completion evidence across dashboard, Slack, and Discord.

Verification: completion, dashboard, Slack, Discord, and messaging suites.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 1c2704bf2663dcf21b10cba4a83523d4524541c5 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 1c2704b

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

An in-flight claim has only fatal exits — a timed-out turn wedges the monitor silently, and the restart recovery then retires it as BLOCKED.

Watch

• No in-band exit for a claim whose turn never reports. mark_monitor_action_in_flight refuses all dispatch while wake_in_flight is set, and only completion (raw EVENT_COMPLETE), pre-start dispatch failure, or restart clears it. The spec itself names the gap — "timeout before that event … do[es] not report monitor completion" — and the Slack TimeoutError branch is an existing, real path (# Timeout spend is REAL spend (issue #874 follow-up)). Consequence: one nudge-turn timeout leaves the monitor active=True, wake_in_flight=True with no stop reason, permanently unable to fire, until a gateway restart converts it to BLOCKED/completion_evidence_unavailable — so a routine timeout or a restart racing a turn each kill a user's monitor rather than degrade it. Fail-closed is the right direction for a budget, but if the later stack slices (feat: add github pull request monitor probe #5174+) don't add a reconciler or a conservative charge-and-continue path for lost evidence, this becomes the dominant way monitors end. Confirm which slice owns that exit.

Suggestions

• The 8-turn universal ceiling is enforced by clamping inside monitor_budget_reason (min(budgets.max_agent_turns, DEFAULT_MONITOR_AGENT_TURNS)), so a persisted max_agent_turns larger than 8 is silently displayed-but-never-honored state; clamping/normalizing at MonitorBudgets construction keeps persisted state truthful.

[DESIGN-REVIEWED] 1c2704b

[kyleseaman]

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

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

All evidence gathered. The change is stage 3 of a declared 7-PR stack; virtually every runtime path it adds is dormant until the probe dispatcher (a later stack PR) starts marking wakes in-flight. Key counts I ran: mark_monitor_action_in_flight / record_monitor_dispatch_failure — 0 production callers (definitions only); last_completion_fingerprint / last_completion_disposition / last_completed_at / token_usage_known — written in autonudge.py, 0 readers anywhere in src/; _report_monitor_completion — 2 call sites, both pass hook= explicitly, so its hook=None re-resolve branch is dead; the new min(budgets.max_agent_turns, 8) clamp adds a second owner for a bound MonitorBudgets already validates.

First-Principles-Verdict: CONCERNS

A "refactor" that ships a dormant accounting substrate — 2 zero-caller service methods, 4 write-only persisted fields, and an undeclared universal 8-turn clamp.

What this change ships

Intent: make structured monitors spend their turn/token budgets only on correlated raw provider completion evidence — an ADDITION (dormant capability), despite the refactor: type.

1. Monitor charges one turn only on a raw provider completion — justified (declared; fail-closed correlation)
2. Restart with an in-flight wake retires the monitor instead of redispatching — justified
3. Turn budgets above 8 now silently clamp to 8 — undeclared in description; behavior change in decide_monitor
4. Budget stops gain named reasons with runtime→turn→token precedence — justified
5. Four new persisted monitor fields (last_completion_*, token_usage_known) — zero consumers (0 readers in src)
6. mark_monitor_action_in_flight / record_monitor_dispatch_failure — zero consumers; stack-deferred
7. Dashboard, Slack, Discord turn paths accept an optional completion hook, absent for legacy turns — justified seam
8. Slack nudge reads destructive provider usage once, fans out to usage row + hook — justified (destructive read is a named platform constraint)
9. Approval stall overrides a nominal success and terminally blocks the monitor — justified
10. _report_monitor_completion re-resolves the hook when none passed — one consumer pattern, dead branch (2 callers, both pass it)

Watch

• Nearly the whole diff is unreachable in production: wake_in_flight is only ever set by mark_monitor_action_in_flight, which has 0 callers (grep mark_monitor_action_in_flight, src). Acceptable only as stack position 3 of 7 with the dispatcher declared "lands separately" — if the stack stalls, this is pure dormant surface plus a persisted schema.
• The 8-turn clamp (decision.py:75) contradicts the framing: description says "charges monitor budgets from correlated raw completion evidence", but this hunk changes what any budget means. It also makes MonitorBudgets.max_agent_turns > 8 accepted-but-inert — validation permits what policy ignores.

Subtractions

• Delete the if hook is None: hook = self._monitor_completion_hook(loop) fallback in _report_monitor_completion (slack/gateway.py:5216) — both callers (gateway.py:4885, 4918) pass hook=; make the parameter required.
• Drop the min(budgets.max_agent_turns, DEFAULT_MONITOR_AGENT_TURNS) second bound in monitor_budget_reason (decision.py:75) — enforce the ceiling once, in MonitorBudgets.__post_init__, the field's existing owner; the agent-writable-store cause is covered equally there since every load path constructs MonitorBudgets.
• Defer the write-only fields last_completion_fingerprint, last_completion_disposition, last_completed_at, token_usage_known (models.py:184-187) to the stack PR that reads them — 0 readers today, and persisted schema is the costliest surface here to retract.

[FIRST-PRINCIPLES-REVIEWED] 1c2704b