Fix superseded PR check rollups - #1348
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this change now. I will check security, code quality, performance, architecture, tests, and the end-to-end behavior. |
| candidate: GitHostPullRequestCheck, | ||
| current: GitHostPullRequestCheck, | ||
| ): boolean { | ||
| if (candidate.startedAt === null) { |
There was a problem hiding this comment.
🚨 slopcop/review — A queued rerun can stay hidden behind an older completed run.
GitHub defines CheckRun.startedAt as nullable. This branch makes a null candidate lose to every dated run. The gh JSON can also represent a missing time as year 0001, which has the same result. I reproduced an older successful run plus a newer queued run. The rollup returned passing with pendingCount: 0. Equal timestamps also depend on the input order.
Please preserve a creation time or a stable run identifier. Add tests for null, zero, and equal timestamps in both input orders.
| ): ThreadPullRequestChecks { | ||
| const latestChecksByName = new Map<string, GitHostPullRequestCheck>(); | ||
| for (const check of rawChecks) { | ||
| const current = latestChecksByName.get(check.name); |
There was a problem hiding this comment.
🚨 slopcop/review — Name-only grouping can combine independent checks.
A visible check name is not a stable producer identity. Separate workflows or check producers can use the same name. A later success can then replace an independent failure. The host currently removes the check kind and producer identity before this map receives the data.
Please preserve a stable workflow or producer identity. Group with that identity and the check name. Keep ambiguous checks separate when the identity is absent.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
GitHub can send several attempts for one check. This change keeps the newest attempt before it reports the result.
Two edge cases can still hide a pending or failed check. The app can then show a green result when it should not.
Findings
-
High: A queued rerun can lose to an older completed run. GitHub permits a null
startedAtvalue. The comparator always keeps the older dated run. A direct test returnedpassingwithpendingCount: 0. Equal timestamps also depend on the array order. -
High: The map uses only the visible check name. Separate workflows or producers can use the same name. A later success can replace an independent failure. Preserve a stable producer identity and keep ambiguous checks separate.
-
Medium: The GitHub plugin has a second check-rollup implementation. It still counts all superseded rows. Move the common GitHub normalization and selection into one internal module. Keep the product attention policy in the server.
I left inline comments for both high-severity findings.
Snapshot note
The requested SHA omitted regenerated plugin SDK declarations. That snapshot failed the generated-output CI check. Commit a53b8c5 fixes this item at the current PR head.
Security and performance
I found no command injection, path escape, authorization bypass, or data exposure. GitHub still enforces merge authorization and branch protection.
The new map uses linear time over bounded check data. I found no material performance problem.
Validation
- The exact SHA passed the full server, host-workspace, and host-daemon-contract test suites.
- The server suite passed 1,433 tests.
- The three changed test files passed 69 tests.
- Five affected package type checks passed.
git diff --checkpassed.- A direct test confirmed the null-time and equal-time defects.
- Dev-browser verified the public GitHub pull request workflow.
- The live product route reached the daemon, but that daemon did not have
gh.
The pull request advanced during this review. The code findings remain fixed to fdef28c83cc80b33af29864b2f9d78303ec1f547.
2 participants
Summary
Testing
Closes #1256