refactor(monitoring): type the probe boundary to a subject, not to GitHub

[chenmingwei23]

Problem / Motivation

The external probe boundary is declared twice -- _Provider in
monitoring/controller.py and GitHubShadowProvider in monitoring/shadow.py --
and both annotate probe's return as GitHubPullRequestProbeResult.

The return type is the defect, not the repetition. An abstraction typed to its
one concrete implementation is not an abstraction: satisfying either boundary
requires producing a pull request's result type, so a second monitored kind cannot
satisfy either one whatever it returns. The two declarations being near-identical
(they differ by class name, visibility and a docstring) is a symptom worth
tidying, but collapsing them while both still named a GitHub type would leave the
boundary exactly as closed as it is now.

The arity is the second half. probe takes one subject and returns one result, so
a host that answers for many subjects in one request -- which most review and CI
hosts do -- cannot express that without changing a signature every implementation
and every caller depends on.

Why it matters

docs/request-for-change/rfc-consolidated-monitor.md describes a substrate where
a kind is registered as data and each kind implements a probe. That is unreachable
while the probe contract names one host in its return type: the registry would
have nothing to register a second kind as. This is the change that makes a
second kind possible at all, and the next one (a registry where a kind declares
its own objectives) has no seam to attach to without it.

What changed

MonitorProbeResult in monitoring/models.py names no host: the subject's
canonical facts plus the generic MonitorObservation the engine classifies.
GitHubPullRequestProbeResult becomes an implementation of it and keeps
response -- its own typed detail -- on itself rather than on the shared type,
which is the line every future kind gets to draw for itself. The two Protocols
collapse into one public MonitorProbe, and the service boundary
(_Service.apply_monitor_probe and AutoNudgeService.apply_monitor_probe) is
retyped to the shared record, so no path names GitHub in order to describe a probe.

Plural from the start. probe takes a sequence of subjects and returns a
mapping. GitHub answers for one pull request per call, so its implementation loops
and the per-subject logic is untouched in _probe_one. Arity is the one property
that cannot be widened later without touching every implementation and every
caller, so it is settled now; a caller wanting one subject passes a one-element
sequence.

Keyed by the subject as passed, not by an identity the host derives. GitHub's
canonical facts carry github.com/owner/repo#123, which is not the URL the caller
handed over -- keying by the derived form would make the mapping unreadable to the
caller that built the request, and it lets a host normalize a subject for its own
use without that reshaping what its caller has to read. A test asserts the two
differ so this cannot regress into keying by whichever one happens to be handy.

A record, not a bare list of rows. This is the part that keeps a later fix
cheap. The status tool treats a published aggregate verdict as authoritative over
the individual rows, while a provider has no aggregate notion and computes from
row states alone; those two can disagree. Closing that gap is another change's
work, but a probe result shaped as a record leaves room for a host's own
aggregate as a defaulted field that reaches every caller without changing this
type or any signature naming it. A protocol returning a bare sequence of rows
would have to change its return type instead -- the same trap this change is
undoing one layer up.

No aggregate field is added here, deliberately. Nothing would read it, and this
probe fetches no published aggregate today -- it enumerates statusCheckRollup
rows and never reads a rollup state -- so populating one would mean an additional
request, which is a behaviour change this stack's golden table exists to forbid.
An unread field is unverifiable surface. The structural room is the deliverable;
the field belongs to the change that reads it.

Two failure modes fail closed, in BOTH consumers. A plural boundary makes it
possible for a provider to answer for a subset of what it was asked, or to answer
with the wrong shape. Neither is a verdict: an absent subject leaves no observation
to decide from, and the decision engine reads attributes off whatever it is handed,
so an untyped value fails deep inside it rather than at the boundary.

MonitorController.tick and run_shadow_probe both resolve through one shared
resolve_probe_result, so the two cannot disagree about what an unusable answer
means. Guarding one consumer and letting the other raise a bare KeyError on the
same input would make the hazard's meaning depend on which path observed it, which
is the asymmetry a shared boundary exists to remove.

GitHubPullRequestProbeResult.response carries no default. The shared base's
fields have none either, so nothing forced one, and a default would only let a
future caller silently omit the typed response.

Tests

test/test_monitor_behaviour_golden.py is the behaviour proof and it is unchanged
except for its own call into the new signature. Its digest and all 28 per-group
digests were captured on pristine kirocrew/main at 53987e756, before any edit
in this stack, and do not move.

New TestPluralProbeBoundary in test/test_github_pull_request_monitor.py covers
the contract that would be expensive to change later:

• several subjects yield one result each, keyed as passed
• the mapping is keyed by the caller's string, asserted against the derived
  canonical identity being different
• each subject sees only its own previous observation, so a head cannot be carried
  against the wrong subject and fake a changed head
• no subjects probes nothing
• a kind that touches no GitHub type satisfies the same protocol -- the claim this
  change actually makes
• the GitHub provider satisfies it, and its result is an instance of the shared
  record

In test/test_monitor_controller.py: a provider that omits the requested subject
fails closed, a provider returning an untyped value fails closed, and the
controller asks for exactly its own subject rather than widening the request. The
shadow path has the matching pair, so the shared resolver is asserted at both
consumers rather than only where it was first needed, and one test pins that the
GitHub result cannot be constructed without its response.

The ~60 existing provider tests call the boundary through one named helper rather
than each constructing a one-element sequence and indexing the mapping. Those
tests are about what the probe derives from a response, not about arity, and their
assertions are untouched.

A green board here does NOT mean the code is verified

Read this before merging. Because this pull request is based on a branch rather
than main, it runs a much smaller check set: 18 checks against the 63 that run
on its main-based base. The 18 are the review and policy lanes -- Design,
First Principles, GPT, Opus, UX, SAST, Inclusive Language, PR Hygiene, PR Scope,
Dependency License, Cross-Platform Portability, Automated Rule Check, Screenshot
Evidence and PR Readiness.

All 45 absent checks were enumerated by diffing the two rollups. They include
every test job and every deterministic gate: Backend Tests (all four shards, the
Windows shards and the namespace sandbox), Frontend Tests, Gateway Tests, Electron
Shell Tests, E2E, Build Wheel, Build Desktop, Build Windows Installer, Linux
Packaging, Coverage Gate, Frontend Coverage Merge, Backend and Frontend Lint &
Type Check, CodeQL, the internal-content scan, and the Brand Name, Changelog
History, Docs Lint, Feature Map, Focus Cue, Harness Parity, Loop-Bound Locks,
Builtin Skill Scope, Testpaths Coverage, Bundle Size and Vendored Tree Integrity
gates.

So a green board here means the review lanes are satisfied and says nothing about
whether the code works. PR Readiness passing is not equivalent to its passing on
a main-based pull request either: it aggregates a set that excludes the tests.

Merge order, so this is not merged early on a green that does not cover the
code. The base merges first; GitHub then retargets this to main
automatically; the full 63 checks run for the first time at that point; only then
is a green here meaningful.

The stack is still the right shape -- basing this on main would recreate the
monitoring/models.py and monitoring/controller.py conflict the sequencing
exists to prevent, and that conflict is a certainty where this is a deferral. But
it is a deferral, and the deferred half is the tests.

Manual verification

No user-visible surface, and the behaviour claim rests on the golden digests
captured before the change rather than on inspection. Because CI is not covering
the code here, the local run is doing more work than it normally would, so here is
exactly what it did and did not cover.

Test files run locally, all passing: test_monitor_behaviour_golden.py,
test_github_pull_request_monitor.py, test_monitor_controller.py,
test_monitor_decision.py, test_monitor_persistence.py, test_autonudge.py.
Those are the files this diff touches plus the suites that exercise the changed
boundary. NOT run locally: the rest of the backend suite, all frontend tests, the
builds, E2E, Gateway and Electron tests, CodeQL, and the Windows and
namespace-sandbox shards. Those are the gap, and they close when this retargets to
main.

Gates run locally, all passing: the repository's own black gate (diff-scoped),
isort, flake8, mypy over monitoring/ and autonudge.py in one invocation, and
the comment-history, sync-IO, subprocess-encoding, brand-name, feature-map,
focus-cue, changelog-history, harness-parity, loop-bound-locks,
builtin-skill-scope, testpaths-coverage, vendored-manifest and docs-lint scans
(the last with its own self-test). NOT run locally: the repository-wide
mypy src/kiro_crew/, the frontend gates (this diff has no frontend changes) and
cfn-lint (no templates changed).

Review rounds

Both design lanes are advisory CONCERNS with no structured findings, so no span
exists and no disposition comment is warranted. Recording the outcomes here.

Accepted. controller.py logged ERROR: structured monitor provider gave no usable result whenever the resolved result carried PROVIDER_ERROR. That status is
carried by BOTH the synthesized fallback and a transient the provider classified
correctly -- and both carry the provider_transient reason code -- so a status test
cannot separate them, and every ordinary rate limit was reported at ERROR level as an
absent answer, on every tick. The log moved into resolve_probe_result, the only
place that knows which branch it took. No shared type or signature changed, and
shadow.py, which logged nothing on that path, now gets the correct log too. The
guard is a controller-level test driving a real tick; it was verified to fail against
the previous behaviour rather than merely to pass against the new one.

Also declared, previously omitted. MonitorProbeResult.__post_init__ refuses a
non-MonitorObservation observation and a non-dict canonical at construction.

Held: the spec's monitor section is not stale. The same-commit rule applies when a
change alters what a spec documents. Measured against
docs/system-specs/modules/learn-cron-dashboard.md: it contains ZERO occurrences of
every symbol this PR changes -- GitHubShadowProvider, MonitorProbe,
MonitorProbeResult, resolve_probe_result, GitHubPullRequestProvider,
run_shadow_probe. The probe boundary was never documented at that level, so there is
no single-subject picture in it to correct. What the spec does document is behaviour:
the adapter's URL rules, that statusCheckRollup is read in a separate request paired
with headRefOid, the ten-page thread cap, and that MonitorController accepts only a
public GitHub pull request with the review_ready objective. Every one of those
sentences remains true, which the 16,800-row golden table captured on pristine main
is the evidence for.

Held: MonitorProbeResult.__post_init__ is not subsumed by resolve_probe_result.
The two guard different things at different points. resolve_probe_result tests the
object's TYPE (isinstance(result, MonitorProbeResult)); __post_init__ tests its
CONTENTS. Constructing MonitorProbeResult(canonical="not a dict", observation=None)
is refused today; without __post_init__ that same object constructs, satisfies the
type guard, and reaches the decision engine, which reads attributes off None.
Verified by running it. The distinction from the sibling change in this stack, which
DID delete a __post_init__ on the same reasoning, is who constructs the type: there,
a static in-module literal with no external constructor; here, every probe
implementation, which is the extension point this PR exists to create.

Held: plural arity. Unchanged from the earlier round. Arity cannot widen later
without touching every implementation and every caller, which is not true of a
defaulted field, so the cost of deferring is not symmetric with the cost of shipping
it now.

Two commits, and which evidence belongs to which

This PR carries two changes, and the file list flattens a distinction that matters
for reading the second one's evidence. Stating it here because a squash erases it.

Commit 1 -- refactor(monitoring): type the probe boundary to a subject, not to GitHub. Owns monitoring/models.py, monitoring/controller.py,
monitoring/github_pull_request.py, monitoring/shadow.py, and their tests. This is
the change that is supposed to touch shared code: it introduces the shared
MonitorProbeResult and the one public MonitorProbe, and retypes both consumers
onto them.

Commit 2 -- feat(monitoring): register a monitored kind as data, not as four hardcodes (#9546). Owns exactly seven files: monitoring/registry.py (new),
monitoring/shadow.py (the capability guard only), validation.py,
mcp_tools/control.py, dashboard/handlers/autonudge.py, autonudge.py, and
test/test_monitor_kind_registry.py.

Why that split is the acceptance test. The registry change was accepted on the
condition that registering a kind as data must NOT force a change to the decision
engine, a shared result type, or a shared protocol -- and that if it did, the right
response was to stop rather than to add a branch to a shared layer. It did not.
Commit 2 touches monitoring/decision.py zero times, monitoring/models.py zero
times, monitoring/controller.py zero times, and monitoring/github_pull_request.py
zero times. Its only edit to monitoring/shadow.py is the capability guard; the
shared protocol in that file was already replaced by commit 1, so commit 2 had no
reason to touch it. The single class it adds anywhere is the registry's own
dataclass.

A reader looking at the merged 13-file list sees models.py and controller.py and
could reasonably conclude the registry reached into shared code. It did not -- those
are commit 1's work. git show on either commit separates them.

Base drift, not authored here

The GPT lane returned [BLOCK-MERGE] naming website/src/apps/aws-control/DrivePage.tsx
and website/src/components/FileRenderers.tsx. This PR touches neither, and touches
nothing under website/ at all -- all 13 files are under src/kiro_crew/ or test/.
The branch was 50 commits behind main, so the reviewed patch included commits already
merged to main, which is what the First Principles lane independently identified as
base drift. Rebasing onto current main drops that from the reviewed surface. The file
list is byte-identical before and after the rebase, verified by diffing it.

The named finding looks legitimate on its own merits and has been passed to a human as
a candidate issue. It is deliberately NOT fixed here: it is outside this PR's scope,
and an unrelated security change should not ride a refactor.

Related Issues

no linked issue: this is the second of three sequenced foundation changes for the
monitor substrate, and it closes no tracked defect on its own. The omission is
deliberate.

Based on feat/monitor-verdict-payload rather than main, because that change is
open and both touch monitoring/models.py and monitoring/controller.py. The
diff shown here is this change alone; GitHub retargets it to main when the base
merges.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) -- N/A, the spec lands separately
• No secrets, credentials, or internal references in the diff

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both construction sites use keyword arguments (so the inherited field reorder is harmless), and apply_monitor_probe reads only .observation/.canonical, both declared on the base MonitorProbeResult. The registry values (publicly_armable_kinds → {github_pull_request}, publicly_armable_objectives → {review_ready}) reproduce the previous hardcoded allowlists exactly, and the internal gh-pr path still resolves through kind_supports_objective. The plural-boundary consumers both fail closed through resolve_probe_result. Nothing survives falsification, and no new grounded defect appears.

No findings.

[OPUS-REVIEWED] 4f3adb8

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 4f3adb8c6adb75dcf2fefd93d15defb03db2e924 — 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 counts I ran: MonitorProbe.probe has 1 real implementation and 2 call sites (controller.tick, run_shadow_probe), both passing one-element tuples; the registry's four replaced hardcode sites check out (MCP schema, validator FieldSpecs, dashboard handler, shadow gate); the gh-pr second spelling is justified because probes/__init__.py imports PrWatchProbe at module level; batched multi-subject probing is RFC step 7, deferred; no test pinned the old error strings. Final review follows.

First-Principles-Verdict: CONCERNS

Half this diff is the kind registry the description itself defers to "the next [change]", and the plural probe arity has zero multi-subject callers.

Not justified as shipped

• 3 — one consumer, generalized: 1 implementation, 2 callers (controller.py:156, shadow.py:82), both pass a one-element tuple; the multi-subject caller is RFC step 7, unbuilt.
• 5–8 — undeclared: the readable description calls the registry "the next one (a registry where a kind declares its own objectives)", yet monitoring/registry.py, the four boundary rewires and three new refusals ship here (commit 4f3adb8c6 is its own feat: referencing feat(monitoring): register a monitored kind as data, not as four hardcodes #9546, so this may be stack mechanics rather than smuggling — but the intent file I was given does not declare it).

What this change ships

Inventory (10 items) — 5 justified

Intent: make the external probe boundary host-neutral so a second monitored kind can exist — a FIX (of an abstraction typed to its one implementation).

1. Providers implement one shared MonitorProbe protocol instead of two GitHub-typed ones — justified
2. Probe results flow as host-neutral MonitorProbeResult; GitHub's response stays on its subclass — justified
3. probe takes many subjects and returns a mapping — one consumer, generalized: every caller passes exactly one
4. A provider omitting the subject or returning a wrong shape degrades to a transient retry, identically in both consumers — justified
5. Arming boundaries read one kind table instead of four hardcodes; allowlists byte-identical, pinned — undeclared: described as the next change
6. Persisting or updating a monitor under an unregistered kind/objective pair now raises — undeclared new step, unreachable via validated paths today
7. Shadow refusal split into "not implemented for kind" vs "objective not declared", new message — undeclared relabel riding with the registry
8. Internal gh-pr kind registered, spelled a second time with a drift-pin test — undeclared; duplication verified justified (probes/__init__.py imports PrWatchProbe at module level)
9. ~60 provider tests route through _probe_one; golden digests unchanged — justified
10. ERROR log fires only when the resolver synthesizes the fallback, not on a classified transient — justified

Watch

• Plural arity rests on "most review and CI hosts do" plus RFC step 7 ("one poller batches many subjects"), which is deferred work; resolve_probe_result, transient_probe_failure and the subset/untyped/keying test suite exist only to fence hazards the unused plurality creates. Counted: 2 callers, both singular; 1 implementation. Clears when: RFC step 7's batching poller lands as a real multi-subject caller, or the boundary is narrowed to singular.
• The registry half is undeclared in the intent text provided (truncated at 8000 bytes) while its commit carries its own feat: title and PR number (feat(monitoring): register a monitored kind as data, not as four hardcodes #9546). Clears when: confirmed this diff spans only refactor(monitoring): type the probe boundary to a subject, not to GitHub #9505's own commits, or the full description declares the registry commit.

Subtractions

• Narrow MonitorProbe.probe to one subject until a multi-subject caller exists: deletes resolve_probe_result, the subset branch of the failure path, and TestPluralProbeBoundary's arity/keying tests (2 call sites to touch, both already singular).

[FIRST-PRINCIPLES-REVIEWED] 4f3adb8

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 4f3adb8

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound, well-argued substrate work; the risk is procedural — none of the CI that exercises this code runs on this stacked base.

Watch

• Every backend test shard, lint, and type gate is absent from this PR's check set ("18 checks against the 63"), yet the diff adds new refusal paths on the arm/update flow (kind_supports_objective raising in AutoNudgeService). A green board plus PR Readiness passing invites an early merge the description can only warn against, not prevent.
  Clears when: the base merges, this retargets to main, and the full 63-check set runs green before merge.
• The registry is the extension seam the whole stack hangs on ("the next one … has no seam to attach to without it"), but its contract ships only in docstrings and PR prose — "the spec lands separately." Until it lands, a contributor adding a kind has no owning spec to read, which is exactly the failure mode the routing table exists to prevent.
  Clears when: the monitor-substrate spec section covering registry.py, MonitorProbe, and MonitorProbeResult lands within this stack.

Suggestions

• The controller's provider: MonitorProbe | None = None defaulting to GitHubPullRequestProvider() is the fifth hardcode the registry deliberately doesn't own; name that residual in the registry docstring's scoping list so the follow-up change is discoverable from the code, not just the RFC.

[DESIGN-REVIEWED] 4f3adb8

[chenmingwei23]

Dispositioning the First Principles advisory. Two findings accepted and fixed in 571a73c3d; one rebutted with reasoning.

Accepted -- guard placement (items 4 and 5). This was a real asymmetry I introduced and the review located it precisely: I created the subset/wrong-shape hazard by making the boundary plural, then guarded only one of its two consumers. run_shadow_probe indexed the mapping directly, so a subset answer raised a bare KeyError and an untyped value went into decide_monitor, which reads attributes off whatever it is handed and would have failed deep inside the engine rather than at the boundary.

Both consumers now resolve through one shared resolve_probe_result in monitoring/models.py, which is where the review suggested it belong -- out of controller.py, beside the shared types. A guard in one consumer and a raise in the other makes the hazard's meaning depend on which path observed it, which is the opposite of what a shared boundary is for. Tests added on the shadow side to match the controller's pair, so the resolver is asserted at both consumers rather than only where it was first needed.

Accepted -- the = None default on response (item 6). Correct, and it was gratuitous: the shared base's fields carry no defaults either, so nothing forced one. Dropped, with a test pinning that the result cannot be constructed without its typed response.

Rebutted -- plural arity with no plural caller today (item 3). The finding is accurate on the facts and the review concedes the widening-cost argument is real. I am keeping the plural signature, because arity is the one property of this contract that cannot be widened later without touching every implementation and every caller: a mapping return and a sequence parameter change the call shape at both ends, unlike a defaulted field on the result record, which is why the aggregate discussed below is deferred while this is not.

The asymmetry is the whole argument. If arity were as cheap to change later as a field, deferring it would be right on the same reasoning that removed brief from the base change in this stack. It is not: reverting to singular now would guarantee the churn when a batching caller lands, and the cost of settling it early is one internal loop in the one implementation that exists.

On the aggregate the record leaves room for. No aggregate field is added, and that is deliberate for two independent reasons. Nothing would read it. And it cannot be populated here at all: this probe enumerates statusCheckRollup rows and never fetches a rollup state, so filling one requires an additional request, which is a behaviour change the golden digests in this stack exist to forbid. The record shape -- rather than a bare sequence of rows -- is what leaves the room, as a defaulted field that reaches every caller without changing this type or any signature naming it.

The behaviour claim is unchanged by all of the above: the golden digest and all 28 per-group digests, captured on pristine main before this stack began, do not move.

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision 4f3adb8c6adb75dcf2fefd93d15defb03db2e924 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.