Skip to content

fix(server): retain GitLab approval system notes - #9858

Open
juliusmarminge wants to merge 2 commits into
mainfrom
fix/audit-gitlab-approval-notes
Open

juliusmarminge wants to merge 2 commits into
mainfrom
fix/audit-gitlab-approval-notes

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 5, 2026 •

Copy link
Copy Markdown
Member

GitLab approval and withdrawal notes are discarded with all system activity, so the existing review view receives no record of either event. This is a bounded candidate for #9802, not a complete system timeline.

Keep only the exact approval and unapproval bodies generated by GitLab's note service, and only when system is true. Map them to the existing review kind with APPROVED or DISMISSED, preserving the note ID, author, body, and timestamp. Other system notes remain hidden. An ordinary comment containing the same words remains a comment.

No new API requests, schema fields, or dependencies. The existing review kind avoids offering an edit button for GitLab-generated notes. GitLab also rejects emoji awards on system notes, so the web and desktop Summary and Timeline hide reaction controls for GitLab review rows. Ordinary GitLab comments and description reactions keep their existing behavior. GitLab's adapter emits review only for these system-derived notes. Raw note counts remain unchanged so a full page of skipped activity does not stop pagination early.

Verification on main 82f64cd8:

  • Before: the production decoder drops both approval and withdrawal. The real CLI note walker follows a full page of unrelated system activity, then still returns no comments from the page containing both events. Both new regressions fail on unchanged main.
  • After: the decoder and CLI output retain the two historical reviews, preserve their author/timestamps, exclude unrelated system activity, and do not classify an authored lookalike as a verdict.
  • vp test run apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts apps/server/src/pullRequest/GitLabPullRequestCli.test.ts apps/server/src/pullRequest/GitLabPullRequestProvider.test.ts apps/web/src/components/pullRequest/pullRequestReactions.logic.test.ts apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts apps/web/src/components/pullRequest/pullRequestEditing.logic.test.ts --maxWorkers=2: 228 tests pass. Reaction permission cases cover GitLab system-derived reviews, ordinary notes, inline comments, other-provider reviews, and disabled or absent host capability. The GitLab review case fails when the helper uses the previous capability-only predicate.
  • Targeted lint/format and server/web typechecks pass. Lint reports one pre-existing array-index-key warning in Summary's loading skeleton. No native client was launched; desktop shares these web views and mobile has no corresponding reaction controls.

Human decision required; leave unmerged. Reusing the review kind feeds the existing comment/timeline presentation and current-review badges from #7077. Those badges use a commit-date heuristic, not GitLab's authoritative current approval state. Automatic approval resets, other system-note variants, and localized/custom bodies are not implemented here. A maintainer must decide whether this historical-note mapping is acceptable or whether approvals need a distinct presentation/current-state read.

The local evidence is decoder/CLI output, not a live GitLab account or integrated client capture. An anonymous GitLab notes request returned 401; no credentials or production state were changed. Integrated visual evidence remains pending. Do not close the broader issue from this candidate alone.

Refreshed September 5 onto main ce4712d, which fixes the Ubuntu CI mirror transport upstream. The seven changed source/test files are byte-identical to the previously reviewed candidate. All 228 focused tests, server/web typechecks and targeted lint/format pass again, with the same existing skeleton warning. Current-head CI, Bugbot, UI and Effect checks pass at c1c5990. Correctness and approvability checks skipped the unchanged rebase; the prior human-review requirement remains. There are no review threads or active change requests. The previous CI run 33936579569 exhausted its one retry on the old HTTP mirror failure; it is not counted as passing. This PR adds no workflow changes or bypasses. Human approval and integrated visual evidence remain required, so it is not merge-ready.

Prepared by GPT 6 Astra via Codex in T3 Code.


Note

Low Risk
Scoped to GitLab note decoding and UI reaction gating, with regression tests; review badges still use commit-date heuristics rather than GitLab’s live approval state.

Overview
GitLab merge request conversations previously dropped all system notes, so approvals and unapprovals never reached the review UI. decodeNotesJson now keeps only exact system: true bodies (approved this merge request / unapproved this merge request), maps them to review comments with APPROVED / DISMISSED, and still filters other system activity. User-authored text that looks like an approval stays a normal comment; rawCount is unchanged so note pagination does not stop on a page of skipped system notes.

The web layer adds canReactPullRequestComment, which turns off emoji reactions on GitLab review rows (system notes cannot be awarded on GitLab). Summary and Timeline use that helper instead of the global reactions capability alone.

Reviewed by Cursor Bugbot for commit c1c5990. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 5, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR makes a focused production fix to GitLab approval-history pagination and reaction affordances, with targeted tests and limited runtime scope. It also adds line-level suppressions for a static-analysis diagnostic in the new test, so human review is required.

No code changes detected at c1c5990. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

github-actions Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.5 KiB 13.6 KiB +51 B (+0.4%) 15.1 KiB ✅
Codex Thread snapshot wire 7.0 KiB 7.0 KiB 0 B (0.0%) 7.3 KiB ✅
Codex Live turn WebSocket wire 6.5 KiB 6.6 KiB +51 B (+0.8%) 7.8 KiB ✅
Codex Live turn WebSocket decoded 57.0 KiB 57.1 KiB +88 B (+0.2%) 66.4 KiB ✅
Codex Live turn messages 8 10 +2 (+25.0%) 21 ✅
Claude Total thread wire 13.8 KiB 13.8 KiB +1 B (+0.0%) 15.1 KiB ✅
Claude Thread snapshot wire 7.0 KiB 7.0 KiB −1 B (−0.0%) 7.3 KiB ✅
Claude Live turn WebSocket wire 6.7 KiB 6.7 KiB +2 B (+0.0%) 7.8 KiB ✅
Claude Live turn WebSocket decoded 59.3 KiB 59.3 KiB 0 B (0.0%) 66.4 KiB ✅
Claude Live turn messages 10 10 0 (0.0%) 21 ✅

Baseline: ce4712d · PR result: c1c5990 · Source CI: success

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: 113.8 KiB
  • Claude decoded thread snapshot: 114.5 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@juliusmarminge
juliusmarminge force-pushed the fix/audit-gitlab-approval-notes branch from 8b40be3 to c1c5990 Compare September 5, 2026 02:12

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant