refactor(monitoring): remove duplicated logic and unreachable branches

[bolichen97]

Problem / Motivation

src/kiro_crew/monitoring/ says the same thing twice in several places, and keeps
two branches that cannot run. None of it is a bug — it is friction for the next
reader, and in two cases the duplication is the kind that drifts:

• shadow.py's _decision_for_outcome is a line-for-line copy of
  decision.py's _terminal_decision. Two copies of a terminal-outcome policy in
  one module is one edit away from the two paths disagreeing.
• shadow.py reimplements models.py's finite/non-negative number check inline in
  eight lines, while three of the module's other users of that check call the helper.
  A fourth inline copy remains in MonitorActionCompletion.__post_init__ and has
  already drifted — see "Known remaining copy" below.
• probe()'s four except clauses each rebuild, by hand, the classification that
  _provider_exception_kind — fifteen lines below, already called from two other
  sites in the same class — performs.
• _review_threads returns the identical tuple from inside its loop on the last
  iteration and again immediately after it.
• _classify_cli_error re-tests three rate-limit markers that its own first
  statement has already returned on.
• monitor_state_from_dict writes outcome = None in an else branch; that is the
  dataclass default, and its four sibling decoders already rely on it.

Why it matters

Duplicated policy is a correctness risk, not a style one: whoever fixes the
terminal-outcome mapping or the exception classification will find one copy and
leave the other. The unreachable branches cost differently — they make a reader
reason about a case that cannot occur, and in _classify_cli_error they actively
mislead, because reading it suggests the second block is what catches rate limits
when the first block already did.

What changed (motivation → approach → change)

Behaviour-preserving throughout. Six sites, four files, no test changes:

Site	Change
shadow.py	_decision_for_outcome deleted; decision.py's twin promoted to terminal_decision_for_outcome and shared
shadow.py	8-line inline now validation → is_finite_non_negative_number (promoted from models.py); drops the now-unused import math
github_pull_request.py	probe()'s four except clauses → one clause delegating to a new _provider_exception_error, which wraps the pre-existing _provider_exception_kind
github_pull_request.py	_review_threads: in-loop duplicate of the post-loop return deleted; the orphaned loop variable becomes _
github_pull_request.py	_classify_cli_error: the second rate-limit block narrowed to its only reachable marker, "http 429"
models.py	monitor_state_from_dict: redundant else: values["outcome"] = None deleted

Two private helpers became public because a second module in the same package now
uses them. That is the whole extent of the API change: no __all__ exists in this
package, and no spec or doc names either symbol.

Comments in the files touched were corrected against the code, not merely
restated (three of them were wrong):

• is_finite_non_negative_number now names the input that actually reaches its
  except OverflowError — an int outside float range, which is what
  test_monitor_persistence.py and test_github_pull_request_monitor.py already
  exercise with 10**400. The example first written there, Decimal('Infinity'),
  is rejected by the preceding isinstance guard and does not raise anyway.
• _provider_exception_error says what the TRANSIENT/SETUP split actually costs:
  TRANSIENT becomes RETRY_PROVIDER, SETUP is unretryable and retires the monitor
  (STOP_BLOCKED, outcome BLOCKED). It also states plainly that only the reason
  string falls back, so a future third kind needs its own reason rather than
  being stamped "provider_setup".
• terminal_decision_for_outcome does not claim parity with the delivery
  controller. It has none: autonudge's apply_monitor_probe refuses a monitor
  with a recorded outcome before decide_monitor runs, flattening every terminal
  outcome to STOP_BLOCKED, so outcome=SUCCESS yields STOP_SUCCESS on the
  shadow path and STOP_BLOCKED on the delivery path. Documenting the divergence is
  the point — a reader who believed the parity claim would "fix" the shadow path.
• models.py's quiet_ticks comment loses an incident anecdote and a PR-scoped
  aside (AGENTS.md forbids both in code comments); every constraint it carried is
  retained.

Known remaining copy — deliberately not fixed here

grep math.isfinite src/kiro_crew/monitoring/ leaves exactly one non-helper hit after
this PR: models.py:187, inside MonitorActionCompletion.__post_init__. It is a fourth
inline copy of the same predicate and it has already drifted in the way this PR's
premise predicts — it omits the OverflowError guard, so:

MonitorState(..., created_ts=10**400)              -> ValueError   (uses the helper)
MonitorActionCompletion(..., completed_ts=10**400)  -> OverflowError (inline copy)

It is left alone because replacing it changes the raised exception type, and this PR
is behaviour-preserving by contract. Tracked as #9045 with the repro, the one-line fix and
the regression test. Raised by First Principles Review; deferral agreed on those grounds.

One observable delta, disclosed deliberately

shadow.py no longer chains the swallowed OverflowError as __cause__ on the
now validation, because the shared predicate returns a bool and has nothing to
chain from. Exception type and message are unchanged, no test asserts on
__cause__, and the three pre-existing callers of that same predicate — including
MonitorState.__post_init__ — never chained either, so this makes shadow.py
consistent with its module rather than divergent. Reachable only by a caller passing
an out-of-float-range int, which is rejected identically either way. Say the word
and I will drop that one hunk.

Overlap with in-flight work

Three open PRs touch these files: #5305 (github_pull_request.py, models.py),
#5185 (models.py, shadow.py), #5186 (models.py). None deletes or
rewrites a function this PR changes, so the file-overlap bar is not met — but #5305
inserts a return immediately above the except SetupError line this PR removes,
so that one adjacency will conflict. This PR should rebase behind them, not the
reverse: a readability change should absorb the conflict, never charge it to a
feature.

Tests

None added or changed — a behaviour-preserving refactor of covered code should move
no assertion. Instead, each rewrite was proven equivalent before it was kept:

• The four collapsed except clauses were differentially executed old-vs-new over
  57 constructed exceptions (7 errno values × 7 OSError subclasses,
  TimeoutExpired, and SetupError with 6 different __cause__ values): zero
  divergences in the resulting (kind, reason_code). SetupError is
  RuntimeError-derived and TimeoutExpired is SubprocessError-derived, so the
  only subtype relation among the four is FileNotFoundError ⊂ OSError — and
  _provider_exception_kind tests FileNotFoundError ahead of the OSError
  branch, exactly as the old clause order did.
• The narrowed rate-limit block was fuzzed old-vs-new over every 1-, 2- and 3-token
  concatenation of 27 adversarial markers under 4 separators, plus 200,000 random
  strings: zero divergences.
• _review_threads was run against a stub runner on both versions: the page-cap
  case returns (10, False, None) after 10 runner calls in both, the early-exit
  case (3, True, None) after 3 in both.
• The outcome deletion was checked for a missing key, an explicit null, and a
  real value, comparing state.outcome, monitor_state_to_dict,
  monitor_state_public_dict and asdict: identical in all three.
• The deleted shadow._decision_for_outcome was diffed against
  decision.terminal_decision_for_outcome from origin/main: line-for-line
  identical bodies.

2,230 passed, 1 skipped across every test module that imports monitoring.
(22 files) plus test_monitor_mcp.py and test_monitor_start_ack.py, re-run after
the rebase onto 5767e0d9c.

Manual verification

N/A — unit coverage sufficient: the change is confined to pure functions and
exception classification that the 2,230 tests above already cover, and no runtime
surface, wire format, or persisted field changes.

Screenshots / video

Why no screenshot: backend-only refactor under src/kiro_crew/monitoring/; no
frontend file, rendered output, or user-visible string changes.

Related Issues

no linked issue: routine readability sweep of one backend module, not a tracked defect.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality (none needed — no new functionality)
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — learn-cron-dashboard.md documents this module's behaviour, which is unchanged, and names none of the touched symbols
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

All claims verified: the collapsed except clauses are order-equivalent (SetupError is RuntimeError-derived, FileNotFoundError is tested ahead of OSError in _provider_exception_kind just as the old clause order did), the second rate-limit block's extra markers were genuinely unreachable past the function's first statement, no spec or doc names any promoted symbol, and the disclosed __cause__ delta makes shadow.py consistent with the predicate's three pre-existing callers.

Design-Verdict: PASS

Deduplication targets the actual drift risk (two copies of terminal-outcome policy), ownership lands in the right modules, and equivalence was differentially proven.

[DESIGN-REVIEWED] b9c6f77

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of b9c6f77f68de2fc7a0ce1c4a62a4dc054ffd9aa9 — 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 checks complete. I verified the unreachability claims (the first statement of _classify_cli_error does return on the three text markers), the class hierarchy (SetupError(RuntimeError) at github_runner.py:62, so the collapsed except-tuple has only the FileNotFoundError ⊂ OSError subtype relation, which _provider_exception_kind orders correctly), the redundant else (the dataclass default covers it), and counted consumers of both promoted helpers. I found one genuine leftover: a fourth inline copy of the finite-number predicate in the same file the helper lives in — and it has already drifted.

First-Principles-Verdict: CONCERNS

Pure subtraction, every claim verified — but one drifted inline copy of the very predicate this PR deduplicates survives in models.py itself.

What this change ships

Intent: make monitoring/ say each policy once so the next editor cannot fix one copy and miss the other — a FIX (of reader-facing duplication), no new capability.

1. Shadow path now shares decision.py's terminal-outcome policy; its copy deleted — justified
2. Shadow's 8-line inline now check replaced by the models.py helper — justified
3. probe()'s four except clauses collapsed into one delegating to _provider_exception_kind — justified
4. _review_threads duplicate in-loop return deleted — justified
5. Rate-limit re-check narrowed to its one reachable marker "http 429" — justified, verified
6. Redundant outcome = None else-branch deleted — justified
7. Two helpers renamed public — declared; consumers counted (2 and 4, both real)
8. __cause__ no longer chained on shadow's now error — declared deliberately
9. Comments rewritten at five sites, incident anecdote dropped per AGENTS.md — justified

Watch

The description says "the module's three other users of that check call the helper" — I count a fourth inline copy: MonitorActionRecord.__post_init__ at models.py:184-190 reimplements the predicate and has already drifted the way this PR predicts. It lacks the OverflowError guard, so completed_ts=10**400 raises OverflowError where every helper-using sibling raises ValueError. Grep pattern math.isfinite in monitoring/: 2 non-helper hits, 1 fixed (shadow.py), 1 left. Deferring is legitimate (fixing it is not behavior-preserving) — but then the description's "three other users" claim should not stand as the completeness argument.

Subtractions

• Replace the inline completed_ts check at models.py:184-190 with is_finite_non_negative_number — deletes the last drifted copy of the predicate this PR exists to deduplicate (here or in a follow-up, since it corrects OverflowError → ValueError).

[FIRST-PRINCIPLES-REVIEWED] b9c6f77

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] b9c6f77

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

All four changes verify as behavior-preserving refactors:

• terminal_decision_for_outcome rename: body identical, both call sites updated.
• Exception consolidation: _provider_exception_kind reproduces the original four-handler mapping exactly (TimeoutExpired→TRANSIENT, FileNotFoundError→SETUP checked before generic OSError, SetupError→cause check, OSError→transient check), and _provider_exception_error derives the reason code correctly.
• _classify_cli_error: the three removed markers are caught by the unchanged early check at line 687; only "http 429" needed to survive and it does.
• _review_threads loop: the dropped if page + 1 == MAX early return returned the same (unresolved, False, None) that falling out of the loop returns.
• is_finite_non_negative_number matches the inline shadow logic (bool excluded, non-number/negative rejected, OverflowError→False); else: values["outcome"] = None removal is a no-op given the field default.

Nothing survives falsification, and I found no grounded new defect.

No findings.

[OPUS-REVIEWED] b9c6f77

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

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

[bolichen97]

• _provider_exception_error / _classify_cli_error comments overstate retry generosity (span=d2abf5b67a12) — fixed in b9c6f77f6.

third consecutive provider failure retires the monitor, contradicting "only costs a backoff" and "TRANSIENT becomes RETRY_PROVIDER" -> Fix: qualify both as retrying only until the provider-error budget is reached.

Legitimate on both clauses, and verified against the code rather than accepted on
assertion. _provider_error_decision returns RETRY_PROVIDER only while
state.consecutive_provider_errors + 1 < budgets.max_provider_errors
(DEFAULT_MONITOR_PROVIDER_ERRORS = 3), so an unqualified "TRANSIENT becomes
RETRY_PROVIDER" was wrong at the budget boundary.

The rate-limit clause was wrong for a second reason worth recording, since it is the
sharper one: there are two provider-error budgets, and only one of them resets. The
consecutive streak (consecutive_provider_errors) is zeroed on a clean probe, but the
cumulative provider_error_count is only ever incremented, and
monitor_budget_reason retires the monitor at
provider_error_count >= max_provider_errors. So a false-positive RATE_LIMITED
permanently spends one of three lifetime provider errors — it does not "only cost a
backoff", it shortens the watch irreversibly.

Both comments now say so. Since this PR's purpose is partly to make these comments
true, shipping a knowingly-wrong one was not an option; the change is comment-only and
the diff is otherwise unchanged.

[bolichen97]

Backend Tests (3.12, 4) red — attributed to a pre-existing flake on main, not this diff

Shard 4 failed two tests, and Coverage Gate is only its downstream fail-closed
(backend-test=failure -- failing closed), so this is one failure, not three:

FAILED test/test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists::test_a_note_delivered_during_the_copy_survives_the_READER
        - AssertionError: the append ran while the copy was still writing, so the two are concurrent rather than ordered
FAILED test/test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists::test_a_FRESH_gateway_still_orders_the_copy_against_a_delivery
        - AssertionError: a delivery on a fresh gateway ran concurrently with the copy: 'no pool' was read as 'no writer'

Attribution, in the order the module-simplification rules require — main-vs-branch
comparison first, before any code change:

1. main fails these same two tests on its own head. Three of main's four most
   recent failing ci.yml runs are exactly this pair:
   34035151321 (13:07,
   ..._survives_the_READER),
   34026408948 (10:04,
   same test), and
   34022725589 (08:45,
   ..._orders_the_copy_against_a_delivery).
2. This diff cannot reach them. It changes four files, all under
   src/kiro_crew/monitoring/; test/test_snapshot.py imports none of them.
3. They pass locally on this branch — 26/26 for the whole
   TestNotificationCopyWhenNoLiveFileExists class on the Python 3.12 CI-parity venv.

Both assertion messages are about ordering vs. concurrency between a snapshot copy and
a notification delivery, which is the signature of a timing-dependent test rather than a
behavioural break.

Per the campaign rules a confirmed flake gets that one job re-run and nothing else —
no code change, no weakened assertion, no added retry — so I have re-run only the failed
jobs of run 34036217154 and left the diff untouched.

Flagging for a maintainer: this is main's flake, not this PR's, and it is currently
reddening unrelated PRs. It wants its own fix (a real ordering barrier in the test, per
testing-conventions.md § Determinism) rather than a re-run on each PR that trips it. I
have deliberately not widened this PR to attempt it.

[bolichen97]

• A fourth, drifted inline copy of the finite-number predicate survives in models.py — accepted-and-deferred to fix(monitoring): MonitorActionCompletion raises OverflowError where its siblings raise ValueError #9045.

The description says "the module's three other users of that check call the helper" — I count a fourth inline copy: MonitorActionRecord.__post_init__ at models.py:184-190 reimplements the predicate and has already drifted. It lacks the OverflowError guard, so completed_ts=10**400 raises OverflowError where every helper-using sibling raises ValueError. [...] Deferring is legitimate (fixing it is not behavior-preserving) — but then the description's "three other users" claim should not stand as the completeness argument.

Correct, and reproduced rather than taken on assertion. On this head:

MonitorState(kind="k", target="t", objective="o", created_ts=10**400)
  -> ValueError: created_ts must be a finite non-negative number
MonitorActionCompletion(monitor_id="m", fingerprint="f",
                        disposition=MonitorActionDisposition.SUCCESS,
                        completed_ts=10**400)
  -> OverflowError: int too large to convert to float

grep math.isfinite src/kiro_crew/monitoring/ now returns exactly one non-helper hit,
models.py:187 — the copy named here. (The class is MonitorActionCompletion; the
finding says MonitorActionRecord, which is the only inaccuracy in it and does not
affect the substance.)

Deferred, on the reviewer's own grounds: the fix changes the raised exception type,
so it is not behaviour-preserving. This PR's contract with all five review lanes is that
it preserves behaviour throughout; smuggling a behaviour change in at the last round is
precisely the "undocumented bug fix riding in a cleanup PR" failure the refactor rules
name. Tracked as #9045 (deferred-finding, assigned, Due: 2026-10-06) with the
repro, the one-line fix, and the regression test it needs.

The completeness claim is corrected rather than left standing. The PR body no longer
says "three other users"; it now states the count and names the fourth copy and why it is
out of scope. That was the actionable half of this concern and it is fixed in the
description, which needs no push.

[dwu96]

Tier 1 auto-approve: refactor (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: duplicate-helper consolidation (terminal_decision_for_outcome shared by decision.py and shadow.py, _provider_exception_error collapsing four except arms onto the existing _provider_exception_kind, is_finite_non_negative_number reused in shadow.py) plus removal of an unreachable rate-limit marker branch already returned by the top-of-function check and a redundant outcome=None assignment matching the dataclass default - verified behaviour-preserving against the head source, no runtime semantics change.