fix: defensive guards for project_id coercion, labels query, and null MR response

[alfonsodg]

Summary

Three defensive fixes for edge cases encountered in production:

1. Guard getEffectiveProjectId against coerced string literals

z.coerce.string() converts undefined/null to the literal strings "undefined"/"null", which bypass the existing !projectId falsy check. This causes the function to treat them as real project IDs, leading to 404 errors from the GitLab API.

Fix: Early return when projectId is falsy or equals "undefined"/"null", falling back to allowed project IDs or GITLAB_PROJECT_ID.

2. Fix labels query parameter format in listIssues

The GitLab API expects labels as a comma-separated string (labels=bug,feature), not array notation (labels[]=bug&labels[]=feature). The array format causes the API to ignore the filter silently.

Ref: https://docs.gitlab.com/api/issues.html#list-issues

Fix: Use value.join(",") for labels while keeping [] notation for assignee_username which does expect array format.

3. Null coalescing for listMergeRequests response

When listMergeRequests returns null (e.g., empty project or API edge case), JSON.stringify(null) produces the string "null" instead of a valid array.

Fix: mergeRequests ?? [] ensures a valid JSON array is always returned.

Testing

• TypeScript compilation passes (tsc --noEmit)
• Tested against self-hosted GitLab 17.x with multiple project configurations

[Copilot]

Pull request overview

This PR adds defensive handling for a few production edge cases in GitLab API tool calls, primarily around project ID resolution and query/response shaping.

Changes:

• Add guards in getEffectiveProjectId to treat "null"/"undefined" (and other falsy values) as missing project IDs and fall back to allowed/default project configuration.
• Fix listIssues query serialization so labels is sent as a comma-separated string instead of labels[] array notation.
• Ensure list_merge_requests tool output is always a JSON array by null-coalescing the response before stringification.

Comments suppressed due to low confidence (1)

index.ts:1304

• The new guard returns early when projectId is falsy, so the later GITLAB_ALLOWED_PROJECT_IDS.length === 1 && !projectId branch is now unreachable. Consider removing the redundant !projectId checks in this whitelist block to avoid dead code and keep the control flow easier to reason about.

  if (GITLAB_ALLOWED_PROJECT_IDS.length > 0) {
    // If there's only one allowed project, use it as default
    if (GITLAB_ALLOWED_PROJECT_IDS.length === 1 && !projectId) {
      return GITLAB_ALLOWED_PROJECT_IDS[0];
    }

[Copilot]

When labels is an empty array, value.join(",") produces an empty string and this now appends labels= to the query string. Previously, an empty array resulted in no labels parameter at all. Consider skipping the param when the array is empty to preserve prior behavior and avoid API-specific ambiguity for an empty labels filter.

Suggested change

	url.searchParams.append(key, value.join(","));
	if (value.length > 0) {
	url.searchParams.append(key, value.join(","));
	}

[Copilot]

listMergeRequests() is typed to return GitLabMergeRequest[] and currently z.array(...).parse(data) will throw if the API returns null. Coalescing here to [] is likely redundant and can hide unexpected return shapes; consider handling a null/invalid response inside listMergeRequests (or adjusting the zod schema) so the behavior is consistent for all callers.

[Copilot]

There are existing automated tests for getEffectiveProjectId (see test/test-geteffectiveprojectid.ts), but they don’t cover the new edge cases where inputs become the literal strings "null"/"undefined". Adding test cases for these values would help prevent regressions and validate the intended fallback/throw behavior across whitelist and default-project configurations.

[Copilot]

The query serialization for labels changed semantics (comma-separated string, and potential labels= for empty arrays). Adding a focused test that asserts the outgoing request URL contains labels=bug,feature (and omits labels for []) would lock in the intended GitLab API behavior.

[Copilot]

Special-casing the literal strings "undefined" and "null" means a real project whose ID/path is exactly "undefined" or "null" can no longer be targeted. If the intent is only to handle z.coerce.string() / null inputs, it’s safer to fix this at the schema boundary (e.g., preprocess null/"null" to undefined) or carry an undefined | string type into this function rather than reserving specific string values.

[zereight]

Left one comment about the null merge-request response handling.

[zereight]

This does not actually handle the null response case described in the PR. listMergeRequests() still parses the response with z.array(GitLabMergeRequestSchema).parse(data) before returning, so if GitLab returns null, it will throw inside listMergeRequests() and this mergeRequests ?? [] fallback is never reached.

Could we move the fallback into listMergeRequests() itself, for example by normalizing data ?? [] before the zod array parse, or by changing the parser to explicitly accept null and return [] there? That keeps the behavior consistent for every caller instead of only this tool-call branch.

[zereight]

Suggested shape:

const data = await response.json();
const normalizedData = data ?? [];
return z.array(GitLabMergeRequestSchema).parse(normalizedData);

If we only want to tolerate null and still reject other unexpected shapes, this keeps the existing zod validation intact while making the documented null -> [] behavior actually reachable for every listMergeRequests() caller.

[zereight]

.