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
98 changes: 90 additions & 8 deletions actions/setup/js/mount_mcp_as_cli.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ const SAFEOUTPUTS_SERVER_NAME = "safeoutputs";
/** Default timeout (ms) for HTTP calls to the local MCP gateway */
const DEFAULT_HTTP_TIMEOUT_MS = 15000;

/**
* Maximum number of times to retry tools/list when a server returns 0 tools.
* The gateway may report a backend as "running" before the backend has finished
* building its tool schema (a race condition more likely with large configs).
*/
const TOOLS_EMPTY_MAX_RETRIES = 5;

/**
* Milliseconds to wait between tools/list retry attempts when the result is empty.
*/
const TOOLS_EMPTY_RETRY_DELAY_MS = 1000;

/**
* Parse a tools JSON file and return a validated tools array.
*
Expand Down Expand Up @@ -319,9 +331,9 @@ function parseMCPResponseBody(body) {
* @param {string} serverUrl - HTTP URL of the MCP server endpoint
* @param {string} apiKey - Bearer token for gateway authentication
* @param {typeof import("@actions/core")} core - GitHub Actions core
* @returns {Promise<Array<{name: string, description?: string, inputSchema?: unknown}>>}
* @returns {Promise<{tools: Array<{name: string, description?: string, inputSchema?: unknown}>, emptyWasSuccessful: boolean}>}
*/
async function fetchMCPTools(serverUrl, apiKey, core) {
async function fetchMCPToolsResult(serverUrl, apiKey, core) {
const authHeaders = { Authorization: apiKey };

// Step 1: initialize – establish the session and capture Mcp-Session-Id if present
Expand Down Expand Up @@ -349,7 +361,7 @@ async function fetchMCPTools(serverUrl, apiKey, core) {
}
} catch (err) {
core.warning(` initialize failed for ${serverUrl}: ${getErrorMessage(err)}`);
return [];
return { tools: [], emptyWasSuccessful: false };
}

// Step 2: notifications/initialized – required by MCP spec to complete the handshake.
Expand All @@ -367,14 +379,79 @@ async function fetchMCPTools(serverUrl, apiKey, core) {
if (respBody && typeof respBody === "object" && "result" in respBody && respBody.result && typeof respBody.result === "object") {
const result = respBody.result;
if ("tools" in result && Array.isArray(result.tools)) {
return /** @type {Array<{name: string, description?: string, inputSchema?: unknown}>} */ result.tools;
return {
tools: /** @type {Array<{name: string, description?: string, inputSchema?: unknown}>} */ result.tools,
emptyWasSuccessful: true,
};
}
}
return [];
return { tools: [], emptyWasSuccessful: false };
} catch (err) {
core.warning(` tools/list failed for ${serverUrl}: ${getErrorMessage(err)}`);
return [];
return { tools: [], emptyWasSuccessful: false };
}
}

/**
* Query the tools list from an MCP server via JSON-RPC.
*
* @param {string} serverUrl - HTTP URL of the MCP server endpoint
* @param {string} apiKey - ****** for gateway authentication
* @param {typeof import("@actions/core")} core - GitHub Actions core
* @returns {Promise<Array<{name: string, description?: string, inputSchema?: unknown}>>}
*/
async function fetchMCPTools(serverUrl, apiKey, core) {
const result = await fetchMCPToolsResult(serverUrl, apiKey, core);
return result.tools;
}

/**
* Fetch MCP tools with retry on empty result.
*
* The MCP gateway may report a backend as "running" before that backend has
* finished building its internal tool schema (a race between process-level
* readiness and schema construction). This is more likely with large
* dispatch-workflow configs where building tool definitions takes long enough
* that tools/list can still return 0 tools immediately after the health check
* passes. Retrying a handful of times with a short delay bridges that gap, but
* only for successful empty tools/list responses; transport/protocol failures
* stop immediately so unavailable backends still fail fast.
*
* @param {string} serverUrl
* @param {string} apiKey
* @param {string} serverName - Server name, used only for log messages
* @param {typeof import("@actions/core")} core
* @param {object} [options]
* @param {(ms: number) => Promise<void>} [options.sleep] - Delay function (injectable for tests)
* @param {(url: string, key: string, c: typeof import("@actions/core")) => Promise<Array<{name: string, description?: string, inputSchema?: unknown}> | {tools: Array<{name: string, description?: string, inputSchema?: unknown}>, emptyWasSuccessful: boolean}>} [options.fetchFn] - Fetch function (injectable for tests)
* @returns {Promise<Array<{name: string, description?: string, inputSchema?: unknown}>>}
*/
async function fetchMCPToolsWithRetry(serverUrl, apiKey, serverName, core, { sleep = undefined, fetchFn = undefined } = {}) {
const doSleep = sleep ?? (ms => new Promise(resolve => setTimeout(resolve, ms)));
const doFetchResult = async (url, key, c) => {
if (!fetchFn) {
return fetchMCPToolsResult(url, key, c);
}
const result = await fetchFn(url, key, c);
if (Array.isArray(result)) {
return { tools: result, emptyWasSuccessful: true };
}
return result;
};
let result = await doFetchResult(serverUrl, apiKey, core);
for (let attempt = 1; attempt <= TOOLS_EMPTY_MAX_RETRIES && result.emptyWasSuccessful && result.tools.length === 0; attempt++) {
core.warning(` tools/list returned 0 tools for '${serverName}', retrying in ${TOOLS_EMPTY_RETRY_DELAY_MS}ms (attempt ${attempt}/${TOOLS_EMPTY_MAX_RETRIES})...`);
await doSleep(TOOLS_EMPTY_RETRY_DELAY_MS);
result = await doFetchResult(serverUrl, apiKey, core);
if (!result.emptyWasSuccessful) {
core.warning(` stopping empty tools/list retries for '${serverName}' because tools/list did not complete successfully`);
break;
}
}
if (result.emptyWasSuccessful && result.tools.length === 0) {
core.warning(` tools/list still returned 0 tools for '${serverName}' after ${TOOLS_EMPTY_MAX_RETRIES} retries; continuing with empty tool list`);
}
return result.tools;
}

/**
Expand Down Expand Up @@ -521,8 +598,10 @@ async function main() {

const toolsFile = path.join(TOOLS_DIR, `${name}.json`);

// Query tools from the server using the host-accessible URL (mount step runs on host)
let tools = await fetchMCPTools(url, apiKey, core);
// Query tools from the server using the host-accessible URL (mount step runs on host).
// Retries on empty to handle the race between gateway health-reporting and
// the backend finishing internal tool-schema construction (common with large configs).
let tools = await fetchMCPToolsWithRetry(url, apiKey, name, core);
const validate = SERVER_VALIDATORS[name];
if (validate) {
tools = validate(tools, core);
Expand Down Expand Up @@ -582,6 +661,7 @@ module.exports = {
AWF_GATEWAY_IP,
main,
fetchMCPTools,
fetchMCPToolsWithRetry,
generateCLIWrapperScript,
isValidServerName,
shellEscapeDoubleQuoted,
Expand All @@ -593,4 +673,6 @@ module.exports = {
writeSafeOutputsGatewayEmptyFlag,
SERVER_VALIDATORS,
buildMCPCLIServersPromptList,
TOOLS_EMPTY_MAX_RETRIES,
TOOLS_EMPTY_RETRY_DELAY_MS,
};
159 changes: 158 additions & 1 deletion actions/setup/js/mount_mcp_as_cli.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,17 @@ import fs from "fs";
import os from "os";
import path from "path";

import { AWF_GATEWAY_IP, buildMCPCLIServersPromptList, getSafeOutputsGatewayEmptyFlagPath, parseMCPResponseBody, recoverSafeOutputsToolsIfNeeded, toContainerUrl } from "./mount_mcp_as_cli.cjs";
import {
AWF_GATEWAY_IP,
buildMCPCLIServersPromptList,
fetchMCPToolsWithRetry,
getSafeOutputsGatewayEmptyFlagPath,
parseMCPResponseBody,
recoverSafeOutputsToolsIfNeeded,
toContainerUrl,
TOOLS_EMPTY_MAX_RETRIES,
TOOLS_EMPTY_RETRY_DELAY_MS,
} from "./mount_mcp_as_cli.cjs";

describe("mount_mcp_as_cli.cjs", () => {
it("parses JSON object responses unchanged", () => {
Expand Down Expand Up @@ -170,4 +180,151 @@ describe("mount_mcp_as_cli.cjs", () => {
expect(docs).toContain('--confidence \"HIGH\"');
expect(docs).toContain("`mcpscripts` — run `mcpscripts --help`");
});

it("exports retry constants with expected values", () => {
expect(TOOLS_EMPTY_MAX_RETRIES).toBeGreaterThan(0);
expect(TOOLS_EMPTY_RETRY_DELAY_MS).toBeGreaterThan(0);
});

it("returns tools immediately when fetchFn succeeds on first attempt", async () => {
const tools = [{ name: "push_to_pull_request_branch" }];
let callCount = 0;
const fakeFetch = async () => {
callCount++;
return tools;
};
const result = await fetchMCPToolsWithRetry(
"http://localhost/mcp/safeoutputs",
"key",
"safeoutputs",
{ warning: () => {} },
{
fetchFn: fakeFetch,
sleep: async () => {},
}
);
expect(result).toEqual(tools);
expect(callCount).toBe(1);
});

it("retries when fetchFn returns empty and succeeds on a later attempt", async () => {
const tools = [{ name: "push_to_pull_request_branch" }];
let callCount = 0;
const fakeFetch = async () => {
callCount++;
return callCount < 3 ? [] : tools;
};
const warnings = [];
const result = await fetchMCPToolsWithRetry(
"http://localhost/mcp/safeoutputs",
"key",
"safeoutputs",
{ warning: msg => warnings.push(msg) },
{
fetchFn: fakeFetch,
sleep: async () => {},
}
);
expect(result).toEqual(tools);
expect(callCount).toBe(3);
expect(warnings).toHaveLength(2);
expect(warnings[0]).toContain("tools/list returned 0 tools");
expect(warnings[0]).toContain("safeoutputs");
});

it("stops retrying after TOOLS_EMPTY_MAX_RETRIES, emits final warning, and returns empty when always empty", async () => {
let callCount = 0;
const fakeFetch = async () => {
callCount++;
return [];
};
const warnings = [];
const result = await fetchMCPToolsWithRetry(
"http://localhost/mcp/safeoutputs",
"key",
"safeoutputs",
{ warning: msg => warnings.push(msg) },
{
fetchFn: fakeFetch,
sleep: async () => {},
}
);
expect(result).toEqual([]);
// 1 initial attempt + TOOLS_EMPTY_MAX_RETRIES retries
expect(callCount).toBe(1 + TOOLS_EMPTY_MAX_RETRIES);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The exhaustion test verifies callCount but doesn't assert on the number of warnings emitted — making it possible to silently remove all retry-warning calls without a failing test.

💡 Suggested addition
const warnings = [];
// pass { warning: msg => warnings.push(msg) } to core
// ...
expect(warnings).toHaveLength(TOOLS_EMPTY_MAX_RETRIES);

This pins the warning behaviour alongside the call-count assertion so that both the retry loop and its observability signal are covered.

@copilot please address this.

expect(warnings).toHaveLength(TOOLS_EMPTY_MAX_RETRIES + 1);
expect(warnings[warnings.length - 1]).toContain("still returned 0 tools");
expect(warnings[warnings.length - 1]).toContain("after");
});

it("invokes sleep between retry attempts", async () => {
let callCount = 0;
const sleepDelays = [];
const fakeFetch = async () => {
callCount++;
return callCount < 2 ? [] : [{ name: "tool" }];
};
await fetchMCPToolsWithRetry(
"http://localhost/mcp/safeoutputs",
"key",
"safeoutputs",
{ warning: () => {} },
{
fetchFn: fakeFetch,
sleep: async ms => {
sleepDelays.push(ms);
},
}
);
expect(sleepDelays).toEqual([TOOLS_EMPTY_RETRY_DELAY_MS]);
});

it("does not retry when fetchFn reports a non-successful tools/list fetch", async () => {
let callCount = 0;
const warnings = [];
const fakeFetch = async () => {
callCount++;
return { tools: [], emptyWasSuccessful: false };
};
const result = await fetchMCPToolsWithRetry(
"http://localhost/mcp/safeoutputs",
"key",
"safeoutputs",
{ warning: msg => warnings.push(msg) },
{
fetchFn: fakeFetch,
sleep: async () => {},
}
);
expect(result).toEqual([]);
expect(callCount).toBe(1);
expect(warnings).toHaveLength(0);
});

it("stops further retries if a later fetchFn call is non-successful", async () => {
let callCount = 0;
const warnings = [];
const fakeFetch = async () => {
callCount++;
if (callCount === 1) {
return [];
}
return { tools: [], emptyWasSuccessful: false };
};
const result = await fetchMCPToolsWithRetry(
"http://localhost/mcp/safeoutputs",
"key",
"safeoutputs",
{ warning: msg => warnings.push(msg) },
{
fetchFn: fakeFetch,
sleep: async () => {},
}
);
expect(result).toEqual([]);
expect(callCount).toBe(2);
expect(warnings).toHaveLength(2);
expect(warnings[0]).toContain("retrying");
expect(warnings[1]).toContain("stopping empty tools/list retries");
});
});
Loading