Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
- `merge_merge_request` no longer strips `sha`, which GitLab 19.2+ groups may require when merging ([#665](https://github.com/zereight/gitlab-mcp/issues/665))
- `list_job_artifacts` no longer fails when GitLab returns `mode: null` on artifact tree entries ([#661](https://github.com/zereight/gitlab-mcp/issues/661))
- `create_tag` no longer fails when GitLab omits `release` from the tag response ([#653](https://github.com/zereight/gitlab-mcp/issues/653))
- `wait_for_pipeline`, `wait_for_job`, `play_pipeline_jobs`, and `rebase_merge_request` now report `Timed out waiting for terminal status` when a status request is still running at `timeout_seconds`, instead of `The operation was aborted due to timeout`

##### Deprecated

Expand Down
16 changes: 6 additions & 10 deletions index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@
import { checkForNewVersion } from "./utils/version-check.js";
import { assertGitLabVersionAtLeast } from "./utils/gitlab-version-gate.js";
import { fetchWithValidatedRedirects } from "./utils/safe-redirect-fetch.js";
import { fetchStatusBeforeDeadline } from "./utils/poll-deadline.js";
import {
parseGitLabVersionApiResponse,
type GitLabInstanceVersionMetadata,
Expand Down Expand Up @@ -862,7 +863,7 @@
})),
}));

mcpServer.server.setRequestHandler(GetPromptRequestSchema, async (request: any) => {

Check warning on line 866 in index.ts

View workflow job for this annotation

GitHub Actions / test

Unexpected any. Specify a different type
const prompt = MCP_PROMPTS.find(p => p.name === request.params.name);
if (!prompt) {
throw new Error(`Unknown prompt: ${request.params.name}`);
Expand All @@ -888,7 +889,7 @@
};
});

mcpServer.server.setRequestHandler(CallToolRequestSchema, async (request: any) => {

Check warning on line 892 in index.ts

View workflow job for this annotation

GitHub Actions / test

Unexpected any. Specify a different type
// Manually retrieve the session context using the session ID passed in the request.
// This is a robust workaround for AsyncLocalStorage context loss.
const sessionId = request.params.sessionId;
Expand All @@ -899,7 +900,7 @@
request.params.arguments as Record<string, unknown> | undefined
);

const logCompletion = (result: any, completionOptions?: { skipJmespath?: boolean }) => {

Check warning on line 903 in index.ts

View workflow job for this annotation

GitHub Actions / test

Unexpected any. Specify a different type
const durationMs = Date.now() - start;
logger.info(
{ tool: toolName, event: "tool_call_done", durationMs },
Expand All @@ -911,7 +912,7 @@
(toolName === "download_release_asset" && !IS_REMOTE);
const filtered = applyJmespathToToolResult(result, jmespathExpression, {
...completionOptions,
maskValue: maskingEngine ? (value: unknown) => maskingEngine!.maskValue(value) : undefined,

Check warning on line 915 in index.ts

View workflow job for this annotation

GitHub Actions / test

Forbidden non-null assertion
});
const masked = maskingEngine
? maskingEngine.maskToolResult(filtered, {
Expand Down Expand Up @@ -1792,7 +1793,7 @@
function defaultAuthRetryConfig() {
return {
isOAuthEnabled: () => USE_OAUTH && oauthClient != null,
refreshToken: (force: boolean) => oauthClient!.getAccessToken(force),

Check warning on line 1796 in index.ts

View workflow job for this annotation

GitHub Actions / test

Forbidden non-null assertion
onTokenRefreshed: (token: string) => {
OAUTH_ACCESS_TOKEN = token;
},
Expand Down Expand Up @@ -2660,7 +2661,7 @@
);

// Build request body, converting labels array to comma-separated string
const body: Record<string, any> = { ...options };

Check warning on line 2664 in index.ts

View workflow job for this annotation

GitHub Actions / test

Unexpected any. Specify a different type
if (body.labels && Array.isArray(body.labels)) {
body.labels = body.labels.join(",");
}
Expand Down Expand Up @@ -2908,7 +2909,7 @@
);

// Convert labels array to comma-separated string if present
const body: Record<string, any> = { ...options };

Check warning on line 2912 in index.ts

View workflow job for this annotation

GitHub Actions / test

Unexpected any. Specify a different type
if (body.labels && Array.isArray(body.labels)) {
body.labels = body.labels.join(",");
}
Expand Down Expand Up @@ -9267,11 +9268,10 @@
mergeRequestIid: string | number,
deadline: number
): Promise<{ status: string; mergeRequest: GitLabMergeRequest }> {
const remaining = deadline - Date.now();
if (remaining <= 0) {
throw new Error("Timed out waiting for terminal status");
}
return getMergeRequestRebaseStatus(projectId, mergeRequestIid, AbortSignal.timeout(remaining));
return fetchStatusBeforeDeadline(
signal => getMergeRequestRebaseStatus(projectId, mergeRequestIid, signal),
deadline
);
}

/**
Expand Down Expand Up @@ -9349,11 +9349,7 @@
pollIntervalSeconds = 5
): Promise<T> {
const deadline = Date.now() + timeoutSeconds * 1000;
const fetchBeforeDeadline = async (): Promise<T> => {
const remaining = deadline - Date.now();
if (remaining <= 0) throw new Error("Timed out waiting for terminal status");
return fetchStatus(AbortSignal.timeout(remaining));
};
const fetchBeforeDeadline = (): Promise<T> => fetchStatusBeforeDeadline(fetchStatus, deadline);
let value = await fetchBeforeDeadline();
while (!terminalStatuses.has(value.status)) {
const remaining = deadline - Date.now();
Expand Down
38 changes: 38 additions & 0 deletions test/test-rebase-merge-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const MR_FORBIDDEN = "6";
const MR_STALE_THEN_MOVED = "7";
const MR_NOOP = "8";
const MR_RECHECK_TIMEOUT = "9";
const MR_SLOW_POLL = "10";

function buildMergeRequest(iid: string, overrides: Record<string, unknown> = {}) {
return {
Expand Down Expand Up @@ -236,6 +237,7 @@ describe("rebase_merge_request tool", () => {
MR_STALE_THEN_MOVED,
MR_NOOP,
MR_RECHECK_TIMEOUT,
MR_SLOW_POLL,
]) {
addRebaseHandler(iid);
}
Expand Down Expand Up @@ -272,6 +274,24 @@ describe("rebase_merge_request tool", () => {
{ rebase_in_progress: false, sha: NEW_SHA },
]);

// The head read answers at once; the poll after the rebase answers after the 1s timeout.
const slowPollQueries: Array<Record<string, unknown>> = [];
statusQueries.set(MR_SLOW_POLL, slowPollQueries);
mockGitLab.addMockHandler(
"get",
`/projects/${TEST_PROJECT_ID}/merge_requests/${MR_SLOW_POLL}`,
(req, res) => {
slowPollQueries.push({ ...req.query });
if (slowPollQueries.length === 1) {
res.json(buildMergeRequest(MR_SLOW_POLL, { rebase_in_progress: false }));
return;
}
setTimeout(() => {
res.json(buildMergeRequest(MR_SLOW_POLL, { rebase_in_progress: true }));
}, 1500);
}
);

mockGitLab.addMockHandler(
"put",
`/projects/${TEST_PROJECT_ID}/merge_requests/${MR_FORBIDDEN}/rebase`,
Expand Down Expand Up @@ -365,6 +385,24 @@ describe("rebase_merge_request tool", () => {
);
});

test("rebase_merge_request reports a timeout when a poll is still in flight at the deadline", async () => {
await assert.rejects(
() =>
callTool(
"rebase_merge_request",
{
project_id: TEST_PROJECT_ID,
merge_request_iid: MR_SLOW_POLL,
timeout_seconds: 1,
poll_interval_seconds: 1,
},
env()
),
/Timed out waiting for terminal status/
);
assert.strictEqual(statusQueries.get(MR_SLOW_POLL)?.length, 2);
});

test("rebase_merge_request keeps waiting when the first finished poll still has the old sha", async () => {
const result = await callTool(
"rebase_merge_request",
Expand Down
118 changes: 118 additions & 0 deletions test/utils/poll-deadline.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
import assert from "node:assert/strict";
import { createServer } from "node:http";
import type { AddressInfo } from "node:net";
import { describe, test } from "node:test";
import { fetchStatusBeforeDeadline, POLL_TIMEOUT_MESSAGE } from "../../utils/poll-deadline.js";

// AbortSignal.timeout does not keep the process alive, so hold it open until the abort.
function waitForAbort(signal: AbortSignal): Promise<void> {
return new Promise(resolve => {
if (signal.aborted) {
resolve();
return;
}
const keepAlive = setInterval(() => {}, 1000);
signal.addEventListener(
"abort",
() => {
clearInterval(keepAlive);
resolve();
},
{ once: true }
);
});
}

async function rejectAfterAbort(signal: AbortSignal, error: () => unknown): Promise<never> {
await waitForAbort(signal);
throw error();
}

describe("When fetchStatusBeforeDeadline runs", () => {
test("should return the fetched value before the deadline", async () => {
const value = await fetchStatusBeforeDeadline(async () => "finished", Date.now() + 1000);

assert.equal(value, "finished");
});

test("should time out without fetching once the deadline has passed", async () => {
let fetched = false;

await assert.rejects(
fetchStatusBeforeDeadline(async () => {
fetched = true;
return "finished";
}, Date.now() - 1),
{ message: POLL_TIMEOUT_MESSAGE }
);
assert.equal(fetched, false);
});

test("should report an abort by the deadline signal as a timeout", async () => {
let reason: unknown;

await assert.rejects(
fetchStatusBeforeDeadline(
signal =>
rejectAfterAbort(signal, () => {
reason = signal.reason;
return signal.reason;
}),
Date.now() + 20
),
(error: Error) => {
assert.equal(error.message, POLL_TIMEOUT_MESSAGE);
assert.equal(error.cause, reason);
return true;
}
);
});

test("should report a body read that starts after the deadline as a timeout", async () => {
const server = createServer((_req, res) => {
res.writeHead(200, { "content-type": "application/json" });
res.end('{"status":"running"}');
});
await new Promise<void>(resolve => server.listen(0, "127.0.0.1", resolve));
const { port } = server.address() as AddressInfo;

try {
await assert.rejects(
fetchStatusBeforeDeadline(async signal => {
const response = await fetch(`http://127.0.0.1:${port}/`, { signal });
await waitForAbort(signal);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return response.json();
}, Date.now() + 1000),
(error: Error) => {
assert.equal(error.message, POLL_TIMEOUT_MESSAGE);
// An abort of the request itself would leave the TimeoutError reason here.
assert.equal((error.cause as Error).name, "AbortError");
return true;
}
);
} finally {
server.closeAllConnections();
await new Promise(resolve => server.close(resolve));
}
});

test("should keep another error that lands after the deadline", async () => {
const failure = new Error("GitLab API error: 500 Internal Server Error");

await assert.rejects(
fetchStatusBeforeDeadline(signal => rejectAfterAbort(signal, () => failure), Date.now() + 20),
error => error === failure
);
});

test("should keep an error that lands before the deadline", async () => {
const failure = new Error("GitLab API error: 404 Not Found");

await assert.rejects(
fetchStatusBeforeDeadline(async () => {
throw failure;
}, Date.now() + 1000),
error => error === failure
);
});
});
31 changes: 31 additions & 0 deletions utils/poll-deadline.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
export const POLL_TIMEOUT_MESSAGE = "Timed out waiting for terminal status";

function isAbortError(error: unknown): boolean {
return error instanceof Error && error.name === "AbortError";
}

/**
* Fetch one poll result inside the deadline. When the deadline signal aborts the
* request, the wait has timed out, so report that instead of the abort. Any other
* rejection passes through unchanged, even one that lands after the deadline.
*/
export async function fetchStatusBeforeDeadline<T>(
fetchStatus: (signal: AbortSignal) => Promise<T>,
deadline: number
): Promise<T> {
const remaining = deadline - Date.now();
if (remaining <= 0) {
throw new Error(POLL_TIMEOUT_MESSAGE);
}
const signal = AbortSignal.timeout(remaining);
try {
return await fetchStatus(signal);
} catch (error) {
// fetch rejects with signal.reason itself when this signal aborts it. A body read
// that starts after the abort throws a new AbortError instead.
if (signal.aborted && (error === signal.reason || isAbortError(error))) {
throw new Error(POLL_TIMEOUT_MESSAGE, { cause: error });
}
throw error;
}
}
Loading