Hi @romaneisner could you review this MR please?

Fred de Gier (9e757928) at 18 Mar 12:30

Thanks for the ping @bastirehm the issue is not fully resolved, I'll update the issues.

Fred de Gier (c01a2845) at 18 Mar 10:46

... and 1 more commit

Fred de Gier (83e46b26) at 18 Mar 10:45

What does this MR do and why?

Freezes factory-created objects in spec/models/ci/runner_spec.rb by adding freeze: true to let_it_be declarations throughout the spec file.

This is a test optimization that:

• Replaces let_it_be (without freeze) and let_it_be_with_reload with let_it_be(:name, freeze: true) where the test does not mutate the object
• Replaces let_it_be_with_refind with let_it_be(:name, freeze: true) where refind was unnecessary
• Converts a few cases to let_it_be_with_refind where the object is actually mutated in before blocks
• Inlines a shared example (group or project runner initializing organization_id) that was only used twice, simplifying the test structure
• Hoists shared admin and user2 let_it_be declarations to a higher scope to avoid duplication across with_creator_id and created_by_admins describe blocks
• Reuses the existing other_organization let_it_be instead of creating a duplicate other_org in the assignable_for context
• Consolidates runner attribute setup (e.g., token_expires_at) into the factory call instead of a separate before block

Frozen objects raise FrozenError if a test accidentally modifies them, catching unintended side effects between examples. This also improves test performance by avoiding unnecessary database reloads.

References

• #591183

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Approved the MR, @junminghuang can you do the maintainer review please?

Thanks @pedropombeiro for addressing my comment, looks great let's get this merged. 🚀

What does this MR do and why?

Freezes factory-created objects in spec/models/ci/runner_spec.rb by adding freeze: true to let_it_be declarations throughout the spec file.

This is a test optimization that:

• Replaces let_it_be (without freeze) and let_it_be_with_reload with let_it_be(:name, freeze: true) where the test does not mutate the object
• Replaces let_it_be_with_refind with let_it_be(:name, freeze: true) where refind was unnecessary
• Converts a few cases to let_it_be_with_refind where the object is actually mutated in before blocks
• Inlines a shared example (group or project runner initializing organization_id) that was only used twice, simplifying the test structure
• Hoists shared admin and user2 let_it_be declarations to a higher scope to avoid duplication across with_creator_id and created_by_admins describe blocks
• Reuses the existing other_organization let_it_be instead of creating a duplicate other_org in the assignable_for context
• Consolidates runner attribute setup (e.g., token_expires_at) into the factory call instead of a separate before block

Frozen objects raise FrozenError if a test accidentally modifies them, catching unintended side effects between examples. This also improves test performance by avoiding unnecessary database reloads.

References

• #591183

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

What does this merge request do and why?

This MR fixes a bug where the _get_discussion_id_from_note_rest method in duo_workflow_service/tools/duo_base_tool.py was only fetching the first page of discussions (default 20 items) from the GitLab API. When a note being replied to was in a discussion on page 2 or beyond, it would not be found, causing the error "Note {note_id} not found in this merge request/issue".

The fix implements pagination to iterate through all discussion pages until the note is found or all pages are exhausted. The method now uses page and per_page parameters (100 items per page) to efficiently fetch all discussions.

How to set up and validate locally

This approach simulates the real pagination scenario end-to-end with GDK, which is more reliable than unit tests alone.

1. Start GDK and AI Gateway

Make sure GDK is running, then start AI Gateway from the fix branch:

cd <ai-gateway-dir>
poetry run uvicorn ai_gateway.app:create_app --factory --host 0.0.0.0 --port 5052 --reload

2. Create a test MR with more than 100 discussion threads

The bug only triggers when the target note is beyond the first page (100 items with the fix, 20 without). Use the GitLab API to seed enough discussions on a local test MR:

PROJECT_ID=<your-local-project-id>
MR_IID=<your-test-mr-iid>
TOKEN=<your-gdk-pat>

for i in $(seq 1 105); do
  curl -s -X POST "http://gdk.test:3000/api/v4/projects/$PROJECT_ID/merge_requests/$MR_IID/discussions" \
    -H "PRIVATE-TOKEN: $TOKEN" \
    -d "body=Discussion note $i"
done

3. Trigger Duo Workflow to reply to a note on page 2

Use Duo Workflow (via the VS Code extension or Web IDE connected to your GDK) and ask it to reply to one of the notes created in the last batch (i.e., one of the note IDs from the 101st discussion onward). This will invoke _get_discussion_id_from_note_rest internally.

4. Verify the behaviour

• Without the fix: Duo returns "Note {note_id} not found in this merge request" for notes beyond page 1.
• With the fix: Duo successfully resolves the discussion ID and posts the reply in the correct thread.

To observe pagination in action, you can temporarily add a debug log inside the while loop in duo_base_tool.py:

print(f"Fetching page {page}, got {len(discussions)} discussions")

5. Run the unit tests

poetry run pytest tests/duo_workflow_service/tools/test_duo_base_tool.py -k "test_get_discussion_id_from_note_rest" -v

Merge request checklist

• Tests added for new functionality. If not, please raise an issue to follow up.
• Documentation added/updated, if needed.
• If this change requires executor implementation: verified that issues/MRs exist for both Go executor and Node executor or confirmed that changes are backward-compatible and don't break existing executor functionality.

Closes #2048

Thanks, great to see.

praise: Great benchmark and great to see we have a huge performance gain.

Fred de Gier (066e7ae4) at 18 Mar 10:23

What does this merge request do and why?

How to set up and validate locally

Impacted categories

The following categories relate to this merge request:

• gdk-reliability - e.g. When a GDK action fails to complete.
• gdk-usability - e.g. Improvements or suggestions around how the GDK functions.
• gdk-performance - e.g. When a GDK action is slow or times out.

Merge request checklist

• This MR references an issue describing the change.
• This change is backward compatible. If not, include steps to communicate to users.
• Tests added for new functionality. If not, raise an issue to follow-up.
• Observability added/updated (logging, metrics, tracing).
• Documentation added/updated.
• Announcement added for notable changes.
• gdk doctor test added.

Closes #3285

Fred de Gier (583cbe88) at 18 Mar 09:52

Thanks @mrincon LGTM. @ramistry can you do the maintainer review please?

What does this MR do and why?

Migrates lodash imports to lodash-es in projects, import, jira connect, kubernetes, deploy keys, labels, mirrors, badges, crm, contributors, and other collaboration modules.

This is part of #558221.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist.

Hi @fabiopitino could you do the devopsverify maintainer review please?

Hi @nateweinshenker could you please do the initial backend review? Please assign to @skundapur for maintainer review.

I should clarify that I just want to make sure that there's no hidden performance benefit of using the default (20) instead of the max (100).