Fix/gitlab multiline comments and suggestions (#1000)

* fix(examples): Fix GitLab CI discussions so they highlight all the lines

* fix(examples): Fix GitLab CI suggestions so they show all the lines of the before state

* Fix(examples): Move sha1 computation and add start_line to loop

* fix(exampes): line_range is now only added fro multiline comments

* fix(examples): Added multiline check to suggestions

* fix(examples): resolve GitLab multiline line_code from the MR diff

A multiline discussion's line_code hardcoded old_line=0
(<sha>_0_<new>), which is only valid for purely-added lines; GitLab
rejects it when a boundary falls on an unchanged context line. Resolve
each boundary from the MR diff: added lines keep <sha>_0_<new>, context
lines use their real old line, and line_range is omitted (single-line
anchor) when the diff or a boundary cannot be resolved.

---------

Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>

examples/gitlab_ci/post_review.py

@@ -40,6 +40,7 @@ Standard library only (json, urllib) so it runs on any stock python3 image.
 """
 
 import argparse
+import hashlib
 import json
 import os
 import random
@@ -261,8 +262,9 @@ def format_comment(comment, comment_id=None):
     The per-comment id tag (when provided) is prepended as an HTML comment so
     :func:`reconcile_posted_id` can match it back on retry. The category/severity
     badge is then prepended on its own line. The suggestion uses GitLab's
-    ``suggestion:-0+0`` info string (kept at the fixed triple-backtick form so
-    the "Apply suggestion" button keeps working).
+    ``suggestion:-N+0`` info string, where ``N`` is the number of extra lines
+    above the anchor covered by a multiline span (``0`` for a single line), so
+    the "Apply suggestion" button rewrites the whole existing block.
     """
     body = ""
     if comment_id:
@@ -274,8 +276,10 @@ def format_comment(comment, comment_id=None):
     suggestion = comment.get("suggestion_code", "")
     existing = comment.get("existing_code", "")
     if suggestion and existing:
+        span = comment_span(comment)
+        suggestion_offset = span["end"] - span["start"] if span is not None and span["multiline"] else 0
         body += "\n\n**Suggestion:**\n"
-        body += "```suggestion:-0+0\n%s\n```" % suggestion
+        body += "```suggestion:-%d+0\n%s\n```" % (suggestion_offset, suggestion)
     return body
 
 
@@ -535,6 +539,88 @@ def parse_diff_hunk_inventory(patch):
     return ranges, (saw_hunk and complete)
 
 
+def build_new_line_positions(patch):
+    """Map each new-file line number to its diff position within a patch.
+
+    Returns ``{new_line: {"type": "new"|"context", "old_line": int|None}}``.
+    Added lines are ``"new"`` (no old-file counterpart, so ``old_line`` is
+    ``None``); unchanged context lines carry their real ``old_line``. Removed
+    lines have no new-file position and are omitted. Empty/blank patches yield
+    an empty map. Line classification mirrors :func:`parse_diff_hunk_inventory`
+    (only ``+`` and space-prefixed lines advance the new-file counter).
+    """
+    positions = {}
+    if not patch:
+        return positions
+    hunk_header_re = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@")
+    old_ln = new_ln = 0
+    in_hunk = False
+    for line in str(patch).split("\n"):
+        match = hunk_header_re.match(line)
+        if match:
+            old_ln = int(match.group(1))
+            new_ln = int(match.group(2))
+            in_hunk = True
+            continue
+        if not in_hunk or line.startswith("\\"):
+            continue
+        if line.startswith("+"):
+            positions[new_ln] = {"type": "new", "old_line": None}
+            new_ln += 1
+        elif line.startswith("-"):
+            old_ln += 1
+        elif line.startswith(" "):
+            positions[new_ln] = {"type": "context", "old_line": old_ln}
+            old_ln += 1
+            new_ln += 1
+    return positions
+
+
+def resolve_line_range_boundary(path_sha1, new_line, positions):
+    """Build a GitLab ``line_range`` boundary for ``new_line``.
+
+    Returns ``None`` when the line is not a resolvable new-file position, so the
+    caller can decline to attach ``line_range`` rather than emit a code GitLab
+    would reject. Added lines use ``<sha>_0_<new>`` with ``type: "new"``;
+    unchanged context lines use ``<sha>_<old>_<new>`` with their real old line.
+    """
+    entry = positions.get(new_line)
+    if entry is None:
+        return None
+    if entry["type"] == "new":
+        return {
+            "line_code": "%s_0_%d" % (path_sha1, new_line),
+            "type": "new",
+            "old_line": None,
+            "new_line": new_line,
+        }
+    old_line = entry["old_line"]
+    return {
+        "line_code": "%s_%d_%d" % (path_sha1, old_line, new_line),
+        "type": None,
+        "old_line": old_line,
+        "new_line": new_line,
+    }
+
+
+def build_line_range(diff, path, span):
+    """Resolve a multiline ``span`` into a GitLab ``line_range`` from ``diff``.
+
+    Returns ``None`` when the inventory lacks per-line positions for ``path`` or
+    either boundary cannot be resolved, letting the caller fall back to a
+    single-line position instead of posting a code GitLab cannot anchor.
+    """
+    positions = ((diff or {}).get("positions") or {}).get(path)
+    if not positions:
+        return None
+    path_sha1 = hashlib.sha1(path.encode("utf-8")).hexdigest()
+    start = resolve_line_range_boundary(path_sha1, span["start"], positions)
+    end = resolve_line_range_boundary(path_sha1, span["end"], positions)
+    if start is None or end is None:
+        return None
+    return {"start": start, "end": end}
+
+
 def classify_comment_against_diff(comment, diff):
     """Tri-state: ``"valid"`` | ``"invalid"`` | ``"unknown"``.
 
@@ -980,6 +1066,7 @@ class GitLabPoster:
         """Build a diff inventory from ``GET /merge_requests/:iid/diffs``."""
         known = set()
         files = {}
+        positions = {}
         complete = True
         per_page = 100
         max_pages = 30
@@ -1000,6 +1087,7 @@ class GitLabPoster:
                     ranges, ok = parse_diff_hunk_inventory(patch)
                     if ok:
                         files[new_path] = ranges
+                    positions[new_path] = build_new_line_positions(patch)
             if len(data) < per_page:
                 break
             page += 1
@@ -1009,7 +1097,8 @@ class GitLabPoster:
         if not known:
             complete = False
             log("[400-fallback] MR diff list came back empty; treating inventory as incomplete.")
-        return {"files": files, "known": known, "complete": complete}
+        return {"files": files, "known": known, "positions": positions,
+                "complete": complete}
 
 
 def make_poster(api_base, token, auth_header, config):
@@ -1050,7 +1139,7 @@ class DryRunPoster:
                 "is_rate_limit_exhausted": False}
 
     def get_mr_diffs(self):
-        return {"files": {}, "known": set(), "complete": False}
+        return {"files": {}, "known": set(), "positions": {}, "complete": False}
 
     def mr_url(self):
         return None
@@ -1254,6 +1343,7 @@ def publish(result, diff_refs, poster, config, sleep=_sleep):
         comment = it["comment"]
         path = comment.get("path", "")
         end_line = comment.get("end_line", 0)
+        span = comment_span(comment)
         if not path or not end_line:
             failed_comments.append({"comment": comment, "reason": NO_LINE_REASON})
             continue
@@ -1272,6 +1362,16 @@ def publish(result, diff_refs, poster, config, sleep=_sleep):
                 "head_sha": diff_refs["head_sha"],
             },
         }
+        if span is not None and span["multiline"]:
+            # Resolve the range from the real MR diff: a boundary's line_code
+            # needs the true old-file line unless the line is a pure addition.
+            # If the diff is unavailable or a boundary cannot be resolved, we
+            # omit line_range and let GitLab anchor the single end_line rather
+            # than reject a bogus code.
+            diff = get_diff_inventory(poster, diff_cache)
+            line_range = build_line_range(diff, path, span)
+            if line_range is not None:
+                discussion["position"]["line_range"] = line_range
         resp = poster.post_discussion(discussion, comment_id=it["id"])
         if resp.get("success"):
             stats["inline"] += 1

examples/gitlab_ci/post_review_test.py

@@ -20,6 +20,7 @@ Test seams:
      canned ``HTTPError`` sequences.
 """
 
+import hashlib
 import io
 import json
 import os
@@ -59,6 +60,13 @@ DIFF_REFS = {
     "head_sha": "ghi789",
 }
 
+
+def diff_inventory(path, lines):
+    """Build a diff inventory for ``path`` from ``{new_line: (type, old_line)}``."""
+    positions = {n: {"type": t, "old_line": o} for n, (t, o) in lines.items()}
+    return {"files": {path: []}, "known": {path},
+            "positions": {path: positions}, "complete": True}
+
 DEFAULT_CONFIG = {
     "success_delay": 2.0,
     "failure_delay": 1.0,
@@ -234,6 +242,24 @@ class FormatCommentTest(unittest.TestCase):
         self.assertIn("```suggestion:-0+0\nx = 2\n```", body)
         self.assertIn("**Suggestion:**", body)
 
+    def test_with_multiline_suggestion(self):
+        body = pr.format_comment(comment(content="fix this", existing_code="x = 1\ny = 2", suggestion_code="x = 2\ny = 3", start_line=5, end_line=6))
+        self.assertIn("fix this", body)
+        self.assertIn("```suggestion:-1+0\nx = 2\ny = 3\n```", body)
+        self.assertIn("**Suggestion:**", body)
+
+    def test_with_multiline_suggestion_no_start_line(self):
+        body = pr.format_comment(comment(content="fix this", existing_code="x = 1\ny = 2", suggestion_code="x = 2\ny = 3", start_line=None, end_line=5))
+        self.assertIn("fix this", body)
+        self.assertIn("```suggestion:-0+0\nx = 2\ny = 3\n```", body)
+        self.assertIn("**Suggestion:**", body)
+
+    def test_with_multiline_suggestion_no_end_line(self):
+        body = pr.format_comment(comment(content="fix this", existing_code="x = 1\ny = 2", suggestion_code="x = 2\ny = 3", start_line=5, end_line=None))
+        self.assertIn("fix this", body)
+        self.assertIn("```suggestion:-0+0\nx = 2\ny = 3\n```", body)
+        self.assertIn("**Suggestion:**", body)
+
     def test_suggestion_without_existing(self):
         body = pr.format_comment(comment(content="fix this", suggestion_code="x = 2"))
         self.assertNotIn("```suggestion", body)
@@ -385,7 +411,13 @@ class SafeFenceTest(unittest.TestCase):
 
 class PublishTest(unittest.TestCase):
     def test_inline_success_and_summary(self):
-        stats, rec = run_publish({"comments": [comment()]})
+        # A multiline span whose boundaries are added lines: line_range is
+        # resolved from the diff and both boundaries use the pure-addition code.
+        diffs = diff_inventory("main.py", {n: ("new", None) for n in range(5, 11)})
+        stats, rec = run_publish(
+            {"comments": [comment(start_line=5, end_line=10)]},
+            poster=Recorder(diffs=diffs),
+        )
         self.assertEqual(stats["inline"], 1)
         self.assertEqual(stats["failed"], 0)
         self.assertEqual(len(rec.disc_calls), 1)
@@ -394,10 +426,45 @@ class PublishTest(unittest.TestCase):
         inline = rec.disc_calls[0]
         self.assertEqual(inline["position"]["new_path"], "main.py")
         self.assertEqual(inline["position"]["new_line"], 10)
+        line_range = inline["position"]["line_range"]
+        expected_path_sha1 = hashlib.sha1(b"main.py").hexdigest()
+        self.assertEqual(line_range["start"]["line_code"], f"{expected_path_sha1}_0_5")
+        self.assertEqual(line_range["start"]["new_line"], 5)
+        self.assertEqual(line_range["start"]["type"], "new")
+        self.assertEqual(line_range["end"]["line_code"], f"{expected_path_sha1}_0_10")
+        self.assertEqual(line_range["end"]["new_line"], 10)
+
         self.assertIn("possible issue", inline["body"])
         self.assertIn("**1** issue(s)", rec.final_summary_body)
         self.assertIn("Successfully posted inline: 1 comment(s)", rec.final_summary_body)
 
+    def test_inline_multiline_context_boundary(self):
+        # The start boundary is an unchanged context line, so its line_code must
+        # carry the real old-file line number (not the pure-addition 0).
+        diffs = diff_inventory("main.py", {5: ("context", 4), 6: ("new", None)})
+        stats, rec = run_publish(
+            {"comments": [comment(start_line=5, end_line=6)]},
+            poster=Recorder(diffs=diffs),
+        )
+        self.assertEqual(stats["inline"], 1)
+        line_range = rec.disc_calls[0]["position"]["line_range"]
+        expected_path_sha1 = hashlib.sha1(b"main.py").hexdigest()
+        self.assertEqual(line_range["start"]["line_code"], f"{expected_path_sha1}_4_5")
+        self.assertEqual(line_range["start"]["type"], None)
+        self.assertEqual(line_range["start"]["old_line"], 4)
+        self.assertEqual(line_range["end"]["line_code"], f"{expected_path_sha1}_0_6")
+        self.assertEqual(line_range["end"]["type"], "new")
+
+    def test_inline_multiline_unresolvable_omits_line_range(self):
+        # With no diff positions for the file, we must not emit a bogus code;
+        # the comment still posts, anchored on the single end_line.
+        stats, rec = run_publish({"comments": [comment(start_line=5, end_line=10)]})
+        self.assertEqual(stats["inline"], 1)
+        self.assertEqual(stats["failed"], 0)
+        inline = rec.disc_calls[0]
+        self.assertEqual(inline["position"]["new_line"], 10)
+        self.assertNotIn("line_range", inline["position"])
+
     def test_fallback_when_diff_refs_none(self):
         stats, rec = run_publish({"comments": [comment()]}, diff_refs=None)
         self.assertEqual(stats["inline"], 0)
@@ -1206,6 +1273,45 @@ class LineResolutionTest(unittest.TestCase):
         self.assertEqual(pr.classify_comment_against_diff(comment(start_line=10, end_line=10), diff), "valid")
 
 
+class LinePositionTest(unittest.TestCase):
+    def test_build_new_line_positions_maps_added_and_context(self):
+        # @@ -1,3 +5,4 @@: new lines 5 (ctx), 6 (added), 7 (ctx); '-' advances
+        # only the old counter and creates no new-file position.
+        patch = "@@ -1,3 +5,4 @@\n ctx\n-gone\n+added\n ctx2\n"
+        positions = pr.build_new_line_positions(patch)
+        self.assertEqual(positions[5], {"type": "context", "old_line": 1})
+        self.assertEqual(positions[6], {"type": "new", "old_line": None})
+        # ctx2 is the old line after the removed one (old counter 2 -> 3).
+        self.assertEqual(positions[7], {"type": "context", "old_line": 3})
+
+    def test_build_new_line_positions_empty(self):
+        self.assertEqual(pr.build_new_line_positions(""), {})
+        self.assertEqual(pr.build_new_line_positions(None), {})
+
+    def test_resolve_boundary_added_line(self):
+        positions = {6: {"type": "new", "old_line": None}}
+        b = pr.resolve_line_range_boundary("sha", 6, positions)
+        self.assertEqual(b, {"line_code": "sha_0_6", "type": "new",
+                             "old_line": None, "new_line": 6})
+
+    def test_resolve_boundary_context_line(self):
+        positions = {5: {"type": "context", "old_line": 4}}
+        b = pr.resolve_line_range_boundary("sha", 5, positions)
+        self.assertEqual(b, {"line_code": "sha_4_5", "type": None,
+                             "old_line": 4, "new_line": 5})
+
+    def test_resolve_boundary_unknown_line(self):
+        self.assertIsNone(pr.resolve_line_range_boundary("sha", 99, {}))
+
+    def test_build_line_range_unresolvable_when_no_positions(self):
+        diff = {"positions": {}}
+        self.assertIsNone(pr.build_line_range(diff, "main.py", {"start": 5, "end": 10}))
+
+    def test_build_line_range_none_when_boundary_missing(self):
+        diff = diff_inventory("main.py", {5: ("new", None)})  # 10 absent
+        self.assertIsNone(pr.build_line_range(diff, "main.py", {"start": 5, "end": 10}))
+
+
 class FallbackPublishTest(unittest.TestCase):
     def test_400_line_failure_classifies_invalid(self):
         config = dict(DEFAULT_CONFIG)