Synced from GitHub
fix(gitlab): retain approval and withdrawal system notes#21570
GitLab approval and withdrawal system notes were discarded, so merge-request history omitted those decisions.
Retain the exact system-note bodies as historical review rows: approval becomes APPROVED and withdrawal becomes DISMISSED. Authored lookalikes remain ordinary comments; unrelated and near-match system activity stays filtered. Author, original body, time, and raw-count pagination are preserved in the moved GitLab adapter.
Rebuilt from @juliusmarminge's #9858, whose adapter paths predate the source-control package move. The original diff was compared with this stack: exact approval/withdrawal decoding and pagination coverage live in this layer; the dependent reaction restriction lives in #21571. Redundant decoder coverage was consolidated into the existing CLI suite; no static-analysis suppressions were carried over.
Verification on headless Linux at e8655cf0fb:
- Original two adapter tests reduced to one CLI regression combining a full filtered page, approval, withdrawal, an authored lookalike, and a near-match system note.
- Before: 104 baseline tests pass; the new regression fails because only the authored comment survives.
- After: 105/105 CLI and decoder tests pass. GitLab package typecheck, targeted lint, formatting, and whitespace checks pass.
- The CLI uses injected responses; no live GitLab account or production server was used. Screenshots do not prove provider pagination.
This retains historical notes and the existing client commit-date heuristic; it does not add authoritative current approval state or requested-changes note support. GitLab may reset an approval after a push without emitting a withdrawal note; if the pushed commit has an older commit date, the inferred approval badge can remain stale. Authoritative reviewer state needs a separate follow-up and this limitation needs maintainer acceptance. macOS, Windows, iOS, Android, and Electron desktop were not exercised. Sol's full-stack review found no code blockers and called out the approval-reset limitation above. Two full-stack rounds from Opus and Sol are complete with no remaining code blockers. Approval and withdrawal notes also wake PR watchers as review activity, using the existing remark watermark. CI remains blocked: the required Test Web job failed twice in the unchanged WorkflowOpeningBlock syntax-highlighting test; that focused file passes locally (3/3), and the dependent layer passed its complete current-head web CI. A passing lower-layer CI run remains required; this is not ready to merge. Human review is still required before merging; nothing is merged.
Tests: Codex GPT-6-Astra (high). Implementation: Codex GPT-6.1-Sol (high). Review: Claude Opus 5.5 and Codex GPT-6-Sol. Maintainer integration: GPT-6.1-Sol through the Codex harness in T3 Code.
Added 2 commits
Synced from GitHub
Approvability
Verdict: Approved at e8655cf
Macroscope's review found this PR approvable — This narrowly scoped GitLab adapter fix restores approval and withdrawal history while preserving existing filtering, pagination, and comment behavior. The accompanying change is test-only and covers the relevant exact-match and pagination cases.
You can add or adjust custom eligibility rules. Learn more.
Synced from GitHub
Thread transfer impact
✅ Thread transfer remains within every enforced ceiling.
| Provider | Metric | Main baseline | This PR | Impact | PR ceiling | |
|---|---|---|---|---|---|---|
| Codex | Total thread wire | 5.0 KiB | 4.9 KiB | −23 B (−0.5%) | 6.8 KiB | ✅ |
| Codex | Thread snapshot wire | 3.8 KiB | 3.8 KiB | 0 B (0.0%) | 4.9 KiB | ✅ |
| Codex | Live turn WebSocket wire | 1.2 KiB | 1.2 KiB | −23 B (−1.9%) | 2.0 KiB | ✅ |
| Codex | Live turn WebSocket decoded | 20.9 KiB | 20.8 KiB | −41 B (−0.2%) | 29.3 KiB | ✅ |
| Codex | Live turn messages | 2 | 1 | −1 (−50.0%) | 8 | ✅ |
| Claude | Total thread wire | 5.0 KiB | 5.0 KiB | +7 B (+0.1%) | 6.8 KiB | ✅ |
| Claude | Thread snapshot wire | 3.8 KiB | 3.8 KiB | 0 B (0.0%) | 4.9 KiB | ✅ |
| Claude | Live turn WebSocket wire | 1.2 KiB | 1.2 KiB | +7 B (+0.6%) | 2.0 KiB | ✅ |
| Claude | Live turn WebSocket decoded | 21.2 KiB | 21.2 KiB | 0 B (0.0%) | 29.3 KiB | ✅ |
| Claude | Live turn messages | 2 | 2 | 0 (0.0%) | 8 | ✅ |
Baseline: 1451e7c · PR result: e8655cf · Source CI: failure
Scenario and decoded snapshot size
10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
- Codex decoded thread snapshot: 108.5 KiB
- Claude decoded thread snapshot: 108.8 KiB
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.
Synced from GitHub
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
- Configuration used: Path: .coderabbit.config.ts
- Review profile: CHILL
- Plan: Team
- Run ID:
d74653f3-f3f9-426a-b797-177738d55bcd
📥 Commits
Reviewing files that changed from the base of the PR and between 1451e7cb23034d29774338608368a15d8845df36 and e8655cf0fb15a2ee86e0d047fac35772c85e1615.
📒 Files selected for processing: 2
packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.tspackages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts
Limit details: You’ve used all 10 included reviews currently available.
📝 Walkthrough
GitLab merge request note decoding now maps exact approval and unapproval system notes to review comments. A new test checks these results alongside ordinary comments and pagination.
Changes: GitLab approval notes
Layer / File(s) Summary Decode approval notes and test pagination
packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts,packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.tsdecodeNotesJsonskips blank notes and system notes other than the exact approval and unapproval messages. It maps those messages toAPPROVEDandDISMISSEDreview comments. The test checks two-page retrieval, ordinary comments, and the decoded review comments.Priority: ⬇️ Low
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers:
maria-rcks
Merge Risk: 🔵 Low · up to e8655
In a narrow reset-on-push case, an approval badge may be stale, but GitLab still controls whether the merge can proceed. This can be merged with awareness of that display limitation.
Pre-merge checks: Passed 4
Check name Status Explanation Title check Passed The title is concise, specific, and accurately describes retaining GitLab approval and withdrawal system notes. 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 explains the problem, implementation, scope, verification results, limitations, CI status, and review context. It does not use the template headings, and explicit maintainer approval o… Full details: Description check
Explanation
The description explains the problem, implementation, scope, verification results, limitations, CI status, and review context. It does not use the template headings, and explicit maintainer approval of scope is not clearly stated, but the required information is mostly present.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
📝 Generate docstrings
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
- Commit to this branch
- Create a new PR
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.