Skip to content

fix(gitlab): submit multi-line range comments without schema errors - #598

Merged
agavra merged 1 commit into
agavra:mainfrom
samuelarbibe:fix/gitlab-multiline-position
Aug 13, 2026
Merged

agavra merged 1 commit into
agavra:mainfrom
samuelarbibe:fix/gitlab-multiline-position

Conversation

@samuelarbibe

Copy link
Copy Markdown
Contributor

Problem

Submitting a review to GitLab fails whenever the payload contains a multi-line / visual-range comment:

400 Bad request - Note {:position=>["must be a valid json schema"]}

(or the draft-notes equivalent, {"message":{"position":["must be a valid json schema"]}}). Single-line comments are unaffected, and the failure takes the whole :submit down with it.

Root cause

Two separate problems in gl_range_endpoint:

1. The endpoints carry line numbers. Each position.line_range.{start,end} was built with type + line_code + new_line/old_line. Grape declares the nested old_line/new_line params as String while position.json requires an integer — no value satisfies both validators, so the note is rejected outright (gitlab-org/gitlab#572121). Endpoints now carry line_code plus, for changed lines only, type. A context line belongs to both sides, so declaring a type for it is rejected as well and it is now omitted.

2. The line codes were wrong. line_code is sha1(path)_<old>_<new> built from the diff's running counters — an added line still carries the old-side counter it was inserted at, and a deleted line the new-side counter it sits before. They were built as sha1(path)_0_<line>, so even an accepted note anchors the range to the wrong lines. Neither counter is recoverable from a single line number, so InlineComment gains range_anchors: both endpoints in both counters, resolved from the parsed diff in find_diff_anchor. Other forges ignore the field.

While anchoring the endpoints it became clear a range ending on a context line has the same problem a single-line comment on that line does — GitLab needs both old_line and new_line in the position — so map_range now fills in counterpart_line for ranges too, matching what the single-line path already sent.

Tests

  • should_send_line_range_for_multi_line_range_comment — endpoints are exactly type + line_code, no line numbers
  • should_omit_endpoint_type_for_context_line_in_range — a context endpoint carries only line_code, and the position names both sides
  • should_anchor_range_endpoints_in_both_line_counters — an added line resolves to (old, new), not (0, new)
  • should_populate_counterpart_line_for_range_ending_on_context_line

cargo test (1364 passed), cargo fmt --check and cargo clippy --all-targets are clean.

Notes

Fixes #557, which I filed and then closed while carrying this on a fork — it is a real upstream bug, verified against a self-hosted GitLab instance.

🤖 Generated with Claude Code

GitLab rejects a range comment with

    400 Bad request - Note {:position=>["must be a valid json schema"]}

because each `position.line_range` endpoint carried `new_line` / `old_line`
alongside `line_code`. The Grape API declares those nested params as String
while position.json requires an integer, so no value satisfies both
validators (gitlab-org/gitlab#572121). Endpoints now carry `line_code` and,
for changed lines only, `type` — a context line belongs to both sides, so
declaring a `type` for it is rejected too.

The endpoint line codes were also wrong. `line_code` is
`sha1(path)_<old>_<new>` built from the diff's running counters, so an added
line still carries the old-side counter it was inserted at. They were built
as `sha1(path)_0_<line>`, which anchors the range to the wrong lines even
when the note is accepted. `InlineComment` now carries `range_anchors` —
both endpoints in both counters, resolved from the parsed diff — and a range
ending on a context line fills in `counterpart_line` so the position names
both sides, matching what a single-line comment on that same line sends.

Fixes agavra#557

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@agavra agavra left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't have a gitlab account to test against, but I checked the non-GL parts of the patch and it LGTM.

@agavra
agavra merged commit 8d5af8b into agavra:main Aug 13, 2026
4 checks passed
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.

GitLab review submit fails with position JSON schema error on multiline/range comments

2 participants