Repository navigation
fix(#8174)!: normalize review-thread position fields across forges - #8693
fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
ReviewThread exposed only line and original_line, but the two forges filled them with different meanings. GitHub used them for the current and creation-time line on an unfetched diff side, while GitLab put new_line and old_line into them. A GitLab removed-line discussion therefore looked like an outdated GitHub thread, and consumers could not match threads by path and line across forges. Define one position contract on forge.ReviewThread: side, line, start_side, start_line, original_line, original_start_line, old_line, new_line and is_outdated, reusing the entity-context-snapshot reviews vocabulary. The GitHub query now fetches diffSide, startDiffSide, startLine, originalStartLine and isOutdated, and maps the current line to old_line or new_line by side. The GitLab fetcher keeps old_line and new_line as reported, derives side and line from them, reads line_range for multi-line starts, and leaves is_outdated and the original_* fields null because GitLab does not expose them. The fake client deep-copies the new pointer fields, and the CLI internals guide documents the contract and the migration. Pre-commit could not fetch remote hook repositories in the sandbox (TLS verification failure). The pre-commit-hooks checks (v6.0.0), gofmt, go vet, lint-broken-symlinks, lint-docs-links and lychee were run directly and passed. scan-secrets replaced gitleaks. BREAKING CHANGE: fetch-review-threads output on GitLab changed. Read the old-file line from old_line instead of original_line, which is now always null, and the new-file line from new_line instead of line. For removed-line discussions, line now holds the old-file line with side "left" instead of null. GitHub line and original_line keep their meaning. Closes #8174 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 8:14 AM UTC · Completed 8:27 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $3.10 |
Site previewPreview: https://11bc7759-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
<!-- fullsend:risk-assessment --> DetailsBreaking API change across forge review-thread position fields in frequently churned files, balanced by zero protected path changes, high test ratio, and exact coverage of the linked issue acceptance criteria. Previous run<!-- fullsend:risk-assessment --> DetailsBreaking API change across forge review-thread position fields in frequently churned files, balanced by zero protected path changes, high test ratio, and exact coverage of the linked issue acceptance criteria. |
ReviewFindingsLow
Earlier findings
Previous run<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6ImludGVybmFsL2ZvcmdlL2dpdGh1Yi9naXRodWIuZ28iLCJpZCI6ImZfMDk4NjZhNTU1NDNjZDQ3NiIsImxpbmUiOjQwMDIsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImFwaS1jb250cmFjdCIsImZpbGUiOiJkb2NzL2d1aWRlcy9kZXYvY2xpLWludGVybmFscy5tZCIsImlkIjoiZl8zMmQzODFlNWQ3OGIxZWIzIiwibGluZSI6MjQ0LCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJwYXR0ZXJuLWluY29uc2lzdGVuY3kiLCJmaWxlIjoiaW50ZXJuYWwvZm9yZ2UvZ2l0bGFiL21ldGhvZHNfdGVzdC5nbyIsImlkIjoiZl81ZDYwNzljZDM3ZmRjMjFhIiwibGluZSI6MjYyLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJhcmNoaXRlY3R1cmFsLWNvbmZsaWN0IiwiZmlsZSI6ImludGVybmFsL2ZvcmdlL2ZvcmdlLmdvIiwiaWQiOiJmXzZhYWM1YzgyZjE5YzFmOTQiLCJsaW5lIjo2NTMsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJiYWNrd2FyZC1pbmNvbXBhdGlibGUiLCJmaWxlIjoiaW50ZXJuYWwvZm9yZ2UvZ2l0bGFiL21yLmdvIiwiaWQiOiJmXzA1OTIyYTY5ZTE4ZjNlNTAiLCJsaW5lIjo5NjIsImFjdGlvbmFibGUiOmZhbHNlfV19 --> ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 8:29 AM UTC · Completed 8:34 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.33 |
Keep StartSide when GitHub reports startDiffSide even if startLine is null (outdated multi-line threads) and cover it with a test. Document original_start_line as belonging to start_side in the position contract and the ReviewThread field comments. Track the GitLab position-contract test route with handleTracked. Not changed: the entity-context-snapshot vocabulary note (no claim in the code or docs) and the GitLab line-meaning migration (cross-repo coordination). pre-commit could not fetch hook repos (certificate failure); ran gofmt, go vet, interface-doc-sync and whitespace checks directly instead. Addresses #8693 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
🔧 Fix agent — iteration 1 (bot-triggered)Fixed the three actionable findings: StartSide handling for outdated multi-line GitHub threads (with a new test), original_start_line side documentation in the contract and field comments, and tracked handler registration in the GitLab test. Recorded disagreements for the two non-actionable findings. Fixed (3):
Disagreed (2):
Tests: passed CI inspections (6):
Conflict update: skipped; strategy
|
|
🤖 Finished Review · ✅ Success · Started 8:36 AM UTC · Completed 8:47 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.30 |
Superseded by updated review
| // diff. Nil when the thread no longer maps onto the current diff (for | ||
| // example an outdated GitHub thread) or has no line position. | ||
| Line *int `json:"line"` | ||
| // StartSide and StartLine give the first line of a multi-line range. |
There was a problem hiding this comment.
[low] architectural-conflict
The PR description says the new ReviewThread fields reuse the entity-context-snapshot reviews vocabulary, but reviews.schema.json defines original_side and has no start_side, old_line or new_line. The code and docs make no such claim; the mismatch is in the PR description. Not changed by the latest push.
Suggested fix: Reword the PR description to say the fields adapt that vocabulary, or align the names with the schema.
| } | ||
| thread.Line = note.Position.NewLine | ||
| thread.OriginalLine = note.Position.OldLine | ||
| thread.OldLine = note.Position.OldLine |
There was a problem hiding this comment.
[low] backward-incompatible
On GitLab, line changes meaning without a key change: removed-line discussions now report the old-file line in line with side left, and original_line is always null. Documented with a breaking-change marker and migration note; remaining risk is external consumers still reading the old fields.
Suggested fix: Coordinate the fullsend-ai/agents migration to old_line and new_line with this release.
fullsend-ai-review
Bot
added
the
ready-for-merge
Summary
This PR replaces the overloaded
line/original_linefields onforge.ReviewThreadwith one position contract that means the same thing on GitHub and GitLab. Consumers offetch-review-threadscan now tell apart:Breaking (GitLab output): the old-file line moves from
original_linetoold_line. A removed-line discussion now reports its old-file line inline, withside: "left". GitHublineandoriginal_linekeep their meaning.Related Issue
Follow-up from #7907.
Changes
internal/forge/forge.go: adds aDiffSidetype (left/right).ReviewThreadnow hasside,line,start_side,start_line,original_line,original_start_line,old_line,new_lineandis_outdated. Each field is documented in the struct, and every one isnullwhen the forge does not report it. The names reuse the vocabulary of the entity-context-snapshotreviews.schema.json.internal/forge/github/github.go):diffSide,startDiffSide,startLine,originalStartLineandisOutdated.lineis also copied toold_lineornew_line, depending on the side.internal/forge/gitlab/mr.go):old_lineandnew_lineare kept exactly as GitLab reports them.sideandlinecome from them: right if a new line exists, otherwise left.line_range.startis read for multi-line ranges. A range that covers only one line is not treated as multi-line.is_outdated,original_lineandoriginal_start_lineare alwaysnull, because GitLab does not expose them (as the maintainer confirmed on the issue).docs/guides/dev/cli-internals.md): adds a table mapping each field to its GitHub and GitLab source, notes on matching (comparepath+new_line), and a migration note for consumers of the old fields.Testing
line: null,is_outdated: true), a multi-line thread, and a check that the query asks for the new fields.new_line: null,is_outdated: null), a multi-line range on one side and across sides, a single-lineline_range, a general (no position) discussion, and table tests forgitlabSideLine.go test ./internal/forge/...passes, and so do thefetch-review-threadsCLI tests. Patch coverage on the changed functions is 98.9–100%.make lint:pre-commitcould not fetch its remote hook repos in the sandbox (TLS verification failure). I ran the relevant hooks directly instead: pre-commit-hooks v6.0.0 (EOF, whitespace, merge-conflict, mixed line endings, private key),gofmt,go vet ./...,lint-broken-symlinks,lint-docs-linksandlychee --offline. All passed.scan-secretsstood in for gitleaks.internal/clitests (TestStartFetchService_*,TestPostScriptEnv_NoSchema*) fail in the sandbox both on cleanmainand with this change, so they are not related to it.Migration for consumers
The agents-side forwarding (fullsend-ai/agents
review-ops.lib.sh) matches dismissals by finding ID, so it does not break. If it forwards GitLaboriginal_line, it should switch toold_line, and usenew_linefor head-file line matching.Checklist
!for breaking changes)🤖 Generated with Claude Code
Closes #8174
Post-script verification
agent/8174-normalize-review-thread-positions)35c8bf8dc98dd91abc582bfd22fab9e5ce905d47..HEAD)