Skip to content

Correct GitLab MR checks for failed and manual pipelines - #34176

Merged
brennanb2025 merged 6 commits into
mainfrom
brennanb2025/tp-gitlab-mr-checks
Oct 27, 2026
Merged

brennanb2025 merged 6 commits into
mainfrom
brennanb2025/tp-gitlab-mr-checks

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​334 $\color{#cf222e}{\Huge{\mathbf{−}}}$​75 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​259
Prod 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​95 $\color{#cf222e}{\Huge{\mathbf{−}}}$​15 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​80

Problem

An MR found by its branch could look green even when GitLab CI failed: the branch search returns a summary without the pipeline, and Orca treated that missing result as neutral. Separately, an MR GitLab considered ready to merge could show pending checks forever when its pipeline waited on a manual stage.

Change

Branch discovery now uses the same single-MR detail read as an explicitly linked MR, so failed pipelines reach the existing red check presentation. Detail-read errors follow the existing strict lookup error path, preserving the last known review instead of replacing it with a neutral summary.

A manual pipeline becomes neutral when GitLab explicitly reports the MR as mergeable and no conflict is reported. It no longer looks like automatic work is still running, and Orca does not claim the unrun manual stage passed. Intended: manual pipelines remain pending when mergeability is unknown or blocked, including ci_must_pass; actual failed and running pipelines keep their existing results. Neutral open-MR rendering is unchanged. This follows the common pattern of keeping check results separate from merge readiness.

Branch discovery adds one detail request per discovered MR refresh, inside the existing shared branch cache and concurrency slot. Found reviews are cached for 60 seconds across callers; absent reviews refresh after 15 minutes, or 60 seconds for a selected branch. Explicit refresh/invalidation can bypass the cache. Linked MRs still need one detail request; an empty branch search needs only its list request. No new polling or cache is introduced.

Verification

Regression tests failed against the original implementation: a failed detail pipeline was returned as neutral, branch discovery never requested detail, and a mergeable manual pipeline remained pending. The tests also cover missing/null/stale pipeline summaries, strict detail-fetch errors, the legacy pipeline field, blocking or unknown mergeability, and equality between linked and discovered results.

104 tests passed across client-mr-branch-lookup.test.ts, mappers.test.ts, mappers-workitem.test.ts, hosted-review.test.ts, forge-provider.test.ts, and worktree-review-helpers.test.tsx. The request-count contract drives 10 and 100 branch cache keys with concurrent and repeated polls: exactly one list plus one detail request per branch until cache expiry, then one new pair. Existing rendering tests cover the check tones. A composed producer/consumer contract covers the actual card and parent row, metadata/reference/folder projections, Checks-tab status, merge readiness, summary text and refresh policy. Main tests compare complete real mapper output with a shared typed review fixture; renderer tests consume that identical review, respecting both compiler project boundaries. Linked and discovered strict detail failures are compared directly. No renderer layout or component changed.

Full path: this changes provider discovery and adds bounded API work on a repeated refresh trigger, so the effect is broader than a local rendering or pure-function change.

Builds on #18492 by @cbarraford — thank you!

Builds on #27470 by @ItsEmilyJpg — thank you! The manual result is neutral rather than passed, while keeping the contextual distinction between mergeable and blocked MRs.

Tracked in #18484 and STA-6591.

Tracked in #27469 and STA-10123.

Review-loop verification also executed real glab commands against isolated localhost HTTP fixtures through production lookup, mapping and branch cache: seven tests passed, including 10/100 branch keys with exactly 20/200 requests despite concurrent and repeated readers. This is real CLI/HTTP fixture evidence, not a live GitLab account, quota or remote-latency test.

A branch-only component fixture on Windows rendered the actual MR mapper, icons, parent Checks row and merge action in dark and light themes; all three failed/mergeable-manual/blocked-manual cases passed with zero final page errors. Intended: a mergeable manual parent row says “No check signal” using the existing generic neutral summary, because the unrun manual stage has no automatic pass/fail result. It never says passed.

The loop added consumer/error-parity test coverage and corrected CI diagnostics: the lookup fixture derives its type from the mapper, the two existing JSON assertions have line-specific documented exceptions, and a shared typed producer/consumer fixture fixes the renderer test project boundary and supplies required reference fields. The final correction passed 60 targeted tests and the normal commit hook. No runtime behavior changed during the review loop. Main was merged once before final validation; no rebase or force push.

brennanb2025 and others added 3 commits October 26, 2026 17:55
Use the same single-MR read for linked and branch-discovered reviews so a
list summary cannot hide a failed pipeline. Preserve lookup failures for
strict callers and keep detail reads within the existing branch cache.

Set mergeable manual pipelines to neutral without claiming an unrun stage
passed; keep blocking manual pipelines pending.

Co-authored-by: Son of Odin <2700975-son-of-odin@users.noreply.gitlab.com>
Co-authored-by: Emily <hadikcze@gmail.com>
@brennanb2025
brennanb2025 marked this pull request as ready for review October 27, 2026 01:17
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 79df2a8f-8f3a-413b-b402-556280d4d259
Commits

Reviewing files that changed from the base of the PR and between e277238 and be6b295.

Files selected for processing: 1
  • src/main/gitlab/client-mr-branch-lookup.test.ts
Files skipped from review as they are similar to previous changes: 1
  • src/main/gitlab/client-mr-branch-lookup.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

Manual pipelines map to neutral status only when the merge request is mergeable and has no explicit conflicts. Branch lookups fetch individual merge-request details to determine pipeline status. Tests cover lookup errors, request caching, and status presentation across worktree and workspace components.

Priority: Normal

Merge Risk: Minimal · up to be6b2

No actionable merge-blocking issue is established for the reviewed change; it is mergeable after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 25.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 surv… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary changes: correcting GitLab merge-request results for failed and manual pipelines.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description is detailed and explains the problem, implementation, rationale, linked issues, testing, performance impact, and review-loop verification. It does not use every template heading, and i…
Full details: Description check

Explanation

The description is detailed and explains the problem, implementation, rationale, linked issues, testing, performance impact, and review-loop verification. It does not use every template heading, and it does not explicitly provide the checklist or a Visual Proof section, but it states that no renderer layout or component changed and provides substantial verification details.

Full details: Docstring Coverage

Explanation

Docstring coverage is 25.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 4 functions across 5 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cab62a6e-2036-463a-80c1-91c4794dfbde
Commits

Reviewing files that changed from the base of the PR and between 57aa892 and e277238.

Files selected for processing: 5
  • src/main/gitlab/client-mr-branch-lookup.test.ts
  • src/main/gitlab/mappers.test.ts
  • src/main/gitlab/mappers.ts
  • src/main/gitlab/merge-request-lookup.ts
  • src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Branch-discovered GitLab MRs could hide failed pipelines because branch search returns an incomplete summary. Mergeable MRs with a manual pipeline could also look pending indefinitely.

The lookup now reads the same MR detail used for linked MRs. A failed pipeline reaches the existing failure display. A manual pipeline is neutral only when GitLab explicitly confirms mergeability and no conflict is reported; it is never called passed. Blocked and unknown manual pipelines stay pending. Intended: the parent Checks row uses its existing “No check signal” summary for the mergeable manual case, which has no automatic pass/fail result. Detail-read failures retain last-known data where available, and remain unavailable when nothing was previously known.

The full review found no production defect requiring a fix. The loop added a composed producer/consumer contract using the same typed cases and linked/discovered failure parity coverage. Main tests prove the complete real mapper output equals the shared review; renderer tests feed that exact review to the actual consumers. It corrected test-project imports and required fixture fields after CI exposed those test errors. No code findings remain deferred. The extra request is bounded: one list plus one detail per found branch cache refresh; linked and absent-review lookups keep their previous request counts. The existing shared cache and concurrency limit remain in place.

Verified: targeted provider/cache/renderer tests; real glab commands against isolated HTTP fixtures for failed/manual/blocked/unknown results and detail errors; 10 and 100 distinct branch keys with concurrent/repeated readers, exactly 20 and 200 requests. These exercise real CLI/HTTP and production mapping/cache behavior with synthetic API data. Live GitLab quota and remote-host latency were not measured. No native-chat, login, process-ownership, layout or agent code changed; validation is the tests row, with a branch component screenshot rather than an Electron/agent QA round.

For Brennan’s review:

Actual components with failed, mergeable-manual and blocked-manual MR inputs (branch only, unchanged layout):

Dark component fixture

Light component fixture

CI is green on 263d5126b7e0959c900f6f88b07787dc9e6fe96f (PR checks). One failed-jobs-only retry resolved a 30-second timeout in the unchanged source-control-ai-settings-persistence.test.tsx; the unchanged-head shard then passed in 8m17s. Successful jobs were retained. No code or timeout changed for the retry.

Evidence is retained from component/CLI head 6c5694c3d57607619ed239792c78eed175061f5f; the current head is 263d5126b7e0959c900f6f88b07787dc9e6fe96f. Since that run:

  • 6ad2127f548 merged main once, without changing the reviewed GitLab mechanism.
  • e277238a92d added consumer and strict detail-error parity tests (57 targeted tests passed).
  • be6b2957b60 derived the lookup fixture type from its mapper (30 targeted tests passed).
  • 9e03dd4dcbf documented two existing lookup JSON assertions with supported line exceptions; runtime behavior is unchanged.
  • 263d5126b7e repaired the loop's compiler-boundary/reference-fixture errors using the composed contract above (60 targeted tests passed). Production code and compiler configuration are unchanged.

These later changes do not change the pictured outcomes. Four review rounds plus the fresh decider's KEEP ruling cover this final head; no new screenshot or agent run was required.

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.

1 participant