docs(spec): one paradigm for every monitoring loop

[chenmingwei23]

Follows #9339, which landed the RFC this spec is the contract for.

Problem

A new monitoring loop has no contract to implement, only an existing watch to copy. The two that exist disagree about nearly every layer, so which one gets copied decides what the new loop can do -- whether it can say what changed, whether it can be batched, whether it re-asserts a condition it missed, whether it stops when stuck.

Neither existing spec is the umbrella. agent-interrupt-controller.md specifies the kernel behind script-cron pollers and babysit-pr-watch.md specifies the pull-request watch built on it; both describe an implementation rather than the paradigm.

Solution

Adds docs/system-specs/modules/monitor-architecture.md as the umbrella above those two, with the index row placing it there.

The goal is a substrate, and the spec now says so in Purpose. Any loop with an external subject plugs in -- a pipeline run, a ticket, a deployment, an alarm, a queue depth -- and a pull request is the FIRST PLUGIN, not the subject matter. Where a layer names a pull request it is naming today's only plugin.

Three prerequisites, stated before the layers

Nothing in the seven layers is reachable until these exist, so they come first:

MonitorDecision is a seven-value enum and decide_monitor returns it bare, so a verdict is an effect selector and nothing more -- it cannot say which observations caused it, and it cannot carry what to tell the woken agent. Both get reconstructed afterwards by format_monitor_wake out of MonitorObservation.canonical and MonitorState.wake_instructions, state the verdict never named. That return type is what forces one fingerprint per subject, not a preference for hashes over names: a verdict with no room for a list has nothing to compare per condition, so the named-entry vocabulary in layer 3 is unreachable until the verdict can hold entries.

_Provider in controller.py and GitHubShadowProvider in shadow.py are two Protocols declaring the same probe method, and both annotate its return as the concrete GitHubPullRequestProbeResult. The duplication is not the defect; the defect is that both generic boundaries name a GitHub-specific type, so the abstraction is typed to its one concrete implementation.

kind and objective are both required with exactly one legal value each. Neither is a Python enum -- objective is a plain str field on MonitorState -- and both are constrained by string allowlists at three separate boundaries: the monitor_watch schema in mcp_tools/control.py, the MONITOR_WATCH_SCHEMA field specs in validation.py, and the REST handler in dashboard/handlers/autonudge.py. Nothing scopes objective to kind, so a shared objective vocabulary means every new kind edits a list it does not own -- the same defect as a dispatch branch wearing a different shape, paid three times.

There is nothing to extend

Recorded as a fact about the package, because it is the precise statement that a second kind has nothing to subclass and no hook to implement. Zero ABC, zero abstractmethod, no behaviour inheritance. The only inheritance is six enums on str, Enum and one exception on RuntimeError; the only polymorphism is four Protocols, three of them private. The nine dataclasses and the two plain classes MonitorController and GitHubPullRequestProvider have no base at all.

A monitor is a field, not a system

This confuses readers who know the code, so the spec now states it. A monitor is a nullable field on a nudge loop: NudgeLoop carries monitor: MonitorState | None alongside gate: bool, and gate is the discriminator -- the field's own comment records that gate=True records belong to the prompt path while controller-owned records carry state with gate=False. One class, three shapes. is_structured_monitor_loop selects the third, and it is the guard the dashboard handlers, the session directive application path and the Slack gateway all branch on. The substrate is therefore a change to what that one field holds, not a new subsystem beside the loop.

The decision is split, and only half is pure

decision.py holds the content policy -- did the subject change, is the budget spent (monitor_budget_reason), is this error retryable (_provider_error_decision against _RETRYABLE_PROVIDER_ERRORS) -- with the clock arriving as its now parameter and no IO at all. The delivery policy is the other half and is impure: MonitorController.tick decides whether a wake is in flight, whether the last dispatch came back busy, and whether the evidence deadline has passed, running the probe off-thread and reading the wall clock through its own injected clock.

Two wiring facts a reader will otherwise trip on. controller.py does not import decide_monitor at all -- its only decision-module import is monitor_budget_reason; it calls service.apply_monitor_probe(...) and the service calls the decider. And terminal_decision_for_outcome records in its own docstring that apply_monitor_probe flattens every terminal outcome to STOP_BLOCKED before decide_monitor runs, so that function's branches are unreachable on the live delivery path, reached only by run_shadow_probe on the persistence-only shadow path. Consolidation merges two halves with different testability rather than lifting one pure function into place.

The layer contracts that carry weight

The probe signature is plural from day one. A per-subject interface cannot be batched later without changing every implementation and every caller, and the cost is not marginal: fifty subjects read one at a time is roughly 150 process invocations against one query. Today batching is reachable only from the single out-of-session poller, for no reason other than interface shape.

An observation is a named entry carrying a key, a severity and a reset scope, never a bare fingerprint. A hash cannot be deduplicated per condition, coalesced with a sibling, or re-asserted, because nothing can tell whether two hashes describe the same condition.

The decision layer is level-triggered. Edge triggering loses any condition that stayed true across a wake that did not happen -- a busy session, an exhausted budget -- because on the next tick it is no longer a change. The re-alert window makes level triggering affordable and the budget makes it safe: a notification pipeline aimed at humans needs no token budget because a paged human self-limits, and an agent does not.

Persisted state is versioned with a migration per bump and holds delivery bookkeeping only. Subject state belongs in the disposable evidence file.

An out-of-session driver is a detector, never a reactor. A cron turn has no owning slot, so its tool calls land on a deny-by-default path and time out, while a denied tool inside a completed turn still records the job as healthy.

An acceptance test that can fail

The "adding a new monitored kind" procedure could not fail, so it now carries one: add a GitHub Actions workflow run as a second kind and change nothing in the shared layers. That kind shares the credential and the CLI, so it adds no authentication work, while being a genuinely different subject with different terminal states and an objective that is not review_ready. It passes only if no shared decision code, no shared result type and no shared protocol changed, and the decision engine's existing tests pass unchanged. If it cannot pass, the prerequisites were wrong and get fixed there -- a branch added for the new kind at that moment is the whole substrate failing quietly.

One boundary, stated rather than papered over

The substrate covers loops with an external subject to probe. A conductor patrolling its own session has no subject to fingerprint, no revision that advances and no host to ask for a verdict, so it stays on the timer path. That is not a gap to close later.

Testing

scripts/docs-lint.sh passes (exit 0, "All documentation checks passed"). scripts/check_brand_name.py passes on the new file. Documentation only; no source file is touched.

Rebased onto 53987e756, and the spec's verified-against commit updated to match. Every status-table row was re-checked against that commit rather than carried forward: all seven rows and both numeric claims still hold. probes/__init__.py still maps one kind in an if with its docstring still recording the deferred registry; monitoring/decision.py is still exactly 119 lines with zero GitHub references and zero IO; irq.py is still 1127 lines holding its own decision logic and unversioned state, which is why its mechanisms are ported rather than its module kept.

The four earlier failures were main-owned and are cleared by the rebase rather than by any change here. The blocking one was test_no_bundled_scripts_are_shipped_here, which asserted builtin_skills/security-conductor/scripts does not exist while that directory does. Upstream narrowed that guard rather than dropping it: at 53987e756 the same class carries test_no_stub_scripts_are_shipped, which permits the directory and instead asserts that every .py file in it is listed in BUNDLED_SCRIPTS and is non-empty. The intent is unchanged -- a present-but-empty script is worse than an absent one -- and only the premise moved, because the old assertion assumed no script had landed yet and scripts/ledger.py since has. Coverage Gate and PR Readiness were downstream aggregates.

The whole file is printable ASCII.

Blocked features

None.

Other suggestions

Three findings a reviewer should weigh.

The lint reports three report-only dead-identifier findings for resets_on and budgets_spent. Those are proposed field names, so their absence from the tree is correct. They are deliberately not baselined: a baseline entry for a name that is about to exist becomes a stale entry someone has to prune.

The "rules the engine enforces" section now marks which rules are target rather than present, because two of them are enforced nowhere in the engine. The stall streak has no engine state at all -- the only consecutive counter in the package is consecutive_provider_errors, which counts provider failures. The aggregate-authority rule is enforced only in the prepare-pr status tool via resolve_readiness_context; the structured provider has no aggregate notion. And the collapse rule is actively contradicted: collapse_superseded in the status tool collapses to the newest attempt per identity, while _normalize_checks gives every check row its own group key and _normalize_check maps CANCELLED to failed, so the provider can wake on a failure that no longer exists. This matters beyond this spec, because it decides whether those rules can be deleted from an agent instruction yet.

babysit-pr-watch.md opens with "two current monitoring modes", which stops being true once the consolidation removes one. Not fixed here.

The spec records one deviation rather than resolving it: every wake re-injects into the same session, so its context grows for the life of the watch, while the prevailing pattern elsewhere is a fresh context per wake. Both current implementations share the deviation. It is larger than this consolidation and wants its own proposal; it is written down so the omission is not mistaken for an argument that same-session wakes are correct.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] d8b0166

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] d8b0166

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

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

The lint can't run here (approval-gated), but it's CI-enforced and line-level regardless. I have what I need: this is a docs-only PR adding an umbrella spec that follows an already-landed RFC (#9339), correctly indexed in both routing tables, with its code-facts (protocol names, module line counts, file layout) verifying against the tree, live links, an explicit status table separating target from present, and a falsifiable acceptance test. The known staleness (babysit-pr-watch.md's opener) and the same-session-wake deviation are both acknowledged with owners deferred, not papered over.

Design-Verdict: PASS

A contract spec for a landed RFC, correctly layered above the two implementation specs, with target-vs-present status made explicit — no design-level concerns.

[DESIGN-REVIEWED] d8b0166

[chenmingwei23]

Design Review CONCERNS on 8508bb6e6, answered per item. Head is now d8b01667c.

Watch -- commit-pinned code census will rot silently. Accepted, fixed. The finding names a real rule: docs/system-specs/README.md says "Do not restate a number the code already pins" for exactly this reason. Every pinned count is gone from the spec, and the symbols stay, since naming a symbol is the durable form the same rule asks for.

• The decision layer's purity is now stated as a property with its evidence -- decide_monitor names no host and reaches no IO, its imports are the state and observation models, its clock arrives as now, and test_monitor_decision.py exercises it with no network and no filesystem. The line count is gone from both the prose and the status table row.
• The class census is now named rather than counted: the enums, the one exception, the Protocols and which of them are private, and the fact that the dataclasses and the two plain classes have no base at all. The claim it supports -- a second kind has nothing to subclass -- does not need a number to be exact.
• The negative-import assertion is gone. It said controller.py does not import decide_monitor; it now says the decider is reached through the service, that the controller calls apply_monitor_probe, and that a reader who opens controller.py expecting the decision finds only monitor_budget_reason. That keeps the navigation a reader actually needs and drops the assertion consolidation would falsify.
• The sharpest part of the finding was that "terminal branches are unreachable on the live path" stops being true the moment the flattening is removed -- which is what this consolidation intends. Rewritten to attribute the claim to the docstring that makes it, and to say plainly that consolidation is where the flattening goes and the branches become live. That is why terminal_decision_for_outcome is named here rather than treated as dead code to delete.

One part not changed, deliberately: Verified against <sha> stays. The review asks to keep the status table as the point-in-time claim, and a point-in-time claim with no point in time is weaker, not cleaner. The rule's ban on SHAs targets changelog narration -- "previously / used to / we now" -- and this stamp is the scope qualifier that tells a reader how far to trust the table. It is one line, it is the only SHA in the document, and it is what makes the table falsifiable rather than vague.

The PR body still carries counts. That is intentional: a PR body describes one moment by nature, and editing it would re-roll a review lane that is currently clean on this head for no gate benefit.

Suggestion -- routing row. Accepted, added. There was no row for this area at all, so nothing routed a new-loop author to the contract before they copied an existing watch, which is the problem this spec is meant to solve. AGENTS.md now routes monitoring/, irq.py and watches here first, then to the two implementation specs.

scripts/docs-lint.sh and scripts/check_brand_name.py pass on both changed files. The three report-only dead-identifier findings for resets_on and budgets_spent remain deliberately un-baselined -- those names are proposed, so a baseline entry for them would become a stale entry to prune.

[bolichen97]

I checked every code claim in this spec against the tree rather than reading it for coherence, and most of it holds: the seven MonitorDecision values, decide_monitor returning the enum bare, both Protocols typed to GitHubPullRequestProbeResult (controller.py:87, shadow.py:33), objective as a plain str, all three allowlist boundaries, NudgeLoop.gate/monitor and is_structured_monitor_loop with its real branch sites, probes/__init__.py's single-if build(), irq.py state genuinely unversioned, and the deadline-preserving cadence at autonudge.py:560-570. Every proposed/target row is labelled correctly, and resets_on/budgets_spent are correctly absent from the tree. No leaked internal references; the file is all printable ASCII.

The problem is the premise the document rests on.

1. "There is nothing to extend" is false, and it is the load-bearing sentence. The scoped part is right — no base class, no ABC, no abstractmethod anywhere in monitoring/. The conclusion is not: "a second kind has nothing to subclass and no hook to implement. The extension point does not exist yet, so the work described here is to create one, not to conform to one." irq.py:255 is class Probe, docstring "Domain half of a watch. Subclass and implement both methods," with two required hooks raising NotImplementedError (irq.py:276, irq.py:284) plus optional tuning() and wake_suffix(). probes/gh_pr.py:302 is class PrWatchProbe(Probe) — today's one kind, already conforming. The spec's own status table credits probes/__init__.py, so the two sections disagree with each other.

   This matters more than a wording slip because of the new AGENTS.md routing row: an author sent here to add a second kind is told to create an extension point, when today's actual first step is subclassing irq.Probe and adding a branch in probes/__init__.build. Stopping that author from copying an existing watch blindly is the document's whole purpose.

2. MonitorObservation.canonical does not exist. MonitorObservation (models.py:221) has exactly seven fields — fingerprint, status, provider_error, supplemental_provider_error, reason_code, summary, head_changed. format_monitor_wake (controller.py:244) takes canonical as a parameter, passed at controller.py:200 as canonical=state.last_observation; that dict originates as GitHubPullRequestProbeResult.canonical (github_pull_request.py:122) and is copied in at autonudge.py:2412. The correct attribution is MonitorState.last_observation. MonitorState.wake_instructions is right.

3. Layer 3 attributes the named-entry vocabulary to the wrong file. "Named entries exist in probes/gh_pr.py" — the type and the vocabulary are in the kernel: Observation(key, severity, brief, epoch_scoped) at irq.py:193 and Severity(WAKE, TERMINAL, NMI) at irq.py:172. The spec's proposed Observation(key, severity, resets_on, brief="") with IMMEDIATE and REVISION/NEVER is a rename of that existing type (epoch_scoped: bool → resets_on, NMI → IMMEDIATE), which the document never says. Presenting a rename as a new type hides that the migration has existing callers.

4. "the only consecutive counter in the package is consecutive_provider_errors" is false. The same dataclass carries quiet_streak ("Consecutive quiet observations since the last delivered turn") with floor_ticks recording the deliveries it forces, and irq.py has its own consecutive-error backstop. The narrower claim this sentence exists to support — no engine state holds a streak of identical verdicts — survives; the "only consecutive counter" wording does not.

5. The Decision row understates irq.py. "Edge-triggered and with no coalescing" is true of monitoring/decision.py but not of "every monitoring loop", which is this doc's stated scope. irq.py already level-triggers on the live cron path: DEFAULT_REALERT_SECS = 6 * 3600 (irq.py:97), per-key alerted timestamps in load_state, _dedupe_key with epoch/sticky sentinels (irq.py:132), a coalesce_secs window, and Severity.NMI documented as bypassing the delay but not the mask (irq.py:172). So the re-assert-after-a-window behaviour is presented as a target the code lacks, when one engine already has it.

6. "gives every check row its own group key" is overstated. Only CheckRun rows do — _normalize_check returns a None group key for them (github_pull_request.py:497) and _normalize_checks substitutes ("independent_check_run", str(row_index)) (:449); StatusContext rows are grouped by ("status_context", context) (:513). The defect the sentence describes is real: CANCELLED maps to "failed" (:488) and the min(...) state fold prefers "failed", so a superseded cancelled attempt reads as a live failure.

Requesting changes rather than commenting because 1 misdirects the exact reader the new routing row sends here, and because 2 and 3 name symbols that do not exist or live elsewhere. The Design round replaced pinned line counts with named symbols precisely because symbols are the durable form — but a wrong symbol name is harder to spot than a stale count, and the docs lint cannot catch it: its dead-identifier pass reported only resets_on and budgets_spent, both intentional. CI is green and all five lanes pass on d8b01667c, so none of this was caught upstream.