feat(gitlab): update existing MR note when marker matches (ENG-3294)

[DavidS-ovm]

• Resolve author via GET /api/v4/user; paginate MR notes (newest first)
• Match non-system notes by same author and
• PUT to update; on non-2xx POST fallback with explanatory banner
• Warn when markdown lacks marker; extend cleanup for temp files

[carabasdaniel]

Code Review: feat(gitlab): update existing MR note when marker matches (ENG-3294)

Files: env0.plugin.yaml (+129/−23), README.md (+3/−1)
Summary: Instead of posting a new GitLab MR comment on every run, the plugin now resolves the current user, paginates existing MR notes, and if it finds a non-system note by the same author containing <!-- overmind-change-summary -->, it PUTs an update. On PUT failure it falls back to a new POST with an explanatory banner.

---

Security Review (P0)

No blocking findings.

Warning

1. Temp file GITLAB_USER_FILE not covered by cleanup trap. The existing cleanup trap (lines ~43–53) was extended to clean up GITLAB_NOTES_PAGE_FILE and GITLAB_FALLBACK_BODY_FILE, but the GITLAB_USER_FILE created at the start of the new logic is only cleaned up inline. If the script is interrupted (e.g., SIGKILL from env0 timeout) between mktemp and the inline rm -f, the file containing the /api/v4/user response (which includes the GitLab user ID, username, email, and avatar URL) will remain on the ephemeral runner filesystem. In CI runners this is very low risk since the filesystem is typically discarded, but for consistency with the other temp files it should be tracked in the trap or assigned to a variable that the trap handles.

2. Token scope assumption. The GET /api/v4/user call requires the token to have at least read_user scope, and PUT …/notes/:id requires api scope. If the token only has api scope (which includes read_user), this is fine. Worth documenting the minimum required scopes in the README so users don't hit opaque 401s.

---

Architecture / Scope Review (P1 — Advisory)

1. Missing marker injection or warning. The PR description states "Warn when markdown lacks marker", but no such warning exists in the diff. The OVERMIND_MARKER='<!-- overmind-change-summary -->' is used to match existing notes, but the plugin never:

   • checks whether MARKDOWN_FILE itself contains the marker, nor
   • injects the marker into the markdown body before posting.

   This means the feature silently relies on the upstream markdown generation (from the Overmind API / CLI wait-for-simulation output) already containing the marker. If the upstream markdown does not contain the marker, every run will POST a new comment and the update-in-place logic will never trigger. Recommend either:

   • (a) Appending the marker to the body before posting, or
   • (b) Adding the promised warning: grep -qF "${OVERMIND_MARKER}" "${MARKDOWN_FILE}" || echo "Warning: ...", or
   • (c) Documenting the contract explicitly — that the upstream must include the marker.

   This is the most important finding in this review. Without it the feature is fragile and can silently degrade to the old duplicate-comment behavior.

2. No corresponding GitHub update-in-place. The GitHub comment path still uses gh pr comment --body-file which always creates a new comment. This is fine if GitHub is intentionally deferred to a future PR, but it creates an asymmetry between providers. Consider at least a tracking comment or issue.

---

DevOps / Deployment Review (P1 — Advisory)

1. No rollback strategy mentioned. If the updated plugin breaks MR commenting, the only rollback is to pin the env0 plugin to a previous version. Consider noting in the README how users can pin a specific version.

2. Pagination correctness. The loop uses sort=desc&order_by=created_at, which is correct for finding the most recent matching note first. The termination conditions (empty page, page smaller than per_page) are correct. The only subtle case is if a note is deleted between page fetches, but that would just result in a missed match and a new POST — acceptable behavior.

---

Frontend Review — N/A

Database Review — N/A

---

Summary

#	Severity	Group	Finding
1	Warning	Security	GITLAB_USER_FILE not in cleanup trap
2	Warning	Security	Minimum GitLab token scopes undocumented
3	Advisory (important)	Architecture	Missing marker injection/warning — feature silently degrades if upstream markdown lacks marker
4	Advisory	Architecture	GitHub path not updated for parity
5	Advisory	DevOps	No rollback/version-pinning guidance

Overall: The core logic is well-structured — the pagination, jq matching, PUT-with-fallback pattern, and graceful degradation are solid. Finding #3 (marker presence) is the key item: without verifying or injecting the marker, the feature's contract is implicit and fragile. I'd recommend addressing that before merging.

[carabasdaniel]

Re-review after update (89061cd)

All five findings from the previous review have been addressed:

#	Original Finding	Status	How
1	GITLAB_USER_FILE not in cleanup trap	Resolved	Added to trap, initialized to "", cleared after inline rm -f
2	Minimum GitLab token scopes undocumented	Resolved	New "GitLab token scopes" paragraph in Self-Hosted GitLab section, plus updated token creation instructions
3	Missing marker injection/warning	Resolved	grep -qF check + warning + printf append before any API call — exactly option (a)+(b) from the original review
4	GitHub path not updated for parity	Acknowledged	README now explicitly states GitHub still posts new comments and update-in-place is not yet implemented
5	No rollback/version-pinning guidance	Resolved	New "Pinning plugin version (rollback)" section with @<ref> syntax and link to env0 docs

Fresh scan of updated diff

Security (P0): No new findings. Temp file lifecycle is now fully covered by the trap for all six temp file variables. Token is not exposed in any new way.

Architecture (P1): The marker check + auto-append is placed early in the gitlab) case (before URL construction and API calls), which is the right position. The new README section "GitLab merge request comments" clearly documents the marker contract and the auto-append fallback.

No blocking or warning findings remain. LGTM.