feat(publish): add ocr publish gitflic to post reviews to GitFlic MRs#138
feat(publish): add ocr publish gitflic to post reviews to GitFlic MRs#138revenant20 wants to merge 3 commits into
ocr publish gitflic to post reviews to GitFlic MRs#138Conversation
Add native publishing of review results to GitFlic merge requests: - internal/publish/gitflic: minimal REST client (discussions/create), new-to-old line mapping built on diff.ParseHunks (GitFlic requires all four of newLine/oldLine/newPath/oldPath for an inline code comment), and a publisher that posts inline discussions, a fallback note for comments that cannot be placed inline, and a summary note. - cmd/opencodereview: 'ocr publish gitflic' subcommand; flags default to the predefined GitFlic CI variables (CI_PROJECT_NAMESPACE, CI_PROJECT_NAME, CI_MERGE_REQUEST_LOCAL_ID, CI_COMMIT_SHA), token via GITFLIC_TOKEN, --dry-run for offline debugging. - examples/gitflic_ci: merge request pipeline demo using the new command.
Gated behind the gitflic_e2e build tag (regular 'make test' is unaffected) and skipped when GITFLIC_TOKEN is unset. The test provisions a throwaway project over REST, seeds master/feature branches with a modified line, an inserted line and a new file, opens a merge request, publishes a synthetic review result, and asserts the discussions GitFlic actually stored: inline positions (including the old-line anchors), the fallback note and the summary note. The project is deleted on cleanup. Defaults target a local GitFlic CE (localhost:8080); the instance, git credentials and API URL are configurable via GITFLIC_E2E_* variables. Run with 'make test-e2e-gitflic'.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - | | ||
| ocr config set llm.url $OCR_LLM_URL | ||
| ocr config set llm.auth_token $OCR_LLM_AUTH_TOKEN | ||
| ocr config set llm.model $OCR_LLM_MODEL |
There was a problem hiding this comment.
Bug: OCR_LLM_MODEL is documented as an optional CI/CD variable (line 17), but this command runs unconditionally. When the variable is not set, the shell expands $OCR_LLM_MODEL to an empty string, causing ocr config set llm.model "" to write an empty model into the config. This will then fail endpoint resolution because the resolver requires a non-empty model value (ep.Model != "" in resolver.go).
Consider guarding this command so it only runs when the variable is set, for example:
- |
if [ -n "$OCR_LLM_MODEL" ]; then
ocr config set llm.model $OCR_LLM_MODEL
fiThe same concern applies to llm.extra_body below if it were ever made optional.
There was a problem hiding this comment.
Thanks! Kept this unconditional to match the gitlab/github examples (they set llm.model the same way). If OCR_LLM_MODEL is unset the unquoted var just expands to nothing, so config set errors out instead of writing an empty model — and a model is required everywhere anyway, so a guard would only move the failure to ocr review.
| NewLine int `json:"newLine,omitempty"` | ||
| OldLine int `json:"oldLine,omitempty"` |
There was a problem hiding this comment.
The omitempty tag on int fields means that when NewLine or OldLine is 0, the field will be completely omitted from the JSON payload. Per the struct's own documentation, if any of these four fields is missing, GitFlic silently creates a general comment instead of an inline one.
While the current internal usage in publisher.go guards against this (filtering out EndLine <= 0 and using clampLine to ensure oldLine >= 1), the Discussion struct is exported as part of the public API. Any external consumer who sets NewLine: 0 or OldLine: 0 would get silent, incorrect behavior with no error.
Consider using pointer types (*int) with omitempty so that an explicit zero value is serialized, while nil still omits the field. Alternatively, remove omitempty from the int fields since they are always expected to be present for inline comments.
Suggestion:
| NewLine int `json:"newLine,omitempty"` | |
| OldLine int `json:"oldLine,omitempty"` | |
| NewLine *int `json:"newLine,omitempty"` | |
| OldLine *int `json:"oldLine,omitempty"` |
| var failed []model.LlmComment | ||
| for _, c := range result.Comments { | ||
| d, ok := byPath[c.Path] | ||
| if !ok || d.IsBinary || c.EndLine <= 0 { |
There was a problem hiding this comment.
Bug: Deleted files are not excluded from inline comment posting. When d.IsDeleted is true, the file no longer exists in the new version, so NewPath may be /dev/null or empty and there are no new-side lines to anchor to. This will likely cause the GitFlic API to reject the discussion or create a malformed one. Add d.IsDeleted to the guard condition alongside d.IsBinary.
Suggestion:
| if !ok || d.IsBinary || c.EndLine <= 0 { | |
| if !ok || d.IsBinary || d.IsDeleted || c.EndLine <= 0 { |
| // GitFlic has no old side for a new file; anchor to the new path. | ||
| oldPath = d.NewPath | ||
| } else { | ||
| oldLine = oldLineFor(diff.ParseHunks(d.Diff), c.EndLine) |
There was a problem hiding this comment.
Performance: diff.ParseHunks(d.Diff) is called for every comment inside the loop. When multiple comments reference the same file, the same diff text is re-parsed each time. Consider pre-computing a map of parsed hunks keyed by file path before the loop to avoid redundant parsing.
| d, ok := byPath[c.Path] | ||
| if !ok || d.IsBinary || c.EndLine <= 0 { | ||
| failed = append(failed, c) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Observability: When a comment's c.Path does not match any entry in the diffs map (!ok), the comment is silently added to the failed list without any log message explaining why. This makes debugging path mismatches difficult. Consider adding a diagnostic log (similar to the one at line 126) when the path lookup fails.
Suggestion:
| d, ok := byPath[c.Path] | |
| if !ok || d.IsBinary || c.EndLine <= 0 { | |
| failed = append(failed, c) | |
| continue | |
| } | |
| d, ok := byPath[c.Path] | |
| if !ok { | |
| p.logf("no diff found for path %s, falling back to summary", c.Path) | |
| failed = append(failed, c) | |
| continue | |
| } | |
| if d.IsBinary || d.IsDeleted || c.EndLine <= 0 { | |
| failed = append(failed, c) | |
| continue | |
| } |
The PR's own OpenCodeReview run flagged a few items on the gitflic publisher; apply the ones that improve clarity/robustness without changing behavior for real inputs: - client.go: make Discussion.NewLine/OldLine *int so the nullable wire fields are explicit (absent vs zero) rather than relying on omitempty over a plain int - publisher.go: skip deleted files in the inline guard (also covered by the no-diff path, but makes the intent explicit) - publisher.go: parse each file's diff hunks once instead of per comment - publisher.go: log when a comment's path has no diff and is folded into the summary note
1 participant
Implements #102.
What
First-class support for publishing OCR review results to GitFlic merge
requests, mirroring the existing GitHub Actions / GitLab CI examples (#14):
ocr publish gitflic— readsocr review --format jsonoutput andposts it onto an MR as inline discussions, with a fallback note for
comments that can't be placed inline and a summary note. Auto-detects the
MR context from GitFlic's predefined CI variables (
CI_PROJECT_NAMESPACE,CI_PROJECT_NAME,CI_MERGE_REQUEST_LOCAL_ID,CI_COMMIT_SHA); token viaGITFLIC_TOKEN;--dry-runfor offline debugging.examples/gitflic_ci/— a merge-request pipeline demo, same shape asexamples/gitlab_ci/.Why a native command (rather than CI glue)
For GitHub/GitLab the posting logic lives in the example YAML, which works
well. GitFlic's Discussions API differs in one important way: an inline code
comment requires all four of
newLine,oldLine,newPath,oldPath—if any is missing it silently falls back to a general (non-inline) comment.
ocr reviewreports positions on the new-file side only, so the old-sideline has to be computed from the diff hunks.
A CI script would have to reimplement unified-diff parsing to do that. OCR
already has this machinery (
internal/diff.ParseHunks+ the merge-base diffprovider used by the review itself), so doing the mapping inside OCR keeps the
line numbers guaranteed-consistent with the review that produced them — in the
spirit of the project's deterministic-engineering design.
Implementation
internal/publish/gitflic/— stdlib-only REST client (client.go),new→old line mapping on top of
internal/diff(linemap.go), and thepublisher with inline/fallback/summary logic (
publisher.go).cmd/opencodereview/publish_cmd.go+ registration inmain.go.Testing
httptest).make test-e2e-gitflic, build taggitflic_e2e, skippedwithout
GITFLIC_TOKEN) against a live GitFlic instance: provisions athrowaway project, seeds a modified line / inserted line / new file, opens
an MR, publishes a synthetic review and asserts the discussions GitFlic
actually stored — inline positions (incl. old-line anchors), the fallback
note and the summary note. Regular
make testis unaffected.Note
This puts review-posting in the core CLI, whereas for GitHub/GitLab it has
lived in the example scripts. I went this route for the diff-parsing reason
above, but I'm happy to adapt if you'd prefer to keep posting out of the core.
Discussion in #102.