Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alfonsodg wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,6 +1281,22 @@ async function handleGitLabError(response: import("node-fetch").Response): Promi
* @throws {Error} If GITLAB_ALLOWED_PROJECT_IDS is set and the requested project is not in the whitelist
*/
function getEffectiveProjectId(projectId: string): string {
// Guard against z.coerce.string() converting undefined/null to literal strings
if (!projectId || projectId === "undefined" || projectId === "null") {
if (GITLAB_ALLOWED_PROJECT_IDS.length === 1) {
return GITLAB_ALLOWED_PROJECT_IDS[0];
}
Comment on lines +1284 to +1288
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
if (GITLAB_ALLOWED_PROJECT_IDS.length > 1) {
throw new Error(
`Multiple projects allowed (${GITLAB_ALLOWED_PROJECT_IDS.join(", ")}). Please specify a project ID.`
);
}
if (GITLAB_PROJECT_ID) {
return GITLAB_PROJECT_ID;
}
throw new Error("No project ID provided and GITLAB_PROJECT_ID is not set");
Comment on lines +1284 to +1297
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}

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) {
Expand Down Expand Up @@ -1509,16 +1525,10 @@ async function listIssues(
// Add all query parameters
Object.entries(options).forEach(([key, value]) => {
if (value !== undefined) {
const keys = ["labels", "assignee_username"];
if (keys.includes(key)) {
if (Array.isArray(value)) {
// Handle array of labels
value.forEach(label => {
url.searchParams.append(`${key}[]`, label.toString());
});
} else if (value) {
url.searchParams.append(`${key}[]`, value.toString());
}
if (key === "labels" && Array.isArray(value)) {
url.searchParams.append(key, value.join(","));
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
} else if (key === "assignee_username" && Array.isArray(value)) {
value.forEach(v => url.searchParams.append(`${key}[]`, v.toString()));
} else {
url.searchParams.append(key, String(value));
}
Comment on lines 1525 to 1534
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Expand Down Expand Up @@ -9390,7 +9400,7 @@ async function handleToolCall(params: any) {

const mergeRequests = await listMergeRequests(project_id, cleanedOptions);
return {
content: [{ type: "text", text: JSON.stringify(mergeRequests, null, 2) }],
content: [{ type: "text", text: JSON.stringify(mergeRequests ?? [], null, 2) }],
Copy link
Copy Markdown
Owner

@zereight zereight May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner

@zereight zereight May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
Comment on lines 9402 to 9404
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}

Expand Down
Loading