Skip to content

fix(gh): classify pr checks by status column, not name substring#2796

Open
john-chen-opus wants to merge 1 commit into
rtk-ai:developfrom
john-chen-opus:fix/gh-pr-checks-bucket-classification
Open

fix(gh): classify pr checks by status column, not name substring#2796
john-chen-opus wants to merge 1 commit into
rtk-ai:developfrom
john-chen-opus:fix/gh-pr-checks-bucket-classification

Conversation

@john-chen-opus

Copy link
Copy Markdown

Problem

format_pr_checks (the rtk gh pr checks summarizer) classifies each row with a whole-line substring test:

if line.contains("[ok]") || line.contains("pass") { passed += 1 }
else if line.contains("[x]") || line.contains("fail") { failed += 1;}
else if line.contains('*') || line.contains("pending") { pending += 1 }

gh pr checks prints tab-separated rows — <name>\t<bucket>\t<elapsed>\t<url> — so matching the whole line has two correctness bugs:

  1. A check is classified by its NAME, not its status. A check named compass-audit or fail-fast smoke contains pass/fail as a substring and gets miscounted regardless of its real bucket.
  2. skipping and cancel(ed) checks are dropped entirely. They match none of the three branches, so they vanish from the summary — the printed counts silently disagree with the real PR. (On a PR whose only non-passing checks are change-gated skips, gh still exits 0, so this summary is what the agent sees.)

This bit me while shipping an unrelated change: the summary disagreed with the actual check set, and the empty-checks case surfaced as a confusing "0 passed" rather than gh's real message.

Fix

Classify on the tab-separated status column, not a whole-line substring:

  • Match the bucket via starts_with (absorbs skipping/skipped, cancel/canceled/cancelled).
  • Fold skipped and cancelled into the totals (cancelled surfaces with failures — gh's own rollup exits non-zero for it).
  • Lead with passed/total so the counts reconcile — skipped/pending checks are part of the total, never silently dropped.
  • When no row parses (e.g. no checks reported on the '<branch>' branch), return empty so the caller falls back to gh's raw output instead of inventing a misleading 0 passed summary.

This aligns with the design philosophy in CONTRIBUTING.md:

  • Correctness vs Token Savings — "when in doubt, preserve correctness";
  • Transparency — the filtered output stays a faithful subset (counts match the real command);
  • Never Block — the unparseable case falls back to raw rather than fabricating a summary.

Tests

Added three unit tests in gh_cmd.rs:

  • test_format_pr_checks_classifies_by_bucket_not_name — names containing pass/fail are classified by their status column;
  • test_format_pr_checks_reconciles_total_with_skips1 pass + 2 skips reports 1/3 with skips surfaced;
  • test_format_pr_checks_empty_falls_back_to_raw — empty / "no checks reported" → empty (raw fallback).

Local: cargo fmt --check clean, cargo clippy --bin rtk clean (only a pre-existing rusqlite future-incompat note), cargo test --bin rtk cmds::git::gh_cmd → 69 passed.

`format_pr_checks` matched pass/fail/pending as a substring anywhere in the
row, so a check whose NAME contains those words (e.g. "compass-audit",
"fail-fast smoke") was miscounted, and skipping/cancelled checks were dropped
entirely — the summary's counts silently disagreed with the real PR.

Classify on the tab-separated status column instead, fold skipped/cancelled
into the totals, and lead with `passed/total` so the counts reconcile. When no
row parses (e.g. "no checks reported on the '<branch>' branch"), return empty
so the caller falls back to gh's raw message instead of a misleading
"0 passed" summary. Adds unit tests for name-substring, skip reconciliation,
and the empty/fallback path.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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