Skip to content

feat: add github pull request monitor probe - #5183

Merged
bolichen97 merged 2 commits into
mainfrom
token-monitors-github-probe
Sep 2, 2026
Merged

feat: add github pull request monitor probe#5183
bolichen97 merged 2 commits into
mainfrom
token-monitors-github-probe

Conversation

@kyleseaman

@kyleseaman kyleseaman commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Stacked change: PR 4 of 8

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

Problem / Motivation

The system has no low-cost provider probe that can determine whether a GitHub pull request changed or became actionable without invoking an agent turn.

Why it matters

Review babysitting otherwise spends a model turn on every interval and may miss actionable blockers when unrelated checks remain pending.

What changed (motivation → approach → change)

  • Add an exact public github.com pull-request target and a hardened provider adapter using the shared GitHub runner.
  • Persist a bounded allowlisted canonical observation and stable fingerprint instead of raw provider payloads or logs.
  • Give known failed checks, requested changes, unresolved threads, merge conflicts, and behind branches precedence over unrelated pending or unknown facts, including incomplete review-thread pagination.
  • Add bounded review-thread pagination, typed provider errors, new-head handling, and dispatcher-free shadow execution.
  • Keep check-run and status-context namespaces distinct, retain every check-run row independently when GitHub exposes only display labels, and pass GraphQL string variables without typed coercion.

Tests

  • URL/identity parsing and hardened runner behavior
  • Canonicalization, actionable fingerprinting, mixed pending/actionable classification, and new-head transitions
  • Provider error taxonomy, pagination bounds, shadow persistence/refusal, and security posture

Manual verification

N/A — provider calls are exercised through deterministic fixtures; no live GitHub credentials are required.

Related Issues

N/A — implements the first provider described by #5180 and depends on #5182.

Checklist

  • Commit count satisfies the stacked PR hygiene limit
  • Existing tests pass and new tests cover the new behavior
  • Self-review completed; code follows project style guidelines
  • Documentation updated where applicable
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

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

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ca6fc25

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

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound provider slice with disciplined trust boundaries; its terminal three-strike error budget turns routine GitHub rate limiting into silent permanent monitor death.

Watch

  • Rate limiting is a routine, self-healing GitHub condition (60-min windows, shared with every other gh use on the host), but three consecutive RATE_LIMITED probes at the 300s default cadence (~15 min) hit consecutive_provider_errors + 1 >= budgets.max_provider_errorsSTOP_BLOCKED, terminally retiring the monitor — the PR later becomes actionable and no wake ever fires, which is exactly the "may miss actionable blockers" harm the PR exists to remove. There is no backoff: next_probe_at = now + staged.cadence_secs unconditionally.
  • This PR extends that same terminal streak to supplemental failures ("counted against the bounded provider-error streak") even when primary facts were read and persisted successfully — a persistently degraded secondary channel (e.g. GraphQL forbidden while pr view works) retires a monitor whose target remains fully readable, where the module's own fail-closed convention (record as durable incomplete/pending evidence, like the local caps already do) was available.

Suggestions

  • Back off next_probe_at on RATE_LIMITED instead of consuming the terminal streak, and let supplemental-only failures with persisted primary facts degrade to incomplete/pending evidence rather than count toward retirement.

[DESIGN-REVIEWED] ca6fc25

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I have the two candidates and the full implementation of github_pull_request.py (lines 190–997), the decision.py change, models.py, and shadow.py. Both candidates concern whether a supplemental read anomaly erases primary facts. Let me verify the exact reachability of each triggering input.

Candidate 1_checks catches _normalize_check's ValueError (line 453) and drops the whole rollup, so a failing CheckRun sharing a rollup with a rejected row would classify PENDING instead of ACTIONABLE. _normalize_check raises only on: an unknown __typename (the StatusCheckRollupContext union is CheckRun | StatusContext today, so this needs a future schema addition), a CheckRun with empty/null name (non-null in the schema), or a StatusContext with empty context (non-null in the schema). None of these coexists with a real failing check in a payload GitHub emits today — the input is a defensive branch the candidate itself could not show occurs. (a) fails the "occurs in practice" bar.

Candidate 2_review_threads re-raises on a returncode-0 non-JSON body (lines 493–496), propagating to probe's outer except (TypeError, ValueError, KeyError) (line 384) and returning a full PROVIDER_ERROR with response=None, erasing primary facts. The code plainly intends supplemental failures not to erase primary facts (that is the entire supplemental_error path, and _checks internalizes every exception). But the only trigger is gh api graphql exiting 0 with stdout that is not a JSON object. On HTTP 200 GraphQL always returns a JSON object; a MITM/captive-portal HTML body would fail TLS to api.github.com and exit nonzero (setting failure_kind, which routes to the supplemental return, not the raise). I cannot name a concrete input that reaches the raise with returncode 0 in practice. (a) fails.

Both candidates are grounded in code I opened, but neither establishes a concrete triggering input that occurs in practice — each rests on a payload GitHub does not emit or a gh exit condition that does not arise over HTTPS. Neither reaches confidence 80. No independently-grounded new finding survives Step 2 (the _checks path internalizes all its exceptions; the _review_threads re-raise is the only supplemental-erasing leak, and it is the same unreachable input).

No findings.

[OPUS-REVIEWED] ca6fc25

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

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

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ca6fc2523a2ac1c0cd247887235f01e77360afe5 — 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. Composing the review.

First-Principles-Verdict: CONCERNS

The probe's stated void — "no low-cost provider probe" — is already filled by pr_watch.py, and five persisted metrics fields plus a refuse-only flag ship with zero readers.

What this change ships

Intent: let review babysitting notice a PR became actionable without spending a model turn — an ADDITION (slice 4 of a spec-recorded RFC stack).

  1. GitHub PR can be probed for review-readiness without a model turn — justified by the recorded RFC plan, but second GitHub PR watcher
  2. Shadow runner persists probe outcomes under budgets — justified; zero consumers until the declared next slice
  3. A green observation with a new head now wakes once, then terminates — declared
  4. Supplemental (checks/threads) provider errors now retry in every category — declared
  5. Five new persisted MonitorState metrics fields — zero consumers
  6. New SETUP non-retryable provider-error kind — justified
  7. run_shadow_probe refuses a wake_delivery=True request — one accepted value, refuse-only surface
  8. Check-identity count/length bounds before persistence — justified
  9. Adapter registered as inbound (non-egress) redaction — justified
  10. Same-commit spec updates — mandated by AGENTS.md

Watch

  • The motivation "The system has no low-cost provider probe that can determine whether a GitHub pull request changed or became actionable without invoking an agent turn" is contradicted by src/kiro_crew/builtin_skills/kirocrew-dev/babysit/scripts/pr_watch.py, whose docstring says exactly that job: "a pure-watch tick costs no tokens at all... A terminal state (merged / closed) removes the job." The new adapter is meaningfully richer (review threads, typed errors, durable fingerprints), so not a bare second spelling — but the two already classify divergently (CANCELLED: noise there, failed here; known_reds baseline: there only), and nothing in the stack names which one retires. Grep statusCheckRollup under src/: 9 files, two of them independent PR-state classifiers.
  • run_shadow_probe and GitHubPullRequestProvider have 0 production consumers (grep: only shadow.py's own import and tests). Acceptable only because the stack declares feat: expose session monitors to agents #5184 next; the claim "review babysitting otherwise spends a model turn" stays untrue until that slice lands.

Subtractions

  • Drop the five write-only MonitorState fields probe_count, provider_error_count, last_probe_at, last_decision, last_provider_error (monitoring/models.py:220-224) — grep shows writers in shadow.py and validators in models.py, 0 readers anywhere; land them in the slice that displays them.
  • Drop the wake_delivery parameter and ShadowWakeDeliveryRefused (monitoring/shadow.py) — its only accepted value is False, and without the parameter wake delivery is equally impossible; add the parameter in the slice that implements delivery.
  • Drop GitHubPullRequestProbeResult.response (monitoring/github_pull_request.py:308) — shadow.py reads only canonical and observation; consumers outside tests: 0.

[FIRST-PRINCIPLES-REVIEWED] ca6fc25

@kyleseaman
kyleseaman force-pushed the token-monitors-github-probe branch from 9efece8 to 4ec26a0 Compare August 23, 2026 15:33
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 23, 2026
@kyleseaman
kyleseaman force-pushed the token-monitors-github-probe branch from 4ec26a0 to 2c166e5 Compare August 23, 2026 15:46
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 23, 2026
@kyleseaman
kyleseaman force-pushed the token-monitors-github-probe branch from 2c166e5 to 32e0d0f Compare August 23, 2026 15:55
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 23, 2026
@kyleseaman
kyleseaman force-pushed the token-monitors-github-probe branch from 32e0d0f to 11f9e80 Compare August 23, 2026 16:17
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 24, 2026
@kyleseaman
kyleseaman force-pushed the token-monitors-github-probe branch from 280f8ae to 6036439 Compare August 24, 2026 10:45
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 24, 2026
@kyleseaman
kyleseaman force-pushed the token-monitors-github-probe branch from 6036439 to a61fd1e Compare August 24, 2026 11:15
bolichen97
bolichen97 previously approved these changes Aug 27, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bolichen97
bolichen97 previously approved these changes Aug 28, 2026
bolichen97
bolichen97 previously approved these changes Aug 28, 2026
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Fixed the blocking persistence finding in 00d1ebbe8. The shadow probe now stages its decision and observation changes on a copy, awaits persistence, and publishes to the live MonitorState only after the write succeeds. A regression test proves a disk-write failure leaves the live fingerprint, decision, and metrics unchanged so the same terminal observation remains retryable.

Focused verification: 96 passed in test/test_github_pull_request_monitor.py; the composed top also passes 182 monitor tests, Linux-targeted mypy for monitoring/shadow.py, frontend TypeScript/build/i18n, and the bundle-size gate.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Fixed in aec92eecc: workflow-less check runs now remain independent even when a provider row carries an Actions-shaped details URL, so a same-label success cannot collapse a known failure into unknown. Added the forged-URL regression case; test/test_github_pull_request_monitor.py passes 96/96.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Fixed the blocking malformed-node finding in 777fde9c3. Review-thread parsing now marks malformed evidence incomplete but continues scanning the page, so later valid unresolved threads remain actionable. Added the null-before-blocker regression case. Focused verification: 97/97 GitHub provider tests and 176/176 composed pull-request monitor tests pass; black, flake8, and Linux-targeted mypy also pass.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Fixed the blocking same-label check-run finding in 6764625fd. Check rows now collapse across workflow dispatches only when every candidate has an exact GitHub Actions run identity and each belongs to a distinct dispatch; identity-less or duplicate same-dispatch rows remain independent, so a newer success cannot hide a known failure. Added the same-workflow/same-label regression. Focused verification: 201 pull-request monitor/controller tests pass; black, flake8, Linux-targeted mypy, and docs lint are clean.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Addressed the current review blocker: a failed supplemental review-thread request now preserves actionable primary evidence (failed checks, requested changes, unresolved threads, or merge blockers) while keeping thread completeness false. Provider errors still preserve the prior observation when no primary blocker is known. Focused verification: test/test_github_pull_request_monitor.py — 100 passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Addressed the current GPT review blocker: CheckRun rows now remain independent because GitHub’s rollup display labels do not identify a workflow file, so a same-label success cannot hide a failure from another workflow. Updated the monitor contract and regressions; the focused GitHub monitor suite passes (100 tests).

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Addressed the current design-review blocker-preservation finding. If a later review-thread page fails after an unresolved thread was already observed, the probe now retains that blocker as incomplete actionable evidence instead of returning a provider error. Added a red/green pagination regression, updated the spec, and corrected the PR description’s check-run identity claim; the focused GitHub monitor suite passes (101 tests).

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Addressed the current review concern about GitHub mergeStateStatus: BLOCKED. GitHub uses that umbrella state while required checks or reviews are still outstanding, so the monitor now keeps it pending unless a concrete failed check, requested change, unresolved thread, conflict, or behind-base condition identifies repair work. Added live-shaped regressions for BLOCKED with pending checks and required review; the focused GitHub monitor suite passes (103 tests), and the restacked provider-neutral suite passes as part of 167 focused tests.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Addressed the current duplicate-check blocker in a7f1b85: canonical check buckets now preserve repeated rows, so adding or removing an identically named failing context changes the fingerprint. The GitHub monitor suite passes (104 tests), and the provider-neutral implementation at the top of the stack carries the same multiplicity contract.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: GitHub PR probe (shadow mode)

Reviewed origin/main...origin/token-monitors-github-probe (10 files, +2439/−3) at e9eba3f5b. Every finding below was reproduced by executing the real GitHubPullRequestProvider / run_shadow_probe / decide_monitor against faked subprocess.CompletedProcess runners — not by reading. pytest test_github_pull_request_monitor.py test_monitor_decision.py test_monitor_persistence.py is 152 passed, so each item is uncovered behaviour rather than a broken build.

Headline: on the focus areas this PR names, the slice does not yet hold. Four separate paths retire or park the monitor on ordinary inputs, two paths erase a typed provider error into a fingerprint indistinguishable from a healthy probe, and the objective can be achieved but never reported.

Blocking

1. monitoring/decision.py:46STOP_SUCCESS is unreachable. Adding observation.head_changed or short-circuits the whole status switch, so a review-ready SUCCESS observation takes the wake branch; its fingerprint is committed to last_fingerprint, and the line-50 NO_CHANGE guard then permanently prevents STOP_SUCCESS. Executed on an approved/all-green/mergeable PR with a stale recorded head: probe 0: wake_actionable, probes 1–3: no_change, outcome=None throughout — the monitor achieves review_ready and never reports it, then polls to STOP_BUDGET. On origin/main the same observation returns STOP_SUCCESS. The same widening also makes PENDING wake: a draft or checks-pending PR with head_changed=True returns WAKE_ACTIONABLE where main returned RECORD_ONLY, so eight ordinary pushes exhaust DEFAULT_MONITOR_AGENT_TURNS. Reverting the line fails exactly 1 of 137 tests.

2. monitoring/shadow.py:71 — terminal decisions are computed and then discarded, and probes run before the budget gate. run_shadow_probe never writes outcome/stopped_reason/stopped_at, and unconditionally advances next_probe_at. Executed: merged PR → decision=STOP_SUCCESS but outcome=None, next_probe_at=1300.0; runtime budget spent → STOP_BUDGET, next_probe_at still advanced, probed again 100k seconds later, probe_count=2; four probes on HTTP 401 Bad credentialsSTOP_BLOCKED each time while consecutive_provider_errors climbed 1→4 past max_provider_errors=3, outcome still None. Because _terminal_decision keys only off state.outcome, the monitor is immortal. The gh spawns at shadow.py:61 happen before the budget check at :67, contradicting decide_monitor's own docstring ("a spent bound must never buy one additional unattended turn").

3. monitoring/github_pull_request.py:240 — the partial-error path is dead, and six tests pin a response shape gh never produces. gh api graphql exits non-zero whenever the body carries a non-empty errors array, so returncode == 0 implies errors is absent and _process_failure fires first. Executed with the same payload two ways: rc=1 (real gh) → response is None, canonical == {}, PROVIDER_ERROR/provider_transient — the observed unresolved thread and the entire successful primary read are discarded; rc=0 (what tests at test_github_pull_request_monitor.py:907/921/940/959/989/1109 fake) → actionable/unresolved_review_threads. Lines 240, 246-253 and 263 are unreachable, and the added spec claim in learn-cron-dashboard.md that "an observed unresolved thread still wins as actionable evidence" is false.

4. monitoring/github_pull_request.py:39 — the review-thread query omits isOutdated, so review_ready becomes permanently unreachable. Query text executed: nodes{isResolved}, isOutdated: ABSENT. The most common real flow — reviewer leaves inline comments, author pushes a fix, nobody clicks Resolve — leaves GitHub reporting isResolved:false, isOutdated:true. _classify_response:556 then returns ACTIONABLE/unresolved_review_threads on every probe, and _actionable_fingerprint_facts keeps the fingerprint stable-actionable, so the objective can never be met; with finding 1 it burns the 8-turn budget to STOP_BUDGET.

5. monitoring/github_pull_request.py:154 — a 403/404 on the supplemental GraphQL call discards a fully successful primary probe and retires the monitor on probe #1. AUTHORIZATION and NOT_FOUND are absent from _RETRYABLE_PROVIDER_ERRORS, and the known_blocker guard only covers PRs that already have a blocker. Executed: complete OPEN/APPROVED/MERGEABLE primary payload + second spawn returning gh: Resource not accessible by integration (HTTP 403) → first-probe decision=STOP_BLOCKED, last_provider_error=AUTHORIZATION, last_observation={}. gh api graphql needs broader token scope than gh pr view, so a token that reads PR metadata but 403s on GraphQL kills every monitor immediately with zero recorded observations.

6. monitoring/github_pull_request.py:31statusCheckRollup shares the gh pr view --json field set, against the repo's own explicit rule. dashboard/handlers/source_providers.py:2438-2442 states it verbatim: "gh pr view resolves a --json field set atomically: one unreadable field fails the whole read. statusCheckRollup needs Checks read access that fine-grained tokens commonly lack, so it must never share a field set with data the token IS authorized for (#5115) — every rollup consumer routes through this one isolated query instead of growing its own copy." pr_status.py:773 and pr_findings.py:180 carry parity copies. Executed: gh: Resource not accessible by personal access token (HTTP 403) → AUTHORIZATION → non-retryable → STOP_BLOCKED on probe #1, with zero lifecycle/review/mergeability facts recorded even though the token can read all of them.

7. monitoring/github_pull_request.py:383group_key is always None, so each CheckRun is keyed by row index and a superseded failure outvotes its green re-run. Executed with two rows sharing workflowName='CI', name='build' — older CANCELLED, newer SUCCESS: canonical['checks'] == {'failed': ['CI / build'], 'passed': ['CI / build'], ...}, status actionable/checks_failed, while GitHub's UI shows the PR green. The same identity lands in both buckets. Forcing recency to '', '�', 'ZZZZ' or '0000' leaves output byte-identical, and deleting the sort/latest_recency/latest_states block still passes 105/105 tests — the whole recency mechanism is unreachable. source_providers.py:2461-2462 documents the opposite invariant ("every consumer MUST collapse identically" or it will "resurrect a superseded CANCELLED failure") and _github_checks:1682-1705 calls this an already-fixed bug. Related: STALE (a real CheckConclusionState) is unmapped → unknown → PENDING/checks_unknown forever, while the sibling _github_check handles it.

8. monitoring/github_pull_request.py:373statusCheckRollup: null (a PR in a repo with no CI) is a malformed response on every probe. Executed: status=provider_error reason=provider_malformed_response canonical={}, GraphQL page never issued. Every sibling consumer defends against this exact shape — source_providers._github_rollup_read uses _as_list(...), pr_watch.py:476 uses ... or [] plus an isinstance check, pr_status.py:806/pr_findings.py:213 use ... or []. The adapter already tolerates gh's null for the sibling reviewDecision, so this is an inconsistency, not a deliberate fail-closed choice. TRANSIENT is retried, so three probes later _provider_error_decision returns STOP_BLOCKED on a healthy PR.

9. monitoring/github_pull_request.py:391 — duplicate same-context StatusContext rows collapse to unknown with no severity precedence, failing OPEN. Executed: rollup [{StatusContext, context:'ci/required', state:'FAILURE'}]checks={'failed': ['ci/required']}, actionable/checks_failed. Same rollup plus a SUCCESS row on the same context → checks={'unknown': ['ci/required']}, pending/checks_unknown — the failing required check disappears from checks['failed'] entirely. No test puts two same-context rows in one rollup, even though test_status_context_failure_cannot_be_hidden_by_same_named_check_run asserts this invariant for the CheckRun case.

Should fix

10. github_pull_request.py:155 — the known-blocker path erases the GraphQL provider error and zeroes the thread count, producing a fingerprint byte-identical to a healthy probe. Executed: open PR + one FAILURE check + graphql rc=1 HTTP 401provider_error=None, unresolved=0, complete=False, fingerprint 456d7d25…; the same PR with a healthy complete page yields the same 456d7d25… (_actionable_fingerprint_facts drops review_threads_complete and maps blocking_review='unknown' back to 'none'). Three run_shadow_probe calls: no_change, no_change, no_change with consecutive_provider_errors=0, provider_error_count=0, last_provider_error=None — a permanently broken credential is invisible forever and can never trip max_provider_errors, while durable last_observation['review_threads_complete'] silently flipped True→False.

11. github_pull_request.py:236 — a mid-traversal page failure with unresolved > 0 swallows the provider error, converting a terminal AUTHENTICATION failure into a normal ACTIONABLE observation. Executed: page1 ok (1 unresolved, hasNextPage), page2 rc=1 HTTP 401actionable/unresolved_review_threads, provider_error=None; decide_monitor resets consecutive_provider_errors to 0 and never reaches STOP_BLOCKED. Identical inputs with 0 unresolved on page 1 do surface provider_authentication. Meanwhile the errors-key path at 246-253 throws the partial count away and does return provider_malformed_response — two opposite policies for the same situation.

12. github_pull_request.py:520_actionable_fingerprint_facts drops unresolved_review_threads, draft, review_decision and review_threads_complete. Executed: 1 unresolved thread and 5 unresolved threads share fingerprint af894728…, so after the first wake a reviewer adding four more yields NO_CHANGE forever. Failed checks keep full multiplicity, so the treatment is inconsistent. Seven materially different states (approved+verified, traversal-incomplete, isDraft=true, REVIEW_REQUIRED, mergeStateStatus=BLOCKED, mergeable=UNKNOWN, extra passed/pending checks) all produced one fingerprint d199281a…. Sharpest case: draft is tested only at line 562, after the failed-check branch, so converting a PR to draft — an explicit "stop reviewing this" signal — leaves it ACTIONABLE with an unchanged fingerprint.

13. github_pull_request.py:167except OSError maps every spawn failure to ProviderErrorKind.SETUP, which is non-retryable. Executed: OSError(24, 'Too many open files'), ConnectionResetError(104, …) and BlockingIOError(11, …) each produced last_provider_error=setup, decision=stop_blocked on probe #1. subprocess.run raises exactly these for transient spawn/pipe conditions, and github_runner.py:729-734 also raises SetupError when the SEL invoked append fails (a transient disk event). Both contradict security.md/learn-cron-dashboard.md ("transport/provider failures are retryable"). Latent companion: _process_failure's reasons dict has no SETUP key — forcing SETUP raises KeyError, which line 169's broad except (TypeError, ValueError, KeyError) would mislabel as provider_malformed_response.

14. github_pull_request.py:595_classify_cli_error matches free-text substrings the monitored target partly controls. Executed against real gh stderr: 'GraphQL: Could not resolve to a PullRequest with the number of 999999.'transient (the marker list only catches the repository case), so a deleted or renamed PR is retried and then stopped as provider_transient, never reporting provider_not_found or reaching TARGET_UNAVAILABLE; SAML-enforcement denials → transient. Marker precedence compounds it: 'HTTP 503: unavailable (owner/authentication)'authentication and 'HTTP 502 bad gateway for acme/permission'authorization — both are legal repo names under _SEGMENT_RE, and gh echoes the slug in its error text, so monitoring such a repo converts every retryable 5xx into an immediate STOP_BLOCKED. Match on structured signals (HTTP <code>) instead.

15. github_pull_request.py:289 — the URL gate never checks parsed.params, and inspects already-mangled urlparse output. Executed: 'https://github.com/o/r/pull/12\r\n3' → ACCEPT as #123, and '…/pull/1\n23' → ACCEPT as #123 (CPython's WHATWG-conformant urlsplit deletes \t\r\n) — a line-wrapped paste of PR 12 silently monitors PR 123. '…/pull/123;touch' → ACCEPT #123, because urlparse moves ;touch into parsed.params, which the code never reads — while the sibling form '…/o/r;touch/pull/123' correctly raises, so the strictness is accidental. /pull/0123 and /pull/00000000123#123, and a 400-digit number is accepted and interpolated into both the gh pr view URL and -F number=. Net: several non-identical state.target strings alias to one identity, breaking per-target dedupe.

Also worth a follow-up (verified, cut for length)

A repeated endCursor is never checked for advancement, so a misbehaving provider inflates the durable count 10× and spends 11 spawns (executed: unresolved=10 for one real thread) · reviewDecision: null/""SUCCESS/review_ready, declaring a PR ready with zero human review · mergeStateStatus=BLOCKED/mergeable=UNKNOWN has no terminal path, and the sibling provider documents that a cold single read reports a conflicting PR as unblocked and mitigates with a bounded re-read this adapter lacks · state.target reaches the URL-only parser outside probe()'s try, so a bare ValueError escapes run_shadow_probe with nothing persisted — including for the identity form github.com/o/r#123 that canonical["target"] itself stores · nothing ever sets last_wake_fingerprint, so shadow emits WAKE_ACTIONABLE every probe forever and agent_turns never increments · head_revision bypasses redact() with no length bound (a 10 MB value is stored verbatim) while the new security_posture.py comment asserts a registered output boundary that does not exist in _REDACTION_SINKS · ANSI escapes, U+202E, zero-width and NUL survive _sanitize_check_identity, unlike the pipeline_fold._printable precedent · check count and identity length are uncapped (5,000 rows → a 3.6 MB persisted MonitorState), and attacker-authored failed-check names sit inside the "stable" ACTIONABLE fingerprint · run_shadow_probe has no lock, loses a committed transition on cancellation after persist, and raises OverflowError rather than ValueError for now=10**400 · dead code at lines 150 and 276 · _SEGMENT_RE duplicates github_runner.py:797 and _normalize_checks duplicates the source_providers._github_check* family with a divergent conclusion vocabulary · test_check_identity_is_redacted_before_it_enters_canonical_state asserts a needle absent from its own input, so a narrower URL regex would leak id=secret while the test passes.


Execution-verified AI-assisted review (Claude Code), run against a local checkout at e9eba3f5b. Nothing was modified. Findings are ordered by severity; each names the input that reproduces it, so please push back where a claim misreads intent. ARCC was not queried — search_arcc was unavailable in the session — so standard security-review practice plus the repo's own documented invariants were applied instead.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Addressed the current review on 573f644d4 and restacked #5184, #5185, #5186, and #5305 bottom-up.

The fixes cover the reproduced decision/shadow failures, terminal persistence and pre-probe budget gating, partial GraphQL responses and outdated threads, supplemental permission/error preservation, null and duplicate check states, bounded canonical/fingerprint completeness, transient OS classification, strict target parsing/sanitization, and the newly reported large-CI case: locally imposed check/review-thread caps now remain incomplete pending evidence without consuming the provider-error budget.

One review suggestion was intentionally not applied: distinct CheckRun rows still remain independent because GitHub’s rollup exposes display labels, not a workflow-file identity; collapsing same-label rows could hide a failure from another workflow. Duplicate StatusContext rows do fold by worst severity, and STALE CheckRun conclusions are non-blocking.

Focused verification:

The current linear heads are #5183 573f644d4#5184 124bb71e9#5185 17b8552bc#5186 bf3918797#5305 0169c018c.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Follow-up after rebasing the stack onto current main:

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Addressed the current Opus blocker in 573410c. Raised supplemental check or review-thread runner failures now remain typed supplemental errors and preserve readable primary blockers instead of replacing the observation with an empty provider error. Added timeout and setup-error regressions; the focused GitHub monitor suite passes 132/132 and the composed monitor suite passes 234/234.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on the strength of a full readiness audit of every open PR against main, not a
line-by-line reading of this diff — recording that plainly so the next reader knows what this
stamp does and does not cover.

Verified against this exact head SHA:

  • readiness: passed present, and PR Readiness — the one required status context on main
    (ruleset protected-branches) — is success on this head.
  • No check run on this head is failure, cancelled, timed_out or still in flight. Skipped
    jobs are path-filtered conditionals, none of them required.
  • mergeable: true, and the head is not far enough behind main for its green CI to describe a
    base that no longer exists.
  • No surviving reviewer CHANGES_REQUESTED: any such review is on an older commit and therefore
    already dismissed by dismiss_stale_reviews_on_push.
  • Every issue comment, inline review comment and review thread was read and classified. Nothing
    left is an unresolved human change request — the remainder is bot review-lane output, resolved
    or outdated threads, explicitly non-blocking suggestions, and author status notes.

Auto-merge (squash) is armed, so this lands once every other ruleset requirement is met.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants