feat(source-control): detect stuck checks degrading mergeStateStatus without completing (#374)

[kyle-sexton]

Summary

Some checks degrade a PR's mergeStateStatus to UNSTABLE without ever completing, blocking a
clean merge-readiness read even when every REQUIRED check is green. Three observed classes: a job
stuck QUEUED forever on an unmatched self-hosted runner label; an orphaned StatusContext posted
pending with no backing run to cancel; and a non-required check that never settles. This was the
dominant operational wall of the autopilot run — the only thing blocking literal zero — and
cancelling makes it worse (CANCELLED is a failure state).

Per the issue's re-derived shape, this is not a skill: it is an engine-side detection signal
(zero new GitHub fetches — reuse of data the snapshot engine already normalizes) plus a routing
reference doc for judgment-call escalation.

Fix

• Shared classifier (scripts/babysit_checks.py). normalize_check now emits a created_at
  inception timestamp (startedAt for a CheckRun, createdAt for a StatusContext — verified
  empirically against gh pr view --json statusCheckRollup; a CheckRun carries no createdAt),
  distinct from the existing _sort_key which prefers completedAt for latest-wins dedupe. New
  pure function classify_stuck_checks(...) classifies three classes, all pending-category:

  • orphaned_status — StatusContext, pending, empty target_url; not age-gated (no backing
    run means no start time to age against).
  • stuck_queued — CheckRun still QUEUED past the age threshold.
  • never_settling — any other pending check past the age threshold.

  A settled failing check is deliberately excluded. A pending check with no provable start time
  (e.g. a QUEUED CheckRun gh reports without startedAt) is left unflagged for the age-gated
  classes — it fails toward silence rather than a false stuck report. Detection fires only under
  mergeStateStatus == UNSTABLE, whose contract ("mergeable, every REQUIRED gate satisfied, a
  non-required status not passing") makes every check reaching a stuck class non-required by
  construction — the gate supplies the required/non-required split with no per-check flag. Because
  the classifier lives in the shared module, the guarded merge gate sees the same normalization.

• Per-PR classifier (scripts/babysit_delta.py). classify_pr attaches checks.stuck[]
  (always present, empty when none) = {name, type, class, target_url, details_url, age_seconds},
  and — when non-empty — appends a single material_findings entry.

  Correctness guard (the crux of the issue): the signal is material_findings, never a
  blockers string. A sticky blocker would pin classification == active and re-dispatch a
  worker every cycle for a check no branch action can clear (the fan-out trap the
  suppressible/unsuppressible delta model exists to prevent). material_findings is write-only for
  classification — it never feeds blockers, classification, needs_worker, any fan-out delta
  arm, or persisted prev state (only its count is persisted). The pre-existing failing/pending
  blocker buckets are left completely untouched: this change is strictly additive and never
  downgrades anything already blocking, so it cannot weaken the merge gate (which correctly keeps
  refusing UNSTABLE). This is the clean escalation path, not a gate bypass.

• Configurable threshold, mirroring the max_quiet_recheck_seconds pattern:
  DEFAULT_STUCK_CHECK_AGE_SECONDS (1800s) → ClassifyConfig.stuck_check_age_seconds →
  validated_stuck_check_age_seconds → --stuck-check-age-seconds CLI flag → build_config →
  babysit_stuck_check_age_seconds userConfig (plugin.json / SKILL.md effective-config table /
  README config table, all kept in sync).

• Routing doc reference/stuck-checks.md (sibling of freshness.md), wired into SKILL.md
  References + the operational runbook step 5. It points at — never restates — safety.md's
  Stop-and-Ask / Never-Do-Automatically rules, the merge wrapper's requiredChecks JSON, and the
  pull-request skill's readiness.md UNSTABLE fact; routes CI-config fixes to the branch workflow /
  ci-workflows and org/settings/orphaned-status to github-iac / app config; and states the
  never-auto-fix rule (branch-config-fixable cases ride the normal head_sha_changed delta).

Rejected shapes (per the issue): a standalone script (second source of truth for check
normalization), a pure-prose skill (can't detect), a subagent (no benefit for a pure function over
data in hand).

The SSOT lift of the "UNSTABLE ≠ required-check failure" fact into a plugin-scope shared reference
is deferred as a follow-up rather than done here: it would edit the pull-request skill too,
widening the blast radius of a merge-gate-adjacent change. The new doc points at the existing
statement in readiness.md (point-don't-copy) in the meantime.

Verification

All commands run in the worktree against the babysit engine suite.

Baseline before the change: 326 tests, OK; ruff clean. After the change: 337 tests, OK
(+11 new), ruff clean, all guarded-wrapper behavior checks pass:

$ sh engine.test.sh
Ran 337 tests in 6.265s
OK
== ruff ==
All checks passed!
== guarded-wrapper behavior ==
PASS: merge wrapper rejects --allow-unpinned-head
PASS: merge wrapper reaches fail-closed CLI (no allowlist)
PASS: resolve wrapper reaches fail-closed CLI (no allowlist)
PASS: merge wrapper rejects --autopilot-merge-tier without required sets

New test coverage:

• tests/test_babysit_checks.py — ClassifyStuckChecksTests (UNSTABLE-only gating; orphaned not
  age-gated; stuck_queued age-gated young-vs-old; QUEUED-without-start-time unflagged;
  never_settling past threshold; settled failing/success never stuck) and
  NormalizeCreatedAtTests (created_at from startedAt/createdAt).
• tests/test_babysit_delta.py — StuckCheckSignalTests: the load-bearing guard asserts the stuck
  signal appears in material_findings and that blockers gained no stuck-specific string
  (the pre-existing pending-check blocker is still present and intact); plus the not-UNSTABLE and
  always-present-field cases.

CLI smoke:

$ python pr_queue_snapshot.py --help | grep stuck-check-age
  --stuck-check-age-seconds STUCK_CHECK_AGE_SECONDS
$ python pr_queue_snapshot.py --pr owner/repo#1 --owners owner --state-dir <tmp> --stuck-check-age-seconds 0
ERROR: --stuck-check-age-seconds must be a finite number greater than zero

Independent review: a fresh-context code-reviewer subagent audited the staged diff (rationale
withheld). It confirmed the merge-gate invariant (stuck never reaches blockers; material is not
a classification input), the CLI/config threading, the default-value consistency (1800 = 30×60), and
the reference doc, and raised one drift finding — the README config table was missing the new key —
which is fixed in this diff.

Closes #374

Related

• Closes ci-health: detect checks that degrade mergeStateStatus without completing (stuck runner-label / orphaned status / never-settling) #374.
• Evidence source: medley#1567 (the stuck-QUEUED runner-label sentinel) and medley#1597 (the
  orphaned codex-review StatusContext).
• The autopilot retrospective .work/handoffs/babysit-autopilot-retro.md (§1.2 / §1.3 / §1.4 /
  §2.1) is cited by the issue as the origin of these three classes; it is an ephemeral
  working-directory artifact and is not present in this worktree.

[claude]

Claude finished @kyle-sexton's task in 4m 5s —— View job

---

PR Review: feat(source-control): detect stuck checks degrading mergeStateStatus

• Gather context and understand request
• Read PR diff and changed files
• Review babysit_checks.py — classifier logic
• Review babysit_delta.py — signal routing and guard invariant
• Review pr_queue_snapshot.py — CLI flag threading
• Review plugin.json / SKILL.md / README / CHANGELOG
• Review reference/stuck-checks.md
• Review test coverage
• Synthesize findings and post review

---

Summary

Solid, tightly-scoped change. The design rationale (stuck signal → material_findings only, never blockers; UNSTABLE-only gate) is load-bearing and correctly implemented end-to-end. The UNSTABLE gate cleverly uses GitHub's own contract ("every REQUIRED gate satisfied, a non-required status not passing") to get the required/non-required split for free, avoiding any per-check flag. Config threading (default constant → dataclass field → CLI flag → userConfig) is complete and internally consistent. The reference doc is well-structured.

Two real findings and a few minor observations follow.

---

Finding 1 — age_seconds can be null for orphaned entries, but the contract is undocumented

File: babysit_checks.py L289–298

For an orphaned_status entry, age is computed via _check_age_seconds but the class fires unconditionally (not age-gated). When the StatusContext carries no createdAt field — which is the realistic production scenario for some external app status posts — parse_timestamp("") returns None, so age = None, and the entry is emitted with "age_seconds": None.

The stuck-checks.md entry schema ({name, type, class, target_url, details_url, age_seconds}) does not say this field is nullable. The tests always supply createdAt=NOW_TS for orphaned entries, so the null case is never exercised.

Risk: A downstream worker iterating checks.stuck and doing arithmetic on age_seconds (e.g. formatting it for display, computing a staleness tier, or comparing it against a threshold) will get a TypeError on a real orphaned StatusContext with no createdAt. The real-world codex-review example from the PR description fits this case: an external app posting a commit status may not populate createdAt in statusCheckRollup.

Suggestions (pick one):

• Add a test where the orphaned StatusContext has no createdAt, assert age_seconds is None, and document the nullable contract in stuck-checks.md (e.g. "age_seconds: float seconds if available, null when the check carries no inception timestamp — always null for orphaned status contexts with no createdAt").
• Or default age to 0 for orphaned entries since the age field is meaningless there anyway (orphaned detection is structural, not age-based), which makes the field always a float and avoids the consumer null-guard burden.

Fix this →

---

Finding 2 — never_settling path for IN_PROGRESS CheckRuns is untested

File: tests/test_babysit_checks.py L216–220

test_never_settling_pending_past_threshold uses a StatusContext with a non-empty targetUrl. An IN_PROGRESS CheckRun past the age threshold also reaches never_settling (it doesn't match the QUEUED branch, so it falls to elif aged_out), but this path is untested.

Not a code bug — the logic correctly handles it — but the test suite doesn't cover it, and it's a distinct production scenario (a long-running job that never finishes vs. a job that never started). The distinction matters for the runbook (a stuck IN_PROGRESS job looks different in GitHub UI than a never-started QUEUED one, and routes to different triage).

Fix this →

---

Minor observations (no action required)

Double-validation of stuck_check_age_seconds: validated_stuck_check_age_seconds is called in main() at line 572 for an early fatal exit, and again inside classify_pr at line 349 as a direct-call guard. This is redundant but harmless, and it precisely mirrors the max_quiet_recheck_seconds pattern — so this is intentional and consistent.

or fallback in build_config (getattr(args, "stuck_check_age_seconds", None) or delta.DEFAULT_STUCK_CHECK_AGE_SECONDS): passes 0.0 through the or to the default silently, but the early main() validation catches 0 before build_snapshot is called. Same behavior as max_quiet_recheck_seconds.

No min constraint on babysit_stuck_check_age_seconds in plugin.json: Consistent with babysit_max_quiet_recheck_seconds. Runtime validation compensates.

---

Invariant correctness — verified

The load-bearing claim in the PR description ("stuck never reaches blockers") holds:

• classify_stuck_checks returns a list appended to checks["stuck"].
• classify_pr appends to material when checks["stuck"] is non-empty (line 618–623) — never to blockers.
• The pre-existing pending-check blocker (f"{len(checks['pending'])} pending check(s)") fires independently as before; the orphaned StatusContext is still counted as pending and still contributes to that existing blocker string. This is correct — it keeps classification == active so the worker is dispatched, but the worker sees the material_findings entry and escalates rather than auto-fixing.
• StuckCheckSignalTests.test_stuck_check_is_material_never_a_blocker validates this contract at the delta level.

The UNSTABLE-only gate, config threading from constant → ClassifyConfig.stuck_check_age_seconds → CLI → userConfig, and the always-present checks.stuck field (empty array when none) are all correctly implemented.

[claude]

Claude finished @kyle-sexton's task in 4m 8s —— View job

---

Security Review

• Gather PR diff and changed files
• Review babysit_checks.py for injection surfaces, unsafe data handling
• Review babysit_delta.py for logic/authorization issues
• Review pr_queue_snapshot.py for CLI arg handling and injection
• Review stuck-checks.md reference doc for sensitive info exposure
• Review plugin.json manifest changes
• Review test files for security-relevant gaps
• Synthesize findings and post review

---

No security issues found. Full analysis below.

Merge-gate integrity (CRITICAL surface — clean)

The core security invariant of this PR is correctly implemented and tested. Stuck-check detection writes only to material_findings, never to blockers, classification, or needs_worker. The persisted_pr_state projection in babysit_state.py:193 confirms checks["stuck"] is not persisted — only the material_count integer is. This means:

• A PR holding UNSTABLE is still refused by the merge gate every cycle (correct).
• No stuck signal can re-pin classification == active and trigger perpetual worker re-dispatch (the fan-out trap the PR specifically guards against).
• The signal cannot bleed across snapshot cycles through persisted state.

The test at test_babysit_delta.py explicitly asserts blockers gains no stuck-specific string, which is the right guard to have.

Injection surfaces (clean)

The new code is purely functional — no subprocess calls, no eval/exec, no file I/O, no shell commands introduced. All value extraction uses str(check.get(...) or "") coercions. The existing run_command allowlist in babysit_util.py is not touched.

Config/CLI input validation (clean)

validated_stuck_check_age_seconds correctly rejects non-finite values, zero, and negative values via math.isfinite(seconds) and seconds > 0. The CLI smoke test in the PR description confirms the --stuck-check-age-seconds 0 path reaches this error. Negative values, NaN, and -0.0 all satisfy the rejection condition. The float(...) or default pattern in build_config is safe because explicit validation runs at line 572 of pr_queue_snapshot.py before build_config is ever called.

orphaned_status classification boundary (clean)

The condition check.get("type") == "StatusContext" and not check.get("target_url") fires for empty-string target_url (the normalized form of any absent/null targetUrl). A space-padded target_url like " " is truthy in Python and correctly falls through to the age-gated never_settling path rather than the structural orphaned_status path. The UNSTABLE outer gate prevents any stuck classification from reaching non-required checks when required gates are still pending — the UNSTABLE contract itself supplies the required/non-required split.

URL passthrough (pre-existing behavior, not a new risk)

target_url and details_url from the GitHub API pass through as strings into the checks["stuck"] output array without URL validation. This is identical to the pre-existing behavior for the same fields in every other normalized check in the snapshot. These fields are NOT surfaced in material_findings strings (the material message only includes a count) and NOT in blockers. They appear only in the structured JSON snapshot consumed by the orchestrating agent. This is not a new attack surface introduced by this PR.

plugin.json manifest (clean)

The new babysit_stuck_check_age_seconds userConfig field is typed "number" with a safe default of 1800. No new permissions, egress endpoints, or trust delegations are introduced.

Reference doc (clean)

stuck-checks.md contains no secrets, no hardcoded credentials, no PII, and correctly defers to safety.md for the Stop-and-Ask / Never-Do-Automatically rules rather than restating them.

---

Summary: This PR introduces no new injection surfaces, no credential exposure, no merge-gate bypass, and no new persistence of sensitive data. The security-relevant invariant (stuck signal is report-only, never a gate input) is both correctly implemented and explicitly tested.

[claude]

Claude finished @kyle-sexton's task in 4m 2s —— View job

---

Security Review — PR #790 (stuck checks detection)

• Gather PR diff and changed files
• Review babysit_checks.py — classifier logic, injection surfaces
• Review babysit_delta.py — signal routing, authorization gaps
• Review pr_queue_snapshot.py — CLI arg handling, input validation
• Review stuck-checks.md — sensitive info exposure
• Review plugin.json manifest changes
• Synthesize findings and post review

---

No security issues found. Full analysis below.

Merge-gate integrity (CRITICAL surface — clean)

The load-bearing invariant is correctly implemented and tested. Stuck-check detection writes only to checks["stuck"], which surfaces only as a material_findings entry — never to blockers, classification, or needs_worker. Specifically:

• babysit_delta.py L618–623: the only write site — material.append(...), not blockers.append(...)
• The pre-existing pending-check blocker at f"{len(checks['pending'])} pending check(s)" fires independently and is untouched
• StuckCheckSignalTests.test_stuck_check_is_material_never_a_blocker explicitly asserts both sides of this invariant

This means a PR holding UNSTABLE is still refused every cycle; no stuck signal can re-pin classification == active and trigger perpetual worker re-dispatch (the fan-out trap the PR guards against).

Injection surfaces (clean)

The new code is purely functional — no subprocess calls, no eval/exec, no file I/O, no shell commands. All value extraction uses str(check.get(...) or "") coercions feeding a pure classifier. parse_timestamp uses datetime.fromisoformat() with a caught ValueError — no injection possible through timestamp strings. generated_at is set to datetime.now(UTC).isoformat() internally (not user-supplied).

UNSTABLE gate (clean)

All stuck-check classification is gated on merge_state == "UNSTABLE" at babysit_checks.py L265–266. The merge_state value derives from str(pr.get("mergeStateStatus") or "").upper() — a GitHub-computed field, not attacker-controlled via user input. The UNSTABLE contract ("mergeable, every REQUIRED gate satisfied, a non-required status not passing") supplies the required/non-required split, preventing the classifier from erroneously touching required checks.

Input validation (clean)

validated_stuck_check_age_seconds correctly rejects non-finite values, zero, and negative values via math.isfinite(seconds) and seconds > 0. The CLI argument is typed float in argparse. Validation runs at L572 before build_config is called, so the or-fallback pattern in build_config (which would silently pass 0.0 through to the default) can never be reached with a zero input.

URL passthrough (pre-existing behavior)

target_url and details_url from the GitHub API pass through as strings into checks["stuck"] without URL validation — identical to the pre-existing behavior for the same fields in every other normalized check. These values appear only in the structured JSON snapshot consumed by the orchestrating agent; they are not rendered in HTML, passed to any subprocess, or included in material_findings strings (the material message includes only a count). Not a new attack surface introduced by this PR.

orphaned_status not age-gated (by design — clean)

An orphaned StatusContext (pending, empty target_url) fires unconditionally without an age check. This is intentional: no backing run means no start time and no way for the check to ever settle, making an age gate meaningless. The UNSTABLE outer gate and the material-only routing still apply.

plugin.json manifest (clean)

The new babysit_stuck_check_age_seconds userConfig field is typed "number" with a safe default of 1800. No new permissions, egress endpoints, or trust delegations are introduced.

Reference doc (clean)

stuck-checks.md contains no secrets, credentials, or PII. It correctly defers to safety.md for Stop-and-Ask / Never-Do-Automatically rules rather than restating them, and explicitly states the never-auto-fix rule.

---

Summary: No injection surfaces, no credential exposure, no merge-gate bypass, and no new persistence of sensitive data. The security-relevant invariant (stuck signal is report-only, never a gate input) is correctly implemented and explicitly tested.

[claude]

Claude finished @kyle-sexton's task in 2m 54s —— View job

---

PR Review: feat(source-control): detect stuck checks degrading mergeStateStatus

• Gather context and understand request
• Read full diff (origin/main...HEAD)
• Review babysit_checks.py — classifier logic and normalize_check changes
• Review babysit_delta.py — signal routing and guard invariant
• Review pr_queue_snapshot.py — CLI flag threading
• Review plugin.json / SKILL.md / README / CHANGELOG
• Review reference/stuck-checks.md
• Review test coverage
• Check new commit (SKILL.md 500-line cap fix)
• Synthesize findings and post review

---

Summary

The fix commit (1ca6276) is clean — a meaning-preserving text compression that brings SKILL.md to 499 lines (confirmed wc -l). The underlying PR is a tight, well-reasoned design: the UNSTABLE gate that drives the required/non-required split without any per-check flag is clever, and the material_findings-not-blockers routing contract is correctly implemented and tested end-to-end.

Two findings from the prior review cycle are still open; no new issues found.

---

Finding 1 — age_seconds is null for no-createdAt orphaned entries, but undocumented

File: babysit_checks.py L288–292

For orphaned_status entries, age is computed via _check_age_seconds but the class fires unconditionally (not age-gated). When created_at is empty — likely for real-world external-app commit statuses — parse_timestamp("") returns None, so age = None, and the entry is emitted with "age_seconds": None.

The stuck-checks.md schema describes each entry as {name, type, class, target_url, details_url, age_seconds} without saying the field is nullable. The delta test test_stuck_check_is_material_never_a_blocker actually does produce a null age_seconds (its STUCK_ORPHAN fixture has no createdAt), but the test never asserts the field's value, so the null path is implicitly exercised but not validated.

A consumer formatting or doing arithmetic on age_seconds (e.g. a "staleness tier" display or age > X comparison) will get a TypeError on a real orphaned StatusContext from an external app.

Suggestions (pick one):

• Test the null case explicitly in NormalizeCreatedAtTests or ClassifyStuckChecksTests, then document the nullable contract in stuck-checks.md ("age_seconds: seconds if available, null when no inception timestamp is known — expected for orphaned_status entries from apps that omit createdAt").
• Or default age to 0.0 for orphaned entries (since age is structurally meaningless there — the class fires unconditionally), making the field always a float and removing null-guard burden from consumers.

Fix this →

---

Finding 2 — never_settling path for IN_PROGRESS CheckRuns is untested

File: tests/test_babysit_checks.py L216–220

test_never_settling_pending_past_threshold uses a StatusContext with a non-empty targetUrl. An IN_PROGRESS CheckRun past the age threshold also reaches never_settling (it doesn't match the QUEUED branch, falls through to elif aged_out), but this path has no test case.

Not a code bug — the logic correctly handles it — but it's a real, distinct production scenario (a long-running job that never completes vs. one that never started), and it routes to different triage in the runbook.

Fix this →

---

Minor observations (no action required)

observed_at is a function parameter, not a local — there's no ordering hazard at the classify_stuck_checks call site (line 377). The parameter is defined at line 338 of classify_pr.

SKILL.md line count is 499 — the new commit correctly stays under the 500-line skill-quality cap.

Double-validation of stuck_check_age_seconds (in main() and again inside classify_pr) mirrors the max_quiet_recheck_seconds pattern — consistent and intentional.

or fallback in build_config (getattr(...) or DEFAULT_STUCK_CHECK_AGE_SECONDS) silently passes 0.0 to the default, but the early main() validation catches zero before build_snapshot is ever called. Same behavior as the existing max_quiet_recheck_seconds path.

---

Invariant correctness — verified

The load-bearing guarantee ("stuck signal never reaches blockers") holds cleanly:

• classify_stuck_checks returns a list attached at checks["stuck"].
• classify_pr appends to material when checks["stuck"] is non-empty — never to blockers.
• The pre-existing pending-check blocker fires independently as before; the orphaned StatusContext still counts as pending and still contributes to that existing string, keeping classification == active so the worker is dispatched — but the worker sees the material_findings entry and escalates rather than auto-fixes.
• test_stuck_check_is_material_never_a_blocker asserts both sides of this contract: the material entry is present AND the blockers list gains no stuck-specific string.
• The persisted_pr_state projection confirms checks["stuck"] is not persisted (only the material_count integer is), so no stuck signal bleeds across snapshot cycles.

[claude]

Claude finished @kyle-sexton's task in 1m 19s —— View job

---

PR Review: feat(source-control): detect stuck checks degrading mergeStateStatus

• Gather context and understand request
• Read new commit (ec8cbe9) — verify both open findings addressed
• Check test_babysit_checks.py — IN_PROGRESS test and nullable age_seconds test
• Check stuck-checks.md — nullable contract documented
• Synthesize findings and post review

---

New commit (ec8cbe9) closes both prior findings

The commit is strictly additive — 2 files, +20/-1 lines — and precisely addresses the two open items from prior review cycles.

Finding 1 — age_seconds nullable contract: resolved

stuck-checks.md L12–15 now documents the contract precisely:

age_seconds is the check's age at snapshot time in seconds, or null when no inception timestamp is known — expected for orphaned_status entries from apps that post a pending status without a createdAt, which is also why that class is detected structurally rather than by age.

test_babysit_checks.py L230–237 adds test_orphaned_status_without_inception_has_null_age: constructs an orphaned StatusContext with no createdAt, asserts the class is STUCK_ORPHANED_STATUS, and explicitly asserts age_seconds is None. This directly validates the path that was previously exercised but unasserted via STUCK_ORPHAN in the delta test fixture.

Finding 2 — never_settling for IN_PROGRESS CheckRuns: resolved

test_babysit_checks.py L223–228 adds test_never_settling_in_progress_check_run_past_threshold: a CheckRun with status="IN_PROGRESS" and startedAt=OLD_TS (24 h before GEN, well past the 1800 s threshold). It asserts class == STUCK_NEVER_SETTLING and age_seconds > THRESHOLD. The distinct "long-running job that never completes" scenario (vs. the "never started" stuck_queued path) is now covered.

---

Invariant correctness — confirmed throughout

The load-bearing guarantee (stuck → material_findings, never blockers) remains intact and untouched by this commit. The test suite now covers every branch in the classifier:

Path	Test
Only fires under UNSTABLE	test_only_fires_under_unstable
orphaned_status — with createdAt	test_orphaned_status_is_not_age_gated
orphaned_status — no createdAt, null age	test_orphaned_status_without_inception_has_null_age ✨
stuck_queued — young vs old	test_stuck_queued_is_age_gated
stuck_queued — no startedAt, unflagged	test_queued_without_start_time_is_unflagged
never_settling — StatusContext with targetUrl	test_never_settling_pending_past_threshold
never_settling — IN_PROGRESS CheckRun	test_never_settling_in_progress_check_run_past_threshold ✨
Settled failing/success — never stuck	test_settled_and_success_checks_are_never_stuck

No new issues found. This PR is clean and ready to merge.

[claude]

Claude finished @kyle-sexton's task in 2m 29s —— View job

---

Security Review — PR #790 (stuck checks detection)

• Gather context and understand request
• Get PR diff and identify changed files
• Review babysit_checks.py — classifier logic, injection surfaces
• Review babysit_delta.py — signal routing, authorization gaps
• Review pr_queue_snapshot.py — CLI arg handling, input validation
• Review stuck-checks.md — sensitive info exposure
• Review plugin.json manifest changes
• Review test files for security-relevant gaps
• Synthesize findings and post review

---

No security issues found. Full analysis below.

Merge-gate integrity (CRITICAL surface — clean)

The load-bearing invariant is correctly implemented and tested. Stuck-check detection writes only to checks["stuck"], which surfaces only as a material_findings entry — never to blockers, classification, or needs_worker. The single write site in babysit_delta.py L610–623 appends only to material, and StuckCheckSignalTests.test_stuck_check_is_material_never_a_blocker explicitly asserts both sides: the material entry is present and blockers gains no stuck-specific string.

Consequence: a PR holding UNSTABLE is still refused every cycle; no stuck signal can re-pin classification == active and trigger perpetual worker re-dispatch.

Injection surfaces (clean)

The new code is purely functional — no subprocess calls, no eval/exec, no file I/O, no shell commands. All API-sourced values are extracted via str(check.get(...) or "") coercions feeding a pure classifier. parse_timestamp uses datetime.fromisoformat() with a caught ValueError — timestamp strings from GitHub cannot cause injection. The material findings string interpolates only len(checks['stuck']) (a Python int), not any check name, URL, or other API-sourced value, so a malicious external-app check name cannot inject into the findings string.

UNSTABLE gate (clean)

All stuck classification is gated on merge_state == "UNSTABLE" at babysit_checks.py L265–266. The value derives from str(pr.get("mergeStateStatus") or "").upper() — a GitHub-computed field. The UNSTABLE contract ("mergeable, every REQUIRED gate satisfied, a non-required status not passing") supplies the required/non-required split, preventing the classifier from touching required checks even when a future timestamp on an API-sourced check produces a negative computed age (negative age < threshold → aged_out = False, check is unflagged — safe behavior).

Input validation (clean)

validated_stuck_check_age_seconds correctly rejects non-finite values, zero, and negative values via math.isfinite(seconds) and seconds > 0. This catches NaN, ±Inf, 0, and −0.0. Validation runs at pr_queue_snapshot.py L569 before build_config is ever called, so the float(... or DEFAULT) fallback in build_config that would silently accept 0.0 is unreachable with a zero input.

URL passthrough (pre-existing behavior — not a new risk)

target_url and details_url from the GitHub API pass through as strings into checks["stuck"] without URL validation. This mirrors the pre-existing treatment of those same fields in every other normalized check. These values appear only in the structured JSON snapshot consumed by the orchestrating agent; they are not interpolated into any natural language prompt, not rendered in HTML, and not passed to any subprocess. Even a crafted target_url containing prompt-injection content would appear only in a structured JSON field, not in a prompt context where injection would be effective. Not a new attack surface introduced by this PR.

orphaned_status boundary (clean)

check.get("type") == "StatusContext" and not check.get("target_url") fires for empty-string target_url (the normalized form of any absent/null targetUrl). A space-padded value like " " is truthy in Python and correctly falls through to the age-gated never_settling path. A QUEUED CheckRun without startedAt produces an empty created_at → parse_timestamp("") returns None → age = None → aged_out = False → the check is silently unflagged (fail toward silence, not false report).

Nullable age_seconds for orphaned entries — documented and tested (no security concern)

The latest commit (ec8cbe9) adds test_orphaned_status_without_inception_has_null_age (asserting age_seconds is None for an app that posts a pending status without createdAt) and updates stuck-checks.md to document the nullable contract. A consumer doing arithmetic on age_seconds without a null-check would get a TypeError, but that is a correctness concern for the consuming orchestrator, not a security vulnerability in this engine.

plugin.json manifest (clean)

The new babysit_stuck_check_age_seconds userConfig field is typed "number" with a safe default of 1800. No new permissions, egress endpoints, or trust delegations.

Reference doc (clean)

stuck-checks.md contains no secrets, credentials, or PII. It explicitly states the never-auto-fix rule and correctly defers to safety.md for Stop-and-Ask / Never-Do-Automatically conditions.

GitHub Actions patterns (clean)

No workflow files changed. No pull_request_target, no workflow_run, no github context interpolation in run: blocks, no permission-widening, no dependency pin changes.

---

Summary: No injection surfaces, no credential exposure, no merge-gate bypass, and no new persistence of sensitive data. The security-relevant invariant (stuck signal is report-only, never a gate input) is correctly implemented and explicitly tested. The prior review findings (nullable age_seconds, untested IN_PROGRESS path) are both addressed in the latest commit.

[kyle-sexton]

Post-green review classification (work lane):

Finding	Source	Classification	Disposition
age_seconds nullable for orphaned_status entries with no createdAt, undocumented	2 independent PR-review passes	VALID, non-blocking	Fixed — commit ec8cbe9: test added, nullable contract documented in stuck-checks.md
never_settling untested for IN_PROGRESS CheckRuns	2 independent PR-review passes	VALID, non-blocking	Fixed — commit ec8cbe9: test added
Everything else	3× security-review, 2× PR-review across both CI runs	No other findings; all 3 security passes independently returned clean, including explicit confirmation of the merge-gate invariant (material_findings never blockers, persisted_pr_state never leaks checks["stuck"])	No action needed

No DEFER-FORBIDDEN class applies (security-posture explicitly clean across 3 independent passes; the load-bearing fail-open-risk invariant — stuck signal can never re-pin classification == active or feed dispatch — was independently re-verified after the fix commit, not just before it).

No inline review threads exist on this PR (GraphQL query returned empty) — nothing to resolve.

Handing off to the babysit lane for ongoing watch.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

Claude finished @kyle-sexton's task in 2m 4s —— View job

---

PR Review: feat(source-control): detect stuck checks degrading mergeStateStatus

• Gather context and understand request
• Read PR diff (HEAD~3...HEAD — all 3 commits)
• Review babysit_checks.py — classifier and normalize_check changes
• Review babysit_delta.py — signal routing and guard invariant
• Review test coverage — confirm prior findings fixed
• Review reference doc and config threading
• Synthesize and post final review

---

Summary

This is a well-scoped, correctly-implemented detection signal with a clear design rationale. Five prior review passes (3 security + 2 code review) across two CI runs already audited this in depth. The two open findings from earlier cycles are both closed in ec8cbe9. The PR is clean and ready to merge.

No new issues found. Full verification below.

---

Prior findings — confirmed resolved

Finding 1 — nullable age_seconds for no-createdAt orphaned entries

stuck-checks.md L12–15 now documents the nullable contract explicitly: "age_seconds is … null when no inception timestamp is known — expected for orphaned_status entries from apps that post a pending status without a createdAt."

test_babysit_checks.py L230–237 adds test_orphaned_status_without_inception_has_null_age: constructs an orphaned StatusContext with no createdAt and explicitly asserts age_seconds is None. ✅

Finding 2 — never_settling for IN_PROGRESS CheckRuns untested

test_babysit_checks.py L223–228 adds test_never_settling_in_progress_check_run_past_threshold: CheckRun with status="IN_PROGRESS" and startedAt=OLD_TS (24 h past threshold), asserts class == STUCK_NEVER_SETTLING and age_seconds > THRESHOLD. The distinct "long-running job that never completes" path is now covered. ✅

---

Classifier logic — verified

The three classification branches in babysit_checks.py L265–298 are correct:

• UNSTABLE-only gate at line 265 — any other mergeStateStatus returns [] immediately.
• orphaned_status fires structurally (empty target_url, StatusContext, pending) — not age-gated, because no backing run means no start time and the check can never settle. Age may be None for apps that omit createdAt; this is now documented and tested.
• stuck_queued fires only when aged_out = True; a QUEUED CheckRun without startedAt produces age = None → aged_out = False → silenced (fails toward silence rather than false report).
• never_settling is the correct catch-all for any other pending check (IN_PROGRESS CheckRun, StatusContext with non-empty target_url) past the threshold.
• Settled failing/success checks have category != "pending" and are filtered out at the top of the loop.

The created_at field added to normalize_check correctly prefers startedAt for CheckRuns (which carry no createdAt) and createdAt for StatusContexts, consistent with the empirical findings cited in the PR description.

Routing invariant — confirmed

babysit_delta.py L610–623: checks["stuck"] appends only to material, never blockers. The pre-existing pending-check blocker fires independently and is untouched — an orphaned StatusContext still counts as pending and keeps classification == active for worker dispatch, but the worker sees the material_findings entry and escalates rather than auto-fixes.

test_stuck_check_is_material_never_a_blocker asserts both sides of this contract, and test_checks_stuck_field_always_present confirms the field exists even when empty, providing a stable consumer API.

Config threading — complete and internally consistent

DEFAULT_STUCK_CHECK_AGE_SECONDS = 30 * 60 = 1800 → ClassifyConfig.stuck_check_age_seconds → validated_stuck_check_age_seconds (correct rejection of zero, negative, non-finite) → --stuck-check-age-seconds CLI flag with early validation in main() before build_snapshot → build_config → babysit_stuck_check_age_seconds userConfig in plugin.json (type "number", default 1800) → SKILL.md effective-config table → README. All layers consistent.

Test coverage — complete

Scenario	Test
Only fires under UNSTABLE	test_only_fires_under_unstable
orphaned_status — with createdAt (not age-gated)	test_orphaned_status_is_not_age_gated
orphaned_status — no createdAt, null age ✨	test_orphaned_status_without_inception_has_null_age
stuck_queued — young vs old (age gate)	test_stuck_queued_is_age_gated
stuck_queued — no startedAt, silenced	test_queued_without_start_time_is_unflagged
never_settling — StatusContext with targetUrl	test_never_settling_pending_past_threshold
never_settling — IN_PROGRESS CheckRun ✨	test_never_settling_in_progress_check_run_past_threshold
Settled failing/success — never stuck	test_settled_and_success_checks_are_never_stuck
Delta: stuck → material_findings, never blockers	test_stuck_check_is_material_never_a_blocker
Delta: no stuck when not UNSTABLE	test_stuck_absent_when_not_unstable
Delta: checks.stuck always present	test_checks_stuck_field_always_present

Reference doc — correct

stuck-checks.md is correctly structured: report-only signal, nullable age_seconds documented, three-class routing table, the "never auto-fix" / "cancel makes it worse" rule explicit, UNSTABLE-contract rationale stated, and Stop-and-Ask conditions deferred to safety.md rather than restated.

---

No new issues found. This PR is clean.

[claude]

Claude finished @kyle-sexton's task in 2m 24s —— View job

---

Security Review — PR #790 (stuck checks detection)

• Gather context and read PR diff
• Review babysit_checks.py — classifier logic, injection surfaces
• Review babysit_delta.py — signal routing, authorization gaps
• Review pr_queue_snapshot.py — CLI arg handling, input validation
• Review stuck-checks.md — sensitive info exposure
• Review plugin.json manifest changes
• Review test files for security-relevant gaps
• Synthesize findings and post review

---

No security issues found. Full analysis below.

Merge-gate integrity (CRITICAL surface — clean)

The load-bearing invariant is correctly implemented and tested. Stuck-check detection writes only to checks["stuck"], which surfaces only as a material_findings entry — never to blockers, classification, or needs_worker. The single write site in babysit_delta.py L610–626 appends only to material. The material string interpolates only len(checks['stuck']) — a Python int — so no API-sourced check name, URL, or external string can appear there.

StuckCheckSignalTests.test_stuck_check_is_material_never_a_blocker explicitly asserts both sides of this contract: the material entry is present and blockers gains no stuck-specific string. A PR holding UNSTABLE is still refused every cycle; no stuck signal can re-pin classification == active and trigger perpetual worker re-dispatch.

Injection surfaces (clean)

The new code is purely functional — no subprocess calls, no eval/exec, no file I/O, no shell commands. All API-sourced values are extracted via str(check.get(...) or "") coercions feeding a pure classifier. parse_timestamp uses datetime.fromisoformat() with a caught ValueError — timestamp strings from GitHub cannot cause injection. The snapshot's generated_at is set to datetime.now(UTC).isoformat() internally at pr_queue_snapshot.py L155, not from any user-supplied input.

UNSTABLE gate (clean)

All stuck classification is gated on merge_state == "UNSTABLE" at babysit_checks.py L265–266. The value derives from str(pr.get("mergeStateStatus") or "").upper() — a GitHub-computed field, not user-controlled. The UNSTABLE contract supplies the required/non-required split, preventing the classifier from touching required checks.

A check with a future-dated created_at (e.g. a clock-skewed external status) would produce a negative computed age, making aged_out = False — the check is silently ignored rather than falsely flagged. Safe failure mode.

Input validation (clean)

validated_stuck_check_age_seconds correctly rejects non-finite values, zero, and negative values via math.isfinite(seconds) and seconds > 0. This catches NaN, ±Inf, 0, and -0.0. Validation runs at pr_queue_snapshot.py L569 before build_config is called, so the float(... or DEFAULT) fallback in build_config that would silently accept 0.0 is unreachable with a zero input from CLI.

URL passthrough (pre-existing behavior — not a new risk)

target_url and details_url from the GitHub API pass through as strings into checks["stuck"] without URL validation. This is identical to the pre-existing treatment of those same fields in every other normalized check. These values appear only in the structured JSON snapshot consumed by the orchestrating agent; they are not interpolated into any natural language prompt, not rendered in HTML, and not passed to any subprocess.

orphaned_status boundary (clean)

check.get("type") == "StatusContext" and not check.get("target_url") fires for empty-string target_url (the normalized form of any absent/null targetUrl via the str(... or "") coercion in normalize_check). A space-padded value like " " is truthy in Python and correctly falls through to never_settling rather than orphaned_status. QUEUED CheckRuns without startedAt produce an empty created_at → parse_timestamp("") returns None → aged_out = False → silently unflagged. Fail toward silence, not false report.

plugin.json manifest (clean)

The new babysit_stuck_check_age_seconds userConfig field is typed "number" with a safe default of 1800. No new permissions, egress endpoints, or trust delegations are introduced.

Reference doc (clean)

stuck-checks.md contains no secrets, credentials, or PII. It explicitly states the never-auto-fix rule and correctly defers to safety.md for Stop-and-Ask / Never-Do-Automatically conditions rather than restating them.

GitHub Actions (clean)

No workflow files changed. No pull_request_target, no workflow_run, no github context interpolation in run: blocks, no permission-widening, no dependency pin changes.

---

Summary: No injection surfaces, no credential exposure, no merge-gate bypass, and no new persistence of sensitive data. The security-relevant invariant (stuck signal is report-only, never a gate input) is correctly implemented, explicitly tested, and consistent with the three independent security review passes already on record for this PR.

[kyle-sexton]

Operator basis recorded (in-session approval, 2026-07-21 ~08:4xZ): operator approved merging this area: security PR via explicit in-session confirmation to the control tower (same pattern as the #665 sign-off). Checks green, 0 unresolved threads, babysit re-confirmed CLEAN 5×. Merging under that recorded basis.

This was generated by AI (control tower, session 6).