feat: add github pull request monitor probe - #5183
Conversation
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of 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
Suggestions
[DESIGN-REVIEWED] ca6fc25 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI have the two candidates and the full implementation of Candidate 1 — Candidate 2 — 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 No findings. [OPUS-REVIEWED] ca6fc25 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence gathered. Composing the review. First-Principles-Verdict: CONCERNS The probe's stated void — "no low-cost provider probe" — is already filled by What this change shipsIntent: let review babysitting notice a PR became actionable without spending a model turn — an ADDITION (slice 4 of a spec-recorded RFC stack).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] ca6fc25 |
9efece8 to
4ec26a0
Compare
4ec26a0 to
2c166e5
Compare
2c166e5 to
32e0d0f
Compare
32e0d0f to
11f9e80
Compare
280f8ae to
6036439
Compare
6036439 to
a61fd1e
Compare
|
Fixed the blocking persistence finding in Focused verification: |
|
Fixed in |
|
Fixed the blocking malformed-node finding in |
|
Fixed the blocking same-label check-run finding in |
|
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: |
|
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). |
|
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). |
|
Addressed the current review concern about GitHub |
|
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
left a comment
There was a problem hiding this comment.
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:46 — STOP_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 credentials → STOP_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:31 — statusCheckRollup 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:383 — group_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:373 — statusCheckRollup: 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 401 → provider_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 401 → actionable/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:167 — except 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.
|
Addressed the current review on 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 |
|
Follow-up after rebasing the stack onto current
|
|
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
left a comment
There was a problem hiding this comment.
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: passedpresent, andPR Readiness— the one required status context onmain
(rulesetprotected-branches) — issuccesson this head.- No check run on this head is
failure,cancelled,timed_outor still in flight. Skipped
jobs are path-filtered conditionals, none of them required. mergeable: true, and the head is not far enough behindmainfor 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 bydismiss_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.
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)
Tests
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
Contribution License Agreement
N/A — the repository template does not yet supply final CLA wording.