Repository navigation
fix(server): Forgejo checks keep the final status when it shares a second with a pending one - #24871
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a small, well-scoped server bug fix that resolves equal-second Forgejo check statuses using their IDs while preserving existing behavior when IDs are unavailable. The accompanying tests cover ordering and backward-compatible fallback cases. You can add or adjust custom eligibility rules. Learn more. |
Walkthrough
Merge Risk:
|
…cond with a pending one Fixes pingdotgg#24858
25786b1 to
f86398a
Compare
|
Note Written by thanks @fixfon. your optional status-id tie-break preserves timestamp precedence and leaves other providers unchanged. i kept your commit and merged current main into this branch without rebasing. i removed the extra comparison helper and its duplicate types. the reverse timestamp comparison now runs only when both ids exist and the incoming id is lower. i also extended your provider regression to cover both check summaries and full pull request details. no behavior or original test coverage was dropped. verification on
unverified: live forgejo and browser rendering. this changes server status selection, with no client code changes; the provider regressions cover decoding and both read paths using fixture api responses. all 29 ci checks are green and gitcafe reports this head mergeable. the automatic review found no merge-blocking issue. its optional approval-link suggestion needs no code change: maria explicitly authorized this takeover and its original scope. |
|
Note Written by Verified in a real dev environmentverified as-is at ran main and the exact pr head sequentially with
the recording shows the pending watch, its actual wake, and sol's response. the idle interval between them is cut. Not verifiednative desktop, ios/android, remote/relay/tunnel connections, live github/bitbucket, and forgejo responses without status ids. no forgejo actions runner was exercised; statuses came from the real status api. production forgejo deployments and other version/database combinations were not verified for this regression. |
…ingdotgg#24706 instead of our own Forgejo fixes' (pingdotgg#97) from fix/forgejo-upstream-prs into dev



Problem
A Forgejo pull request watch never wakes when a check's final status and an earlier pending status for the same check carry the same
updated_at. Forgejo timestamps have one-second resolution, so a job that is skipped or finishes within a second leaves both statuses dated alike.dedupeCheckskeeps the newest entry per check, andisAtLeastAsNewreturns true on equal timestamps. Forgejo lists statuses newest first, so the older pending status comes later in the list, ties with the final one and replaces it. The check stays pending and the watch never reaches "All checks passed". The statuses in #24858 show it: id 15successand id 14pendingboth at10:14:29.Change
dedupeChecksentries take an optionalsequence. When two entries for a check have the same instant and both carry asequence, the higher one is kept. In every other case the choice is the same as before, including the existing "last copy wins" for equal instants without asequence.packages/source-control-core/src/server/pullRequestChecks.ts: the tie-break.packages/source-control-forgejo/src/server/forgejoPullRequestJson.ts:ForgejoStatusdecodes an optionalid, andforgejoCheckspasses it assequence. It is optional because existing fixtures and servers may omit it.GitHub and Bitbucket never set
sequence, so their results do not change. GitCafe does not usededupeChecks.I considered sorting the statuses by id inside the Forgejo mapping instead. That would also reorder the checks shown for Forgejo pull requests, so I kept the tie-break inside
dedupeChecks.Scope and approval
This fixes the bug in #24858, which has no maintainer triage yet. It is a small, focused fix of an obvious bug with no new setting or behavior: it only changes which of two statuses wins when their timestamps are equal. The issue itself suggests keeping the higher Forgejo status
id, and that is what this does. That is why I'm sending it under the CONTRIBUTING exception for a very small fix of an obvious bug: the statuses posted in #24858 show the wrong pick directly, and the fix touches only the tie-break.Verification
vp test runon the two touched test files gives2 failed | 50 passed. Both failures arependingwheresuccesswas expected: the Forgejo provider test with the two statuses from [Bug]: Forgejo PR watch never wakes when a check's final status shares a second with its pending one #24858, and thededupeCheckstie test. The other two newdedupeCheckstests (a newer instant beats a highersequence, and asequenceon only one side) pass either way, because they guard behavior that already exists.52 passed. Reverting only the two source files fails the same two tests again.vp lintandvp fmt --checkon the changed files, typecheck ofsource-control-coreandsource-control-forgejo, andvp run check:changedall pass.idmeans a later status rests on the status table in [Bug]: Forgejo PR watch never wakes when a check's final status shares a second with its pending one #24858 (id 15success, 14pending, 13pending) and on my reading of the Forgejo API, not on a test against a server. I also did not confirm that every Forgejo version returnsidon statuses. A status without anidkeeps the old behavior. The full CI suites and the build were not run locally.Closes #24858
Claude Sonnet 5.5 (xhigh) via T3 Code.