Repository navigation
fix(host-service): treat a check run that failed to start as a failure - #8963
Conversation
mapCheckRunStatus had no STARTUP_FAILURE case, so a COMPLETED check run whose workflow never started fell through to default and reported pending. computeChecksStatus then kept the pull request pending forever. Map it to failure next to TIMED_OUT and ACTION_REQUIRED, and cover both the per-check status and the aggregate in tests.
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Map startup failures and verify aggregate status packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts, packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.test.ts |
STARTUP_FAILURE maps to failure. Tests verify the parsed check status and aggregate status. |
Priority:
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: kitenite
Merge Risk: 
Minimal · up to 91bc4
-
No actionable merge-blocking risk is evident in this change.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Title check
-
Explanation
The title "fix(host-service): treat a check run that failed to start as a failure" follows conventional commit format and states the primary change: a check run with STARTUP_FAILURE is now reported as failure. The diff maps STARTUP_FAILURE to failure in mapCheckRunStatus and adds tests for it, which matches the title.
Full details: Linked Issues check
-
Explanation
Issue
#8962has one coding requirement: a completed check run with conclusion STARTUP_FAILURE must map to status failure, and the aggregate checks status must be failure. The reviewed head addscase "STARTUP_FAILURE":next toTIMED_OUTandACTION_REQUIREDinmapCheckRunStatus(pull-request-mappers.ts), which returns "failure". Before this change, the value fell through todefaultand returned "pending".parseCheckContextspasses the check run's status and conclusion tomapCheckRunStatus, so the per-check status is now failure.computeChecksStatusreturns "failure" when any check has status failure, so the aggregate is also failure. The PR adds two tests in pull-request-mappers.test.ts. One assertsparseCheckContextsreturns{ name: "build", status: "failure", url: null }for a STARTUP_FAILURE run. The other asserts thatcomputeChecksStatusover that result returns "failure". These tests match the issue's expected results. The issue's reproduction steps (opening the PR in the review tab) are manual and not coding requirements. I read the code and test diff only. I did not execute the tests, so test results rely on the author's report.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff against the merge base contains two files. In pull-request-mappers.ts, the change is one added
case "STARTUP_FAILURE":line that returns "failure", which directly implements#8962. In pull-request-mappers.test.ts, the change adds a type import forGitHubCheckContextNode, adds aparseCheckContextsimport, adds acheckRunfixture helper, and adds adescribe("parseCheckContexts")block with two tests. All of these support the new tests for#8962. No other code changed.mapStatusContextStateand the STALE handling are unchanged, as the PR description states and the diff confirms. No unrelated changes were found.
Full details: Description check
-
Explanation
The description explains the problem, the code changes, test coverage, known unrelated failures, and checklist status. It uses Problem and Changes headings instead of the template's What & why heading, but it provides the required information. The How I tested it content is complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
[Medium impact] This change appears safe to merge. SummaryThe host now treats a completed GitHub check with
Reviews (1) · Last reviewed commit: "fix(host-service): treat a check run tha..." · Reviewed by Greptile |
|
I have read the CLA Document and I hereby sign the CLA |
Kitenite
left a comment
There was a problem hiding this comment.
Thanks! Verified the new tests fail without the fix and pass with it.
…#9161) Host-service and the desktop pull request view each had their own copy of the GitHub check run to status mapping and of the overall checks rollup. The copies drifted: the desktop handled STARTUP_FAILURE, host-service did not (#8962, fixed in #8963). Move both functions to @superset/shared/pull-request-checks so a new GitHub conclusion is added in one place. The v2 workspace computeChecksRollup and the mobile effectiveCheckStatus stay as they are: they count checks and treat skipped and neutral runs differently, so sharing them would change what the UI shows.
* chore: seed CLA signature file * @tanxarx has signed the CLA in #8881 * @ozeitis has signed the CLA in #8888 * @shubhankiit has signed the CLA in #8894 * @chrisfentiman has signed the CLA in #8910 * @RohithReddy20 has signed the CLA in #8907 * @larcane97 has signed the CLA in #8922 * @moizghumann has signed the CLA in #8935 * @andyst-dev has signed the CLA in #7930 * @svenpeeters has signed the CLA in #8943 * @kenil-kanani has signed the CLA in #8940 * @akshatcoder-hash has signed the CLA in #8963 * @NilotpalK has signed the CLA in #8995 * @serhii-kucherenko has signed the CLA in #9000 * @tonydzi has signed the CLA in #9002 * @adaam2 has signed the CLA in #9038 * @tsavo-at-pieces has signed the CLA in #9059 * @ganeshmshetty has signed the CLA in #9130 * @aaronrosenthal has signed the CLA in #9135 * @alexnault has signed the CLA in #9138 * @kunalgoesbyken has signed the CLA in #9235 * @Joaco-gavernet has signed the CLA in #9236 * @theblondealex has signed the CLA in #8240 * @talarczykco has signed the CLA in #7274 * @throw99 has signed the CLA in #7906 * @lacherogwu has signed the CLA in #9310 * @KGKallasmaa has signed the CLA in #9316 * @Niltonsf has signed the CLA in #9331 * @Anmol1578 has signed the CLA in #9351 * @RyoKusnadi has signed the CLA in #9317 * @thedotmack has signed the CLA in #9414 * @abudker has signed the CLA in #9439 * @sanchitmonga22 has signed the CLA in #9443 * @Bruno-rose has signed the CLA in #9446 * @Ashad001 has signed the CLA in #9487 * @vivek-pothina has signed the CLA in #8359 * @colingreig has signed the CLA in #9489 * @anmol0b has signed the CLA in #9492 * @resteqs has signed the CLA in #9506 * @pgilmore has signed the CLA in #7878 * fix(desktop): name the DMG from the display name The DMG file and its volume title now use DESKTOP_DISPLAY_NAME, so a Paradev build ships Paradev-<version>-<arch>.dmg. Unset, names stay Superset. The release keeps the Superset-<arch>.dmg alias beside the new one, since the README, docs and download button link to it. The .app bundle and the -mac.zip update files keep the Superset name. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Fixes #8962
Problem
mapCheckRunStatusinpull-request-mappers.tshas noSTARTUP_FAILUREcase. A check run that GitHub reports as COMPLETED with that conclusion falls to the default branch and comes out as pending, socomputeChecksStatuskeeps the pull request pending. The workflow never started, so GitHub shows that check run as a failure, and the mobile client already maps this conclusion to failed.Changes
STARTUP_FAILUREtofailureinmapCheckRunStatus, next toTIMED_OUTandACTION_REQUIRED.parseCheckContexts, and the aggregate fromparseCheckContextspluscomputeChecksStatus.mapStatusContextStateandSTALEuntouched.Tests
pull-request-mappers.test.tsbefore the fix: 12 pass, 2 fail, both new tests. After the fix: 14 pass, 0 fail.cd packages/host-service && bun test --pass-with-no-tests --parallelwith bun 1.4.3: 2683 pass, 8 todo, 3 fail. The three failures are the terminal router integration tests that need a real pty daemon, and the same three fail on main.biome checkandbiome formaton the changed files report no issues.bun run typecheckis red on main because of unrelated@superset/trpcerrors, and this change adds no new errors.Checklist
type(scope): subject)bun run lintandbun run typecheckpass (CI fails on lint warnings too). Lint passes on the changed files. Typecheck fails on main in@superset/trpcand is unrelated to this change.