Bug/STC-652: Match GitHub/GitLab records by webhook URL to fix incorrect MR/PR linking

[akabiru]

Ticket

https://community.openproject.org/wp/STC-652

Continues the community contribution in #21932 by @foxweb.

What are you trying to accomplish?

When several GitHub or GitLab repositories are connected to one instance, merge/pull requests and issues can link to the wrong work packages. GitLab numbers them per repository — every repository has a !1 — and lookups were keyed on that number, so a webhook from one repository could match and overwrite a record belonging to another. For users nothing changes visually; the MR/issue/pipeline entries on a work package are simply always the right ones, no matter how many repositories are connected.

What approach did you choose and why?

Matching now keys on the resource URL alone, since the URL is the only globally unique identifier stored on every record. The database enforces that uniqueness, and the migration folds any pre-existing duplicate rows into the newest one so existing links and history are preserved.

Matching on the per-repository number as well wouldn't work: records created from comments before the entity is synced carry only a URL, so a combined match would duplicate them instead of completing them when the full webhook arrives.

A few deliberate exceptions follow provider semantics. Pipelines resolve through the merge request they belong to, which keeps fork pipelines pointing at the right work package. GitHub still falls back to its globally unique id when a URL misses, so a pull request stays linked across a repository rename or transfer. GitLab gets no such fallback, since its numbers repeat per project — the very collision this PR removes.

How it was tested

Reproduced end to end against a live GitLab 19 with two repositories webhooked to one instance, both carrying !1/#1: on current dev one repository's webhook takes over the other's record; on this branch all flows stay separate — opens, updates, comment references, partial-record completion and merge-request pipelines, in both classic and semantic work-package identifier modes. The migration was exercised against seeded duplicate data.

Merge checklist

• Added/updated tests
• Added/updated documentation in Lookbook — N/A
• Tested major browsers — N/A (backend only)

Closes #21932

[github-actions]

Deploying openproject with ⚡ PullPreview

Field	Value
Latest commit	aaa43ac
Job	deploy
Status	✅ Deploy successful
Preview URL	https://pr-23614-stc-652-communit-ip-157-90-247-104.my.opf.run:443

View logs

[Copilot]

Pull request overview

This PR fixes incorrect cross-repository linking of GitHub pull requests and GitLab merge requests/issues by switching lookups to use the webhook HTML URL as the globally-unique identifier (instead of per-repository numeric IDs), and adds database-level uniqueness enforcement to prevent duplicates.

Changes:

• Update GitHub/GitLab lookup paths (models + webhook handlers) to match records by *_html_url only.
• Add a migration to deduplicate existing rows by URL and introduce unique DB indexes on URL columns.
• Adjust/extend specs to cover same-number-different-repo scenarios and reflect the new URL-centric matching.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
modules/gitlab_integration/spec/models/gitlab_merge_request_spec.rb	Updates finder specs to assert URL-only matching and cross-repo iid collision behavior.
modules/gitlab_integration/spec/models/gitlab_issue_spec.rb	Updates finder specs to assert URL-only matching and cross-repo iid collision behavior.
modules/gitlab_integration/spec/lib/open_project/gitlab_integration/notification_handler/pipeline_hook_spec.rb	Ensures pipeline hook associates pipelines via repository-specific MR URL reconstruction.
modules/gitlab_integration/spec/lib/open_project/gitlab_integration/notification_handler/merge_request_hook_spec.rb	Updates handler spec to pass URL to the finder and persist correct gitlab_html_url.
modules/gitlab_integration/spec/lib/open_project/gitlab_integration/notification_handler/issue_hook_spec.rb	Updates handler spec to pass URL to the finder and persist correct gitlab_html_url.
modules/gitlab_integration/spec/features/work_package_gitlab_merge_request_activity_spec.rb	Aligns feature spec payload to use a shared GitLab MR HTML URL.
modules/gitlab_integration/spec/features/work_package_gitlab_issue_activity_spec.rb	Aligns feature spec payload to use a shared GitLab issue HTML URL.
modules/gitlab_integration/lib/open_project/gitlab_integration/notification_handler/pipeline_hook.rb	Rebuilds MR URL from project web URL + iid and finds MR by gitlab_html_url.
modules/gitlab_integration/lib/open_project/gitlab_integration/notification_handler/note_hook.rb	Switches issue lookup to URL-only and memoizes nil safely.
modules/gitlab_integration/lib/open_project/gitlab_integration/notification_handler/merge_request_hook.rb	Switches MR lookup to URL-only and memoizes nil safely.
modules/gitlab_integration/lib/open_project/gitlab_integration/notification_handler/issue_hook.rb	Refactors note generation and switches issue lookup to URL-only.
modules/gitlab_integration/lib/api/v3/gitlab_merge_requests/gitlab_merge_requests_by_work_package_api.rb	Normalizes file header to frozen_string_literal.
modules/gitlab_integration/lib/api/v3/gitlab_merge_requests/gitlab_merge_request_collection_representer.rb	Normalizes file header to frozen_string_literal.
modules/gitlab_integration/lib/api/v3/gitlab_issues/gitlab_issues_by_work_package_api.rb	Normalizes file header to frozen_string_literal.
modules/gitlab_integration/lib/api/v3/gitlab_issues/gitlab_issue_collection_representer.rb	Normalizes file header to frozen_string_literal.
modules/gitlab_integration/app/workers/cron/clear_old_merge_requests_job.rb	Normalizes file header to frozen_string_literal.
modules/gitlab_integration/app/models/gitlab_user.rb	Updates validations to modern validates ..., presence: true.
modules/gitlab_integration/app/models/gitlab_pipeline.rb	Updates validations to modern validates ..., presence: true.
modules/gitlab_integration/app/models/gitlab_merge_request.rb	Makes finder URL-required and URL-only; modernizes validations.
modules/gitlab_integration/app/models/gitlab_issue.rb	Makes finder URL-required and URL-only; modernizes validations.
modules/github_integration/spec/models/github_pull_request_spec.rb	Updates finder specs to assert URL-only matching.
modules/github_integration/spec/lib/open_project/github_integration/services/upsert_pull_request_spec.rb	Updates upsert specs to ensure partial PRs are promoted via URL match (no duplicates).
modules/github_integration/spec/lib/open_project/github_integration/notification_handler/pull_request_spec.rb	Updates webhook handler specs to use URL-based behavior and new IDs/URLs.
modules/github_integration/spec/lib/open_project/github_integration/notification_handler/issue_comment_spec.rb	Normalizes file header to frozen_string_literal.
modules/github_integration/lib/open_project/github_integration/notification_handler/pull_request.rb	Switches already-referenced lookup to URL-only (but needs a nil-safe fix).
modules/github_integration/app/models/github_pull_request.rb	Makes finder URL-required and URL-only; modernizes validations.
db/migrate/20260608120000_add_unique_index_to_integration_html_urls.rb	Deduplicates integration rows by URL and adds unique indexes on URL columns.

[akabiru]

Hi @foxweb please add your CLA acknowledgement on this PR as well (while you're at it, feel free to review! 😄 )

[github-actions]

Warning

Flaky specs

• rspec ./spec/features/work_packages/details/inplace_editor/subject_editor_spec.rb[1:2:1:1:2]

[foxweb]

Hi @akabiru!

I have read the CLA Document and I hereby sign the CLA.

A lot of work has been done! But I think it was worth merging my changes first, and then adding your changes. Otherwise, it's very difficult for me to do a review to see the diff between my commits and yours.

I'm reviewing your commits, it looks legit! I also wanted to suggest linking MR/PR by unique url, but preferred a tiny code fix. First of all, I made this fix for myself and my company. Your solution is deeper, more complete and involves database migration and conversion of old numbers.

Maybe it makes sense to write new Gitlab/Github modules altogether, with a new linking system and new regex pattern, and leave the old modules as legacy.

Also, in our production instance, we have already made our own fork of the GitLab module, in which we independently implemented a new regex-pattern JIRA-like codes as PROJECT-1234. When the mainstream module is in the official version, we will replace our fork with the official updated module.

P.S. I'm not ready to review Copilot's comments.

[akabiru]

Thanks for signing the CLA and for taking the time to look it over, @foxweb - really appreciate it! 🙏

No pressure at all on a detailed review - a quick scan to sanity-check the direction (or flag anything I've missed) would be more than enough. 🙂 The work will still go through our own team reviews.

I hear you on the review difficulty, and I'm sorry it's awkward to diff against your original commits. The tricky part is that this PR deliberately takes a different resolution strategy than your fix, which I mentioned in #21932 (review).

Because that reshapes the foundation rather than just extending it, merging your PR first and layering on top would not be ideal, since it would ship an intermediate state into the dev branch. On the practical side, since your PR comes from a fork, I don't think GitHub lets us stack on it while keeping CI authorization and the preview environment working - which is the primary reason I rebuilt it as a single unit here, with you credited as co-author of the foundation.

FWIW, project-based semantic IDs are already supported on the current GitLab/GitHub modules (Ex. see #23566 (comment))

Cheers,
Kabiru

[github-actions]

Warning

Flaky specs

• rspec ./spec/features/projects/persisted_lists_sharing_spec.rb[1:2:4:2:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #23614, linked for reference only):

- `rspec ./spec/features/projects/persisted_lists_sharing_spec.rb[1:2:4:2:1]`

Treat this as a standalone task, unrelated to PR #23614. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23614 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @akabiru to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @akabiru, and request a review from @akabiru.
On every commit, set @akabiru as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

[machisuji]

The implementation looks sensible to me, addressing the issue with the duplicate IDs in gitlab specifically.

So this could be merged as is. I do have some comments, but some might consider most of them to be nit-picking. Feel free to address them, nor not.

[github-actions]

Warning

Flaky specs

• rspec ./modules/gantt/spec/features/timeline/timeline_dates_spec.rb[1:2:1]
• rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:2]
• rspec ./spec/features/projects/lists/columns_spec.rb[1:2:1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #23614, linked for reference only):

- `rspec ./modules/gantt/spec/features/timeline/timeline_dates_spec.rb[1:2:1]`
- `rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:2]`
- `rspec ./spec/features/projects/lists/columns_spec.rb[1:2:1:1]`

Treat this as a standalone task, unrelated to PR #23614. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23614 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @akabiru to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @akabiru, and request a review from @akabiru.
On every commit, set @akabiru as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.