Skip to content

chore: supervisor: PR-approval Telegram ping fires before all CI checks exist (check-run snapshot race) - #688

Merged
getappz merged 2 commits into
masterfrom
task/259-supervisor-pr-approval-telegram-ping-fir
Sep 16, 2026
Merged

getappz merged 2 commits into
masterfrom
task/259-supervisor-pr-approval-telegram-ping-fir

Conversation

@getappz

@getappz getappz commented Sep 16, 2026 •

Copy link
Copy Markdown
Owner

Status

Fixed the false-positive-early-ping bug in decide_from_checks (src/worktree/mod.rs:350), the shared decision function for both call sites (pr_ci_status_impl's REST path and pr_ci_status_from_batch's GraphQL batch path).

Change: decide_from_checks now takes an additional mergeable_state: Option<&str> param. When the fetched check-runs all look green (0 pending, 0 failed) but GitHub's own mergeable_state is "blocked", it now returns PrCiStatus::Pending instead of Passing — since "blocked" means GitHub's required-checks/branch-protection view still has outstanding items the sweep's check-run snapshot hasn't picked up yet (e.g. gated build matrix jobs not yet created). Both call sites already had mergeable_state in hand (used for the existing Behind check), so no new API calls needed.

Tests: Added pr_ci_status_from_batch_reports_pending_when_blocked_despite_green_checks, mirroring the live repro (green checks + mergeable_state: "blocked" → Pending, not Passing). All 8 tests in worktree::tests pass, including the pre-existing ones (clean mergeable_state still reports Passing/Failing/Pending as before).

Verification run synchronously:

  • cargo build --lib — clean
  • cargo test --bin agentflare pr_ci_status — 8/8 pass
  • cargo fmt --check -- src/worktree/mod.rs — clean except one pre-existing, unrelated diff (import ordering at line 51) confirmed present on the base branch before my changes (verified via stash)
  • cargo clippy --bin agentflare --all-targets — no new warnings (only pre-existing unrelated unsafe-block warnings elsewhere)

Concerns: None blocking. One judgment call worth flagging: I scoped the cross-check to mergeable_state == "blocked" only, per the issue's repro. Other transient states like "unknown" (GitHub still computing mergeability) or "dirty" weren't touched — the issue didn't report those as problematic and expanding scope there felt speculative rather than evidence-driven.


Opened by claude-code on flared:51bb8de6c33b for item #259 via agentflare.

Summary by CodeRabbit

  • Bug Fixes
    • Improved CI status reporting for pull requests marked as blocked, even when all visible checks are green.
    • Blocked pull requests now remain pending until their merge status is resolved, avoiding premature “Passing” results.
    • Applied consistent status evaluation across individual and batch checks.

Fixed the false-positive-early-ping bug in `decide_from_checks` (`src/worktree/mod.rs:350`), the shared decision function for both call sites (`pr_ci_status_impl`'s REST path and `pr_ci_status_from_batch`'s GraphQL batch path).

**Change:** `decide_from_checks` now takes an additional `mergeable_state: Option<&str>` param. When the fetched check-runs all look green (0 pending, 0 failed) but GitHub's own `mergeable_state` is `"blocked"`, it now returns `PrCiStatus::Pending` instead of `Passing` — since "blocked" means GitHub's required-checks/branch-protection view still has outstanding items the sweep's check-run snapshot hasn't picked up yet (e.g. gated `build` matrix jobs not yet created). Both call sites already had `mergeable_state` in hand (used for the existing `Behind` check), so no new API calls needed.

**Tests:** Added `pr_ci_status_from_batch_reports_pending_when_blocked_despite_green_checks`, mirroring the live repro (green checks + `mergeable_state: "blocked"` → `Pending`, not `Passing`). All 8 tests in `worktree::tests` pass, including the pre-existing ones (`clean` mergeable_state still reports `Passing`/`Failing`/`Pending` as before).

**Verification run synchronously:**
- `cargo build --lib` — clean
- `cargo test --bin agentflare pr_ci_status` — 8/8 pass
- `cargo fmt --check -- src/worktree/mod.rs` — clean except one pre-existing, unrelated diff (import ordering at line 51) confirmed present on the base branch before my changes (verified via stash)
- `cargo clippy --bin agentflare --all-targets` — no new warnings (only pre-existing unrelated `unsafe`-block warnings elsewhere)

**Concerns:** None blocking. One judgment call worth flagging: I scoped the cross-check to `mergeable_state == "blocked"` only, per the issue's repro. Other transient states like `"unknown"` (GitHub still computing mergeability) or `"dirty"` weren't touched — the issue didn't report those as problematic and expanding scope there felt speculative rather than evidence-driven.

Agentflare-Branch: task/259-supervisor-pr-approval-telegram-ping-fir
Agentflare-Item: 259
@coderabbitai

coderabbitai Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Essentials

Run ID: cad4ebac-49fb-48a1-8091-0da7923076fa

📥 Commits

Reviewing files that changed from the base of the PR and between d632bdc and 412b258.

📒 Files selected for processing (1)
  • src/worktree/mod.rs

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.


📝 Walkthrough

Walkthrough

CI status evaluation now passes GitHub’s mergeable_state through per-PR and batch paths. Blocked pull requests with green checks return Pending. A regression test verifies this behavior.

Changes

CI status evaluation

Layer / File(s) Summary
Mergeable-state decision logic
src/worktree/mod.rs
decide_from_checks accepts an optional mergeable state. It returns Pending for "blocked" state after failed checks are excluded.
Status-path wiring and regression coverage
src/worktree/mod.rs
Per-PR and batch evaluations pass mergeable_state to the shared decision function. A regression test covers blocked pull requests with green checks.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 412b2

No merge-blocking risk is established for the reported CI-status correction.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: preventing premature Telegram approval pings caused by incomplete CI check-run snapshots. It is specific and related, although longer than necessary.
Description check ✅ Passed The description explains the change, motivation, affected paths, test coverage, verification results, and edge-case considerations. It does not use the template headings or explicitly address backward…
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch task/259-supervisor-pr-approval-telegram-ping-fir

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.


Comment @coderabbitai help to get the list of available commands.

@getappz

getappz commented Sep 16, 2026

Copy link
Copy Markdown
Owner Author

Tracking this PR for automated review (flared:c997d745ae66).

@getappz
getappz merged commit 4554d93 into master Sep 16, 2026
17 checks passed
@getappz
getappz deleted the task/259-supervisor-pr-approval-telegram-ping-fir branch September 16, 2026 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant