Merge dev into main: GitLab diff-aware review anchors and note fallback - #125
Conversation
gitlab-post-review sent `new_line`-only positions for every RIGHT-side
comment, but GitLab requires both line numbers for unchanged context
lines and refuses lines outside the diff entirely, so those findings
400'd and the job hard-failed (seen on a customer MR where both
approved comments targeted untouched code).
- Build a per-file line index from the MR changes and shape each
position to the line's role: added -> new_line, removed -> old_line,
context -> both.
- Post comments whose line is not in the diff (or that GitLab refuses)
as plain MR notes instead of dropping them; only comments failing
both routes count as failures, and the job only hard-fails when
nothing reached the MR at all.
- Surface the GitLab error response body in GitlabApiError messages
("400 Bad Request" alone was undebuggable from CI logs).
- Track fallback counts in review_post_results.json and the sticky
tracking note.
- Tell Pass 1/Pass 2 prompts (GitHub + GitLab) that anchors must be
lines visible in the diff.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…wording - validator prompt: mention removed/LEFT lines as valid anchors and what `side` means for the line anchor - ReviewPostOutcome.failed doc + tracking note: `failed` now means both inline and note fallback failed, not an anchoring refusal - fallbackNoteBody: use old_path for LEFT-side comments and drop the "outside the MR diff" claim (API refusals also land here) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
fix(gitlab): diff-aware anchors and note fallback for review posting
|
Droid finished @factory-nizar's task —— View job The diff-aware GitLab anchoring changes look solid and are well-covered by tests. Remaining follow-ups are prompt wording around LEFT-side anchors, slightly misleading tracking-note text for fallback notes, and minor hardening for Markdown injection in fallback-note locations. |
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| 'Relative file path (use the new_path from the diff, e.g., "src/index.ts")', | ||
| lineFieldDescription: | ||
| "Target line number in the new file (single-line) or end line number (multi-line). Must be ≥ 0.", | ||
| "Target line number in the new file (single-line) or end line number (multi-line). Must be ≥ 0. " + |
There was a problem hiding this comment.
[P2] Mention LEFT-side anchors (removed lines) in GitLab terminology
This still says the anchor is "in the new file" and only mentions added/context lines, but the posting code now supports side: "LEFT" anchors to old-file (removed) lines. Updating this description will help the model emit correct anchors and reduce fallback-to-note.
| "Target line number in the new file (single-line) or end line number (multi-line). Must be ≥ 0. " + | |
| lineFieldDescription: | |
| "Target line number (single-line) or end line number (multi-line). Must be ≥ 0. " + | |
| "Must be a line that appears in the MR diff (added, removed, or context inside a hunk). " + | |
| "Use side: \"RIGHT\" to anchor to the new-file line and side: \"LEFT\" to anchor to the old-file line. " + | |
| "A finding about untouched code should anchor to the nearest related changed line instead.", |
| counts.push( | ||
| `${review.fallbackPosted} posted as ${ | ||
| review.fallbackPosted === 1 ? "a regular note" : "regular notes" | ||
| } (line outside the diff)`, |
There was a problem hiding this comment.
[P2] Don’t attribute all fallback notes to out-of-diff lines
fallbackPosted counts any case where inline posting fails but the MR note fallback succeeds, including API refusals that aren’t strictly "line outside the diff". The tracking note text currently hard-codes the out-of-diff explanation.
| } (line outside the diff)`, | |
| `${review.fallbackPosted} posted as ${ | |
| review.fallbackPosted === 1 ? "a regular note" : "regular notes" | |
| } (could not be posted inline)`, |
| ): string { | ||
| const filePath = | ||
| comment.side === "LEFT" ? (comment.old_path ?? comment.path) : comment.path; | ||
| const location = line !== null ? `${filePath}:${line}` : filePath; |
There was a problem hiding this comment.
[P2] [security] Sanitize/validate file paths in fallback MR notes
fallbackNoteBody() interpolates comment.path/comment.old_path into a Markdown inline-code span without escaping, but parseValidatedReview() accepts any non-empty string. If the validated JSON is prompt-injected, a path containing backticks or newlines can break out of the code span and inject arbitrary Markdown into the bot-posted MR note. Consider validating paths against the MR changes list (or at least stripping control chars/backticks) before formatting location.
1 participant
Ships #124 to main (the only delta between dev and main):
new_line, removed →old_line, context → both) — fixes the customer-reportedGitLab API 400: Bad Requeston context-line anchorsValidation: 577 unit tests, typecheck, prettier, review feedback addressed, plus a live e2e replication on a scratch GitLab project (old code reproduced the 400s byte-for-byte; fixed code posted every anchor type correctly).
Part of VAL-14