refactor: account for completed monitor turns

[kyleseaman]

Stacked change: PR 3 of 8

Stack: #5180 → #5181 → #5182 → #5183 → #5184 → #5185 → #5186 → #5305
Base: #5181
Next: #5183

Problem / Motivation

Legacy delivery counts dispatch attempts, while structured monitors need to charge only work that actually reaches a provider completion boundary.

Why it matters

Charging probes, busy handoffs, or failed delivery overstates cost; inferring completion from helper returns can undercount failures or double-count retries.

What changed (motivation → approach → change)

• Add a shared completion adapter correlated to the persisted monitor claim and fingerprint.
• Account from raw provider EVENT_COMPLETE evidence before any buffered renderer flush or finalization, including authoritative token usage when available.
• Persist Slack raw completion before cancellable analytics usage-row writes so cancellation cannot strand a completed wake.
• Share the completion boundary across dashboard, Slack, Discord, and the messaging driver.
• Keep missing-token usage explicit and preserve all legacy loop accounting.
• Validate the configured completed-turn budget against the universal eight-turn ceiling before persistence or enforcement.

Tests

• Raw-completion correlation, duplicate suppression, and missing-token behavior
• Dashboard, Slack, Discord, and messaging completion paths
• Restart-safe in-flight state and approval/runtime/turn/token precedence

Manual verification

N/A — transport and completion behavior is covered with deterministic provider-event fixtures.

Related Issues

N/A — implements the accounting layer specified in #5180 and depends on #5181.

Checklist

• Single commit with a Conventional Commits title
• Existing tests pass and new tests cover the new behavior
• Self-review completed; code follows project style guidelines
• Documentation updated where applicable
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

N/A — the repository template does not yet supply final CLA wording.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 529e20235af94cc84af8ebfcb1ad882db74a3acb and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 529e202

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

The completion contract fail-closes on end_turn — the genuine success reason — so as specified, a successful wake can never be charged or release its claim.

Watch

• _SYNTHETIC_STOP_REASONS excludes end_turn ("that reason remains uncharged until ACP events carry provenance"), yet the real provider result path (client.py:4713) reports end_turn for normal success. So disposition_for_stop_reason's SUCCESS branch is unreachable, wake_in_flight is never cleared after a successful turn, mark_monitor_action_in_flight then refuses every later dispatch, and the next restart retires the monitor as completion_evidence_unavailable. Dormant today (nothing sets the claim), but if the dispatcher PR lands before the provenance PR, every structured monitor does exactly one successful action and bricks. Confirm the stack ordering, or add the synthetic-event provenance flag here — this PR already owns the stop-reason vocabulary.
• On the kiro seam neither turns (end_turn excluded) nor tokens (credits → both fields zero → None, None) accrue, leaving runtime as the only live budget — the stated goal, "charge work that reaches a provider completion boundary," is not met on the first-class harness until provenance lands.
• Restart with a persisted in-flight claim retires the whole monitor (BLOCKED) rather than charging one FAILURE turn and resuming; a routine gateway upgrade mid-wake silently kills a monitor the user configured. Charging conservatively bounds crash-loop spend at the same budget without the kill.

Suggestions

• MonitorBudgets raising when a persisted max_agent_turns exceeds the ceiling bricks the record on load; clamp with a logged reason instead, matching the repo's degrade-don't-raise pattern — and give the ceiling its own constant rather than overloading DEFAULT_MONITOR_AGENT_TURNS.

[DESIGN-REVIEWED] 529e202

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 529e20235af94cc84af8ebfcb1ad882db74a3acb — 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 and counts verified. Here is the review.

First-Principles-Verdict: CONCERNS

Every runtime capability here is dormant — nothing sets wake_in_flight, so the entire completion pipeline ships with zero live activations, awaiting PR #5183's dispatcher.

What this change ships

Intent: charge a structured monitor's budget only when a provider actually completed a turn — an ADDITION (labeled refactor).

1. Monitors charge one turn per correlated provider completion across all surfaces — justified, but dormant (zero activations)
2. Restart retires an in-flight wake as BLOCKED instead of redispatching — justified
3. Budget stops now name which bound tripped, runtime→turns→tokens — justified
4. Turn budgets above 8 rejected at construction/load — justified, declared
5. Approval stall becomes a terminal completed turn — justified
6. Ordinary end_turn success is never charged — declared gap; the headline's main case is deferred
7. Four new persisted monitor fields — zero readers
8. stream_and_collect gains public on_complete — one consumer (slack/gateway.py)
9. agent_sdk gains AgentTurnUsage + two stop constants — mandated by the recorded SDK-boundary RFC
10. New service API: claim / complete / dispatch-failure — claim and failure have zero callers

Watch

• mark_monitor_action_in_flight and record_monitor_dispatch_failure: 0 production callers (grepped src/; only the definitions). Declared — "the structured probe dispatcher … lands separately" — so this is stacked-PR dormancy, not undeclared surface, but nothing in this PR can exercise the path it hardens.
• _SYNTHETIC_STOP_REASONS re-spells as raw literals what acp/types.py:297-310 already names (STOP_REASON_STALE_RECOVER, STOP_REASON_TOOL_STALL, STOP_REASON_COMPACTION_FAILED); the boundary forbids the import, but the copies diverge silently, and a newly added synthetic reason fails OPEN — it gets charged as completion evidence. Root cause (no provenance on ACP events) is declared out of scope.
• Title type refactor: ships live behavior — restart retirement (item 2) and the budget ceiling (item 4) fire on real persisted data today.

Subtractions

• Defer MonitorActionDisposition.SUCCESS and the end_turn branch of disposition_for_stop_reason (completion.py:40-41) to the provenance PR — 1 construction site, unreachable: all 3 call sites gate on is_monitor_completion_evidence, which excludes end_turn.
• Defer the persisted fields last_completion_fingerprint, last_completion_disposition, last_completed_at, token_usage_known (models.py:197-200) — 0 readers each (grepped src/); duplicate suppression already runs on wake_in_flight + last_wake_fingerprint.

[FIRST-PRINCIPLES-REVIEWED] 529e202

[bolichen97]

LGTM

[kyleseaman]

Addressed the current blocking review: completed-turn accounting is now staged, persisted, and only then published to live state. Failed writes leave counters, terminal fields, and timers unchanged; repeated cancellation drains the executor write before releasing the service lock. Also fixed the slice-owned import-order failure in chat_runner.py. Focused verification: 600 monitor/provider tests passed; isort, flake8, Linux mypy, black gate, and docs lint passed. CI was not manually rerun; the restack push triggered the normal checks.

[bolichen97]

Please rebase/split this PR to residual-only scope. Merged predecessor #5181 (6ea91612) already supplies durable MonitorState, persistence, and the pure monitor decision layer. Preserve only the still-distinct completion integration: the completion hook/adapters, authoritative token accounting, raw EVENT_COMPLETE correlation, budget charging, and Slack/Discord/dashboard completion-path tests.

This is also the base of the intentional #5182 -> #5183 -> #5184 -> #5185 -> #5186 stack. Rebase the stack in that order after shrinking this base so children do not continue carrying the already-merged #5181 layer.

[bolichen97]

Approved after a full-diff review (scope match, no out-of-scope files, security surface checked, tests verified non-vacuous). Review notes available on request.

[bolichen97]

Re-approving after conflict-resolution rebase: three same-anchor conflicts (main's name-grant audit identity injection + this PR's monitor_completion hook) resolved by keeping both sides; chat_runner keeps main's newer name_grant import set plus this PR's monitoring.completion import; AST-verified, driver body wires monitor_completion at 5 sites; single commit, 21 files.

[kyleseaman]

Addressed the blocking GPT finding in this PR, where the legacy Slack collector owns the completion accounting. Synthetic timeout terminal events are now filtered through is_monitor_completion_evidence before charging a monitor turn. Added a fail-first regression test here and removed the duplicate later-stack test. Focused verification: 350 monitor/Slack tests and 582 provider/babysit tests passed; formatting and flake gates passed.

[kyleseaman]

Fixed in 444a3a42f: completion accounting now excludes every ACP-synthesized terminal (timeout, stale recovery, cancellation/tool-stall/compaction failures, and the empty synthetic fallback), while authoritative raw completion remains billable. Focused monitoring/driver regressions pass.

[kyleseaman]

Addressed the review blocker by excluding ACP end_turn completions from monitor accounting until completion-event provenance can distinguish provider results from stale-stream synthesis. Added regression coverage across the shared driver, dashboard, and Slack paths, updated the documented contract, and restacked the dependent PRs.

Focused local verification: 807 passed (test_messaging_driver.py, test_dashboard_chat.py, test_monitor_turn_completion.py, test_turn_duration_slack.py), plus black gate, flake8, docs lint, and git diff --check.