Create the GitHub Check as in progress before benchmarks run

[epompeii]

Summary

bencher run --github-actions currently creates the Bencher Report GitHub Check only after the benchmark completes, always with a terminal conclusion. On a rerun, the stale conclusion from the previous run lingers until the new benchmark finishes, which can take hours for slow benchmarks, and a cancelled job leaves the stale conclusion forever.

This PR creates the check with an in_progress status before the benchmark command runs and then completes that same check run with the results once they finish:

• GitHubActions::start_check creates the in progress check before the benchmark runs (best effort: failures log a warning and never abort the run)
• The check run ID is carried in a CheckRunHandle and threaded down to the existing CI completion step, which now updates the check run in place instead of creating a new one
• If the in progress check could not be created, completion falls back to the previous behavior of creating the check with a conclusion
• If bencher run exits with an error before the results are posted (harness failure without --allow-failure, report send failure, remote job poll error or timeout), the check is completed as failure with a short summary instead of lingering in progress
• --dry-run never creates a check, same as before
• The handle is consumed with Option::take at the completion site, so the error path never overwrites a check that was already completed with the full report (for example the --error-on-alert path)

Notes

• A hard kill (CI job cancellation, SIGKILL) still leaves the check in progress, since no code gets to run. That is strictly better than showing a wrong terminal conclusion, and a signal handler could be a follow up.
• The --detach path keeps its existing semantics: the check is completed at submit time with the empty results report; it is just more visible now because the in progress check appears first.
• octocrab's retry feature transparently retries 5xx responses on the in progress create, so transient GitHub hiccups do not even need the fallback path.

Testing

• New unit tests for head SHA resolution, conclusion mapping, and check run output shape
• Verified end to end against a local API server and a request recording GitHub API stub across four scenarios: successful run (POST in_progress lands before the benchmark command executes, then PATCH with conclusion: success and the report summary), failing benchmark (PATCH with conclusion: failure), --dry-run (zero GitHub requests), and a rejected in progress create (falls back to POST with a conclusion)
• cargo nextest run -p bencher_cli, cargo clippy --all-targets --all-features -- -Dwarnings, cargo check --no-default-features, and cargo fmt are all clean

Docs

• Documented the new behavior in the --github-actions section for all nine languages
• Added a changelog entry under Pending

[github-actions]

🤖 Claude Code Review

PR: #950
Base: devel
Head: claude/github-check-in-progress
Commit: 467986b55e9af5894539fece53b8dca037b9ebde

---

I've completed a thorough review of the diff. Here is my assessment.

Code Review: Create the GitHub Check as in-progress before benchmarks run

Summary

This PR creates the GitHub Check with an in_progress status before benchmarks run, then completes it (with success/failure) once results are posted, or marks it failure if bencher run errors first. A stale conclusion from a prior run is cleared immediately on rerun. The work is well-decomposed, the branded title is deduplicated into a shared const, and the docs are updated across all locales.

Strengths

• Solid handle lifecycle. CiCheck is threaded through every result-posting path via &mut Option<CiCheck>, and consumed with .take() exactly where results are posted (display_results → ci.run). The run wrapper then fails any unconsumed handle. I traced every path (happy, dry-run, send-error, alert-error, detach, poll Completed/Failed/Canceled, fetch_report failure) and each consumes-or-fails exactly once. No double-completion, no leaked in-progress check on the reachable paths.
• Good refactoring. resolve_head_sha, check_conclusion, check_run_output, branded_summary, and check_error extract duplicated logic cleanly. resolve_head_sha taking github_sha: Option<String> for test determinism is the right call and follows the CLAUDE.md deterministic-test guidance.
• Best-effort discipline is consistent. Start/complete/fail all log and swallow errors rather than aborting the run, matching the pre-existing check behavior and the documented contract.
• Compiler-enforced exhaustiveness. The irrefutable destructuring in ci/mod.rs:81 and the tuple match in fail will fail to compile if a second Ci/CiCheck variant is added — aligned with the CLAUDE.md destructuring-over-field-access principle.
• Tests cover the new pure helpers (resolve_head_sha precedence/fallback/missing, check_conclusion, branded_summary, check_run_output). Reasonable given octocrab is hard to mock.
• Style compliance: no emdashes, thiserror variants wrap the original octocrab::Error, docs use relative terms, changelog + all locale docs updated.

Observations (non-blocking)

1. Detached / failed / canceled jobs complete the check based only on alert status, not job outcome. For a detached remote job, the check is marked success at submission time (before benchmarks run), and for Failed/Canceled jobs best_effort_display_report → display_results completes the check as success when there are no alerts even though the job itself failed. This is pre-existing behavior (the old create_github_check did the same), not a regression from this PR, but the in-progress check makes the premature-success on detach slightly more visible. Worth a follow-up if a green check on a failed job is undesirable.

2. Two Octocrab clients are built per run (one in start_check, one in complete_github_check/fail_check) instead of reusing one. Minor extra auth/setup cost, and each run now makes an extra Checks API call (create + update vs. create). Intentional and acceptable, just noting the doubled API traffic.

3. CheckRunHandle uses String for owner/repo rather than strong types. CLAUDE.md prefers strong types, but this matches the existing split_full_name (&str) surface, so it's consistent with surrounding code.

4. The generate_report() == None path returns Ok without consuming the handle, which would mark the check failed. In practice generate_report never returns None today, and the inline comment in exec_inner explicitly acknowledges this fallback behavior, so it's fine.

Verification notes

• I did not run cargo build/clippy (heavy in this environment). Before merge, confirm cargo clippy --no-deps --all-targets --all-features -- -Dwarnings, cargo test --doc, and cargo nextest run -p bencher_cli pass, and that the new octocrab::models::CheckRunId import resolves against the pinned octocrab version. Since the API surface is CLI-only and no #[typeshare] types changed, cargo gen-types should be unaffected.

Overall: approve. The handle lifecycle is careful and correct on all reachable paths, the refactor reduces duplication, and tests/docs are in good shape. No correctness or security issues found; the notes above are pre-existing-behavior context and minor efficiency observations.

---

_{Model: claude-opus-4-8}

[github-actions]

Bencher Report

Branch	claude/github-check-in-progress
Testbed	intel-v1

Click to view all benchmark results

Benchmark	Latency	Benchmark Result microseconds (µs) (Result Δ%)	Upper Boundary microseconds (µs) (Limit %)
Adapter::Json	📈 view plot 🚷 view threshold	4.71 µs (+1.55%) Baseline: 4.64 µs	4.90 µs (96.21%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	4.57 µs (+1.24%) Baseline: 4.51 µs	4.72 µs (96.80%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	25.51 µs (-0.37%) Baseline: 25.61 µs	26.77 µs (95.29%)
Adapter::Rust	📈 view plot 🚷 view threshold	3.51 µs (+0.52%) Baseline: 3.49 µs	3.61 µs (97.32%)
Adapter::RustBench	📈 view plot 🚷 view threshold	3.51 µs (+0.56%) Baseline: 3.49 µs	3.60 µs (97.46%)

🐰 View full continuous benchmarking report in Bencher

[epompeii]

Addressed the review findings in the latest push (5a1932d):

Finding 1 (orphaned check on the generate_report() == None path): fixed. The error hook in exec_inner no longer gates on result.is_err(). Every path that posts results consumes the check handle, so any unconsumed handle at the end of the run now marks the check as failed. This makes the invariant total instead of depending on that branch staying unreachable.

Finding 2 (failed remote job completes the check from the fetched report): deferred. This mirrors the pre-existing behavior, where the check was also created from whatever report was displayed. Making a failed or canceled remote job force a failure conclusion is a behavior change to remote job semantics that deserves its own issue rather than a ride-along here.

Finding 3 (permission help text printed twice): declined. The two warnings come from two distinct failed API calls (start and complete), and each message names the operation that failed. Suppressing the second would require threading state between the two call sites to dedupe a best-effort log line, which does not seem worth the complexity.

Also in this push: the in-progress and failed check summaries now render the same branded Bencher Report header as the completed report, so the check reads consistently in every state.

Local verification on the new commit: cargo nextest run -p bencher_cli, cargo clippy --all-targets --all-features -- -Dwarnings, and cargo fmt are all clean.

[epompeii]

Addressed the second round of review feedback in 467986b:

Issue 1 (duplicate permission warning): partially addressed. The start phase warning now notes that the check will still be created when the results are posted, so the second warning reads as the expected follow up rather than a mystery repeat. Full deduplication would require threading state between the two best effort call sites, which still does not seem worth it for a log line.

Issue 2 (header markup duplication): fixed. The shared title markup now lives in a single pub const BENCHER_REPORT_TITLE in bencher_comment, used by both ReportComment::html_header and the new CLI branded_summary helper, so the two cannot drift apart. Added unit tests for the in progress and failed summaries.

Issue 3 (fail fallback invariant): addressed via comment. The exec_inner comment now spells out that only error paths leave the handle unconsumed today, and that any future Ok path without results would mark the check as failed, which is still better than an eternally pending check. Exercising the fallback in an integration test would need an HTTP mock dependency, which this workspace intentionally avoids.

Local verification: cargo nextest run -p bencher_cli -p bencher_comment (35 tests), cargo clippy --all-targets --all-features -- -Dwarnings, cargo check --no-default-features, and cargo fmt are all clean.