source-control/babysit-prs: pr_queue_snapshot.py silently drops a PR from the listing (and the N-PR(s) count) when its reviews-fetch call errors, instead of degrading gracefully

[kyle-sexton]

lane: babysit

Triggering example: cycle 2 of a 2026-07-19 babysit session. First snapshot this cycle returned 8 PRs (#377, #489, #561, #563, #564, #567, #568, #569). After dispatching 5 fix workers, I re-ran the snapshot to close out the cycle:

python pr_queue_snapshot.py --queue --author @me --owners kyle-sexton,melodic-software --state-dir <dir> --write-state

Output: Babysit PR snapshot: 4 PR(s), cadence=active, with an Errors: block naming 4 PRs (#377, #489, #563, #567) whose gh api repos/.../pulls/<N>/reviews?per_page=100 --paginate --slurp call hit a transient GitHub 503. All 4 of those PRs are then completely absent from the listing below — not shown with degraded/partial data, not counted in the N PR(s) total, just gone. I confirmed independently via gh pr list --state open --author kyle-sexton that all 8 PRs were still open at the time; #489/#563/#567 had just been pushed to by my own workers minutes earlier, so they were very much live and actionable.

Observed vs expected: observed — a transient per-PR API error on one field (the reviews list) silently removes the entire PR from the queue snapshot's output and count, with only a terse top-of-output Errors: line as the only trace, easy to miss if you don't count PRs by hand against gh pr list. Expected — a PR whose reviews-fetch errors should still appear in the listing (using whatever other data succeeded, e.g. from the earlier successful fetch this same cycle), flagged as degraded/partial-data with the specific missing field noted, so an operator/orchestrator relying on the snapshot's PR count and body doesn't silently lose visibility into an actionable PR because of an unrelated, transient upstream 503. In an unattended loop with less careful cross-checking than this cycle had, this could mean a PR sits ignored indefinitely purely because it happened to hit a 503 on the reviews call at snapshot time.

Category: bug (silent data loss / incomplete degrade-on-error behavior in pr_queue_snapshot.py).

[kyle-sexton]

Additional data point, same session, next cycle (~1hr later): the errored-PR count grew from 4/8 to 5/8 (#377, #489, #567, #568, #569 now all erroring on the reviews-fetch call), and the script's own output now shows a line I hadn't seen before: Quarantined (persistently erroring, no longer forcing cadence): #489, #567 — so there IS a quarantine mechanism for PRs that error repeatedly across snapshots. That's a reasonable design (avoids one flaky PR blocking cadence forever), but it doesn't change the core bug: quarantined AND freshly-erroring PRs are still completely absent from the listing body and the N-PR(s) count, with only the terse top-of-output Errors:/Quarantined: lines as any trace. A quarantined PR should still show up in the body (using last-known-good data, explicitly marked stale/quarantined) rather than vanishing — 'no longer forcing cadence' should mean 'stop escalating on it', not 'stop showing it at all'.

Also independently corroborating: this cycle also saw a straight gh run view call 503, and one gh api comment-PATCH call 503'd on first try (succeeded on retry) — so this looks like broad GitHub API instability, not something specific to the reviews endpoint. Worth keeping the fix generic to 'any per-PR enrichment call can fail' rather than special-casing the reviews fetch.

[kyle-sexton]

This was generated by AI during triage.

Triage plan: graceful degradation for per-PR enrichment failures

Where the drop happens

plugins/source-control/skills/babysit-prs/scripts/pr_queue_snapshot.py, build_snapshot(), the per-target loop (currently ~L185–L235). Every per-PR enrichment call is inside one try, and the except appends to errors/pr_error_keys and skips the prs.append(...) entirely:

for repo, number in targets:
    key = f"{repo}#{number}"
    try:
        pr = gh.view_pr(repo, number)
        pr["comments"] = gh.fetch_issue_comments(repo, number)
        pr["reviews"] = gh.fetch_pull_request_reviews(repo, number)   # 503 observed here
        inline_comments = gh.fetch_unresolved_review_comments(repo, number)
        ...  # checks, review_gate_state, fetch_review_evidence, fetch_reaction_signals
        prs.append(delta.classify_pr(...))
    except Exception as exc:
        errors.append(f"{key}: {exc}")   # PR now absent from prs -> absent from listing AND pr_count
        pr_error_keys.append(key)

So the PR vanishes from prs (the listing body) and from pr_count = len(prs) (L286). The quarantine mechanism (babysit_state.update_error_quarantine, threshold 3 cycles, 24h TTL) only stops the errored key from forcing cadence to active; it does nothing to keep the PR visible. Confirmed the follow-up comment's point: the failure is any of ~6 enrichment calls (view_pr, fetch_issue_comments, fetch_pull_request_reviews, fetch_unresolved_review_comments, fetch_review_evidence, fetch_reaction_signals) — not reviews-specific — so the fix must be generic per-call, not endpoint-special-cased.

What last-known-good data already exists

babysit_state.py persists a per-PR projection in state["prs"][key] via persisted_pr_state() (L193–231), reloaded each cycle as previous_prs and merged into previous via previous_with_ledger() before classify_pr. It carries: head_sha, updated_at, is_draft, classification, author, author_type, merge_state, mergeable, human_stop, head_ref_uniqueness, mutation_policy, branch_freshness, branch_refresh*, review_trigger, blocker_count, material_count, feedback id lists, checks_failing/pending(+identities), worker-checkin fields, and recorded_at.

Gap: it does not persist the display/shape fields a listing row needs — title, url, head_ref, base_ref, base_sha, and the full checks/feedback/blockers/material_findings/new_feedback/needs_worker* structures classify_pr returns (only counts and ids are persisted). So a row for a PR whose view_pr itself failed cannot be faithfully rebuilt from state today without (a) persisting a few more display fields and (b) synthesizing placeholders for the derived structures.

Proposed design (root-cause, generic)

1. Never drop — split the enrichment into individually-guarded steps. Replace the single try with per-call guards that record a failed field name and fall back instead of aborting the PR. Two failure shapes:

• view_pr fails (no fresh PR object): emit a degraded row synthesized from the persisted projection (last-known-good), marked stale with data_as_of = previous.recorded_at. If no prior record exists (cold/never-classified), emit a minimal placeholder row (key + failed field) so it still appears in the listing and count.
• A later enrichment fails (fresh pr present, one dimension missing): still call classify_pr, but pass the missing dimension as its last-known-good analog where one exists, else a safe empty default, and tag the result degraded for that field.

2. Represent degradation honestly — add a degraded block to the PR dict, e.g. {"degraded": true, "partial_fields": ["reviews"], "stale": true, "data_as_of": "<recorded_at>"}, and keep the entry in the same shape classify_pr returns so print_text/--json/save_state consume it unchanged. Add one degraded line to print_text per PR.

3. Force a non-acting posture while degraded. The skill dispatches workers only when needs_worker is true and reads classification/material_findings (SKILL.md L380–381; reference/orchestration.md). A degraded entry must set needs_worker = false (never dispatch on unknown/stale data) and must not be merge-eligible, so graceful degradation cannot cause a wrong auto-action in worker/autopilot tiers. The merge tier is independently gate-proven by babysit_merge.py, which is the second safety layer.

4. Do not corrupt last-known-good in save_state. Critical: save_state (babysit_state.py L454–457) calls persisted_pr_state(pr) for every snapshot_prs entry and stamps a fresh recorded_at. For a degraded PR this would overwrite good state with partial data and reset staleness. Fix: in the snapshot_prs write loop, preserve the existing current_prs[key] record for any degraded PR (skip overwrite) so the cross-cycle last-known-good survives until a clean cycle refreshes it. Keeping the snapshot complete=false while any PR is degraded (already the effect, since the error stays in errors and substantive_errors is non-empty) also prevents the complete-sweep branch from deleting the degraded PR's stale record.

5. Leave quarantine/exit-code machinery intact. Still append to errors and pr_error_keys (feeds update_error_quarantine and exit code 1). The only change is that the PR is also in prs. A quarantined PR then naturally shows a degraded row from persisted state — realizing the comment's "stop escalating cadence, not stop showing it."

Files to touch

• pr_queue_snapshot.py — per-call guards + degraded-row synthesis; print_text degraded line.
• babysit_state.py — persist title/url/head_ref/base_ref(/base_sha) in persisted_pr_state; preserve-not-overwrite for degraded entries in save_state.
• babysit_delta.py — a helper to build a full-shaped degraded entry from a persisted projection (or classify_pr accepts a degraded marker), keeping one canonical output shape.
• Docs: SKILL.md decision step + reference/orchestration.md/cadence.md to document the degraded field and that degraded PRs are visible-but-non-acting.
• Tests: tests/test_pr_queue_snapshot.py, tests/test_babysit_state.py, tests/test_babysit_delta.py, tests/test_integration.py — new cases: enrichment-call raises → PR still in listing/count and flagged degraded, needs_worker=false, last-known-good preserved across the degraded cycle, quarantined PR still rendered.

Open decisions flagged for a human (why this is NOT CONFIDENT)

The mechanical "never drop, keep in listing + count" is unambiguous. What is not safely decidable without a human call is the stale-data representation contract, because this snapshot feeds an auto-merge-capable system:

1. Snapshot schema addition. A degraded marker is a contract change every consumer (SKILL.md decision step, orchestration/merge tiers) must honor. Reconstructing a full classify_pr-shaped row from a lossy persisted projection means feedback/blockers/material_findings become empty-with-a-note — which a consumer reading those fields without checking the degraded flag would misread as "clean." Forcing needs_worker=false mitigates worker dispatch, but the exact contract for classification/material_findings on a degraded entry (reuse persisted value vs. a new degraded classification) needs a deliberate design decision.
2. Extra persisted fields. Adding title/url/head_ref/etc. to persisted_pr_state is a state-schema change under STATE_SCHEMA_VERSION (currently 1). Whether to bump the version / handle old files needs a call.
3. Partial vs. whole staleness. When only one dimension failed (fresh head, stale reviews), whether to persist the fresh fields mixed with stale derived signals, or hold the whole record stale, affects next-cycle delta detection — a correctness tradeoff worth an explicit choice.

Labeling needs-human for the schema/representation decisions above; the drop itself is a real bug and the fix location is confirmed.

[kyle-sexton]

This is a 2-minute decision brief. The "never drop a PR from the listing/count" fix is already settled in the triage plan above; this brief only asks for a human call on the three stale-data representation/schema decisions that triage flagged as needs-human.

Problem

Once a degraded PR stays in the listing, we have to decide how to represent stale/partial data in a snapshot that feeds an auto-merge-capable system (#476, autopilot merge tier — OPEN). The three open decisions — the degraded contract, whether adding persisted display fields bumps STATE_SCHEMA_VERSION, and partial-vs-whole staleness — each trade contract-blast-radius, migration cost, and delta-detection correctness against each other, and they interact, so they want one coherent call rather than three isolated ones.

Today (verified against repo HEAD)

Paths: plugins/source-control/skills/babysit-prs/scripts/{pr_queue_snapshot.py,babysit_state.py,babysit_merge.py}.

• Schema version: STATE_SCHEMA_VERSION = 1 (babysit_state.py:35). The loader hard-fails on any mismatch — RuntimeError(... "migrate or move the file before rerunning") (babysit_state.py:104-108). There is no lazy-migration path today.
• What's persisted per PR: persisted_pr_state() (babysit_state.py:193-231) carries head_sha, updated_at, is_draft, classification, author/author_type, merge_state, mergeable, human_stop, head_ref_uniqueness, mutation_policy, branch_freshness, branch_refresh*, review_trigger, blocker_count/material_count (counts only), feedback id lists, checks_failing/pending(+identities), worker-checkin fields; recorded_at is stamped in save_state (:456). It does not persist title/url/head_ref/base_ref/base_sha, nor the full feedback/blockers/material_findings structures — confirmed gap. So a degraded row for a PR whose view_pr itself failed cannot be faithfully rebuilt from state without adding display fields.
• The drop: one try wraps all ~6 enrichment calls; the except appends to errors/pr_error_keys and skips prs.append (pr_queue_snapshot.py:185-235), so the PR vanishes from prs and from pr_count = len(prs) (:285).
• Overwrite hazard: save_state re-stamps persisted_pr_state(pr) with a fresh recorded_at for every snapshot entry (:454-457) — the triage plan's "preserve-not-overwrite for degraded entries" targets exactly this.
• Consumers of the contract: the skill's decision step reads classification / needs_worker / material_findings (SKILL.md:380-381); the merge tier is independently gate-proven by babysit_merge.py (second safety layer). Both would have to honor any new degraded marker.

Drift check: the triage plan's citations are accurate; only trivial off-by-ones (pr_count is :285 not :286). No design-relevant drift.

---

The decision: pick one coherent bundle

Each option answers all three flagged decisions together (1 = degraded contract, 2 = schema bump, 3 = staleness granularity).

✅ Option A — Additive degraded block + version bump with lazy migration + whole-record staleness — RECOMMENDED

1. Contract: add a separate additive degraded block ({degraded, partial_fields, stale, data_as_of}); reuse the persisted classification for the row (display + delta continuity) and rebuild material_findings/blockers/feedback as empty-with-note. The degraded block — not classification — is the authoritative action gate: it forces needs_worker = false and merge-ineligibility. Do not add a degraded value to the classification enum (that would force every switch-on-classification site to grow a branch).
2. Schema: bump STATE_SCHEMA_VERSION 1→2 and convert the loader (:104-108) from hard-fail to lazy-migrate-and-backfill (add empty display fields, rewrite version). Chosen for the safety fence: the version gate makes a rolled-back or concurrent old engine refuse to operate on a new-schema file rather than silently read new degraded records it doesn't understand and misread stale-as-fresh — in an auto-merge-capable fleet (feat(source-control): babysit autopilot merge tier — bot-account review + criteria-gated merge #476), "refuse on unknown schema" is the safer failure mode, and it's the same stale-data→wrong-action concern this issue is about.
3. Staleness: hold the whole record stale when any dimension fails (preserve-not-overwrite at :454-457, data_as_of = previous.recorded_at). A persisted record then always stays internally consistent — all fields from one successful cycle — so next-cycle delta detection can't be fooled by a fresh head_sha paired with stale review-derived signals reading as a spurious re-trigger.

Closest call inside A is decision 2 (see toss-up note below); it's a real toss-up, so A carries a discriminating question for you rather than a silent choice. Reversibility: high on 1 and 3 (additive block / behavioral); moderate on 2 (once you own a migration path, later schema evolution is cheaper, but the bump itself is a one-way step for existing files).

Option B — same 1 and 3, but no version bump

Add the display fields as purely-additive optional keys, read via .get(default); when absent (pre-upgrade file, or a PR whose view_pr never once succeeded), fall back to a minimal placeholder row. Pro: zero migration code; additive optional fields are backward/forward compatible. Con: you lose the safety fence — an old or concurrent engine silently reads a file containing degraded records and, not knowing the block, can treat a stale record as fresh last-known-good and act on it. Defensible if you judge the fleet is effectively single-version and value avoiding migration code over the fence. Reversibility: high.

Option C — richer / dedicated degraded classification + partial (per-field) staleness

Add a degraded value to the classification enum and persist the heavier feedback/blockers/checks structures so degraded rows are faithful (not empty-with-note); mark only the failed dimension stale. Pro: most explicit for a consumer that reads only classification; precise next-cycle deltas on fresh dimensions. Con: widest blast radius — every switch-on-classification site (SKILL.md:380-381, orchestration, merge tier) must add a branch or risk fallthrough; per-field staleness reintroduces the mixed-freshness delta hazard A avoids; heavier schema. Reversibility: low (enum values + a bigger persisted shape are sticky).

Option D — minimal placeholder only (smallest change)

Never rebuild from state; a degraded PR always shows a bare placeholder row (key + failed field + degraded), no last-known-good body. No new fields, no schema change. Con: under-delivers on the issue's explicit ask — the follow-up comment wants a quarantined PR to show last-known-good data, not just reappear in the count. Fixes visibility/count only, not usefulness.

---

Decision 2 is the toss-up — the question for you

Not "bump breaks files" (nobody proposes bumping without a migration — that's a strawman). The honest cost of bumping is "you must build the lazy-migrate loader path that doesn't exist today," and how heavy that is hinges on one thing:

• The PR projection is regenerable — a cold/corrupt restart self-heals ("every PR re-classifies as new"), costing ~one transitional cycle. If that's all you care about, the bump can migrate via cheap cold-restart and you get the safety fence nearly for free.
• The mutation_ledger (seen_human_feedback_ids, review-request history) is not re-derivable — a cold restart risks re-requesting reviews / re-surfacing already-handled feedback. If you value ledger continuity, the bump needs a real backfill migration (heavier).

So the one question that breaks the tie: how much do you value mutation_ledger continuity across the upgrade? High → A with a real backfill migration. Low → A via cold-restart (cheap, keep the fence). Prefer to skip migration entirely and accept the silent-old-engine risk → drop to Option B (clean substitution; decisions 1 and 3 are unchanged).

I recommend A because this snapshot drives #476's auto-merge tier, and the whole point of the issue is that stale/degraded data must never silently drive a wrong action — the schema-version fence enforces exactly that at the state boundary, complementing the needs_worker=false / merge-ineligible gate at the record level.

Generated by an AI brief-writer subagent (decision-briefing lane). Code citations verified against repo HEAD; no code, labels, or state changed.

[kyle-sexton]

This was generated by AI (decide lane — brief freshness supplement).

Freshness supplement — the load-bearing fact under the brief changed

This supplements, does not replace, the decision brief above. One premise it rests on has moved; the recommendation is re-derived against the new reality below.

What changed

The brief frames the whole decision as being about a snapshot that "feeds an auto-merge-capable system (#476, autopilot merge tier — OPEN)." That is now stale:

• #476 is CLOSED / COMPLETED (closed 2026-07-20). The prerequisite chain the maintainer decision staged (source-control:babysit-prs: gh-bot.sh bot token generation fails, worker writes fall back to personal identity #450, safety trio babysit-prs: merge-step no-monitor STOP, bare-wrapper pipe rule, snapshot exit-code taxonomy, cross-PR dependency channel #373/fix(source-control): review-trigger head-staleness hardening (BLOCKED-behind guard, stale-head reactions) #327/fix(source-control): require per-thread pins for autonomous bulk babysit thread resolves #326, shared classifier source-control:babysit-prs: one shared authorship/finding/approval classifier + golden fixtures; gate script owns the merge-ready boolean (6-issue class) #534) is exhausted and the autopilot merge-tier implementation shipped ("Closes feat(source-control): babysit autopilot merge tier — bot-account review + criteria-gated merge #476").
• Precisely: the tier is built and merged; activation is imminent, not yet running. The actual gate-off flip is a separate, loudly-announced step still contingent on an open §3-wiring precondition (per feat(source-control): babysit autopilot merge tier — bot-account review + criteria-gated merge #476's control-tower comments). Either way, feat(source-control): babysit autopilot merge tier — bot-account review + criteria-gated merge #476 is no longer a pending proposal — it is real capability, one flip from live.

So the brief's stakes assumption ("this snapshot might someday feed auto-merge") is now understated: it feeds a merge tier that is built and about to activate.

Re-derived recommendation

RECOMMENDED: unchanged — Option A (additive degraded block + version bump with lazy migration + whole-record staleness). The changed fact reinforces Option A; it does not reverse it. Rationale:

1. Live/imminent auto-merge makes graceful-loud degradation MORE important, not less. Under human-GATE-ON, a silently-dropped or misrepresented-as-clean PR meant "an operator might miss it." Under an active merge tier, the same failure can drive a wrong auto-action — or, in an unattended loop, permanent loss of visibility on an actionable PR with no human watching. The never-drop / honestly-degraded / force-needs_worker=false + merge-ineligible posture is exactly the mitigation that failure mode now requires.

2. This is the same failure family feat(source-control): babysit autopilot merge tier — bot-account review + criteria-gated merge #476 is already hardening its gate against. feat(source-control): babysit autopilot merge tier — bot-account review + criteria-gated merge #476's own flip preconditions — babysit classifier: count_classified counts stale pipe-row classifications from non-thread review-summary comments #642 (count_classified fail-open in the readiness gate) and the decision-default-veto criterion — are both "stale/ambiguous data must fail closed" fixes. source-control/babysit-prs: pr_queue_snapshot.py silently drops a PR from the listing (and the N-PR(s) count) when its reviews-fetch call errors, instead of degrading gracefully #574's degraded-data posture is a member of that same family, not a hypothetical. It is now part of live merge-gate safety.

3. One sharpened requirement (the genuine re-derivation). In the original brief, "babysit_merge.py independently gate-proves merge / the degraded block forces merge-ineligibility" was a design property. With the tier built and activation imminent, it becomes load-bearing: babysit_merge.py MUST explicitly treat any degraded/stale entry as merge-ineligible (fail-closed) — not rely solely on needs_worker=false upstream. This should be listed alongside babysit classifier: count_classified counts stale pipe-row classifications from non-thread review-summary comments #642 and the decision-default veto as a merge-gate fail-closed criterion.

Do NOT switch to Option C on account of this. The added risk (an automated consumer reading classification/material_findings and misreading empty-with-note as clean) is correctly answered by making the gate fail-closed on the degraded flag (point 3), not by faithfully rebuilding data — stale-but-faithful data still must not auto-merge. Option C's blast radius (every switch-on-classification site) and its mixed-freshness delta hazard remain unjustified.

The three sub-decisions themselves (contract shape, schema bump, staleness granularity) are not re-litigated here — only the premise feeding them, and it points the same way, harder.

Cross-link: #476.

[kyle-sexton]

Frontier disposition (drain session)

Ruling: promoted to agent-ready. pr_queue_snapshot.py must degrade gracefully when a per-PR reviews fetch errors: keep the PR in the listing and count, mark its review state unknown, and surface the error inline. Scoped error-handling change with a reproducible trigger already documented.