fix: Fetch serverInstructions for OAuth MCP servers after connection

[mnaser]

Summary

When an MCP server requires OAuth, the server inspection at startup skips fetching serverInstructions since no user is authenticated yet. This caused serverInstructions: true to be shown as the literal string "true" in the LLM context instead of the actual instructions.

This fix:

• Skips including boolean true or string "true" values in the formatted instructions (prevents showing literal "true")
• Fetches actual instructions from the server after a successful OAuth connection is established
• Adds updateServerInstructions method to MCPServersRegistry to update the cached config with fetched instructions

Fixes: #9700

Change Type

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

Testing

• Configure an MCP server with OAuth that has serverInstructions: true
• Verify the literal "true" is no longer shown in the LLM context before OAuth
• Complete OAuth authentication flow
• Verify actual server instructions are fetched and shown in subsequent requests

Checklist

• My code adheres to this project's style guidelines
• I have performed a self-review of my own code
• I have commented in any complex areas of my code
• I have made pertinent documentation changes
• My changes do not introduce new warnings
• I have written tests demonstrating that my changes are effective or that my feature works
• Local unit tests pass with my changes
• A pull request for updating the documentation has been submitted.

[Copilot]

Pull request overview

This PR fixes an issue where OAuth-protected MCP servers would display the literal string "true" in the LLM context instead of the actual server instructions. The fix prevents showing placeholder values and implements fetching of actual instructions after OAuth authentication completes.

• Adds filtering logic to skip boolean true and string "true" values when formatting server instructions for LLM context
• Implements automatic fetching of server instructions after successful OAuth connection establishment
• Adds updateServerInstructions method to MCPServersRegistry for updating cached configurations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File	Description
packages/api/src/mcp/registry/MCPServersRegistry.ts	Adds updateServerInstructions method to update cached server instructions and clear read-through caches
packages/api/src/mcp/UserConnectionManager.ts	Fetches and updates server instructions after OAuth connection succeeds, using isEnabled utility
packages/api/src/mcp/MCPManager.ts	Adds filtering logic to skip boolean true and string "true" values when collecting instructions for LLM context

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The updateServerInstructions method lacks concurrency control. If multiple users connect to the same OAuth server simultaneously, they could all attempt to fetch and update the serverInstructions concurrently, potentially causing race conditions with cache updates and unnecessary duplicate fetches. Consider adding a lock or deduplication mechanism to ensure only one update happens at a time per server, or check if instructions have already been fetched before updating.

[Copilot]

The two conditions checking for "true" values are redundant and confusing. Line 125 checks isEnabled(config.serverInstructions) && typeof config.serverInstructions !== 'string', which is true only when serverInstructions is the boolean true (since isEnabled returns true for both boolean true and string "true", but the type check excludes strings). Lines 128-130 then separately check for the string "true". These could be simplified to a single condition like: config.serverInstructions === true || (typeof config.serverInstructions === 'string' && config.serverInstructions.toLowerCase().trim() === 'true') which is more explicit and easier to understand.

Suggested change

	if (isEnabled(config.serverInstructions) && typeof config.serverInstructions !== 'string') {
	continue;
	}
	if (typeof config.serverInstructions === 'string' && config.serverInstructions.toLowerCase().trim() === 'true') {
	continue;
	}
	if (
	config.serverInstructions === true ||
	(typeof config.serverInstructions === 'string' &&
	config.serverInstructions.toLowerCase().trim() === 'true')
	) {
	continue;
	}

[Copilot]

The updateServerInstructions method only updates CACHE-based server configs (YAML-defined servers) and does not handle DB-based configs. If an OAuth server is stored in the database rather than YAML config, the instructions will not be persisted. The method silently does nothing when configFromCache is undefined. Consider either updating both repositories or adding a parameter to specify which repository to update, and log a warning when the server is not found in cache.

Suggested change

	* @param serverName - The name of the server
	* @param instructions - The instructions string fetched from the server
	*/
	public async updateServerInstructions(serverName: string, instructions: string): Promise<void> {
	// Update in cache repository (YAML-defined servers)
	const configFromCache = await this.cacheConfigsRepo.get(serverName);
	if (configFromCache) {
	configFromCache.serverInstructions = instructions;
	await this.cacheConfigsRepo.update(serverName, configFromCache);
	// Clear read-through caches to ensure fresh data on next read
	await this.readThroughCache.clear();
	await this.readThroughCacheAll.clear();
	logger.debug(`[MCPServersRegistry] Updated serverInstructions for "${serverName}"`);
	* Tries to update both CACHE (YAML-defined) and DB (dynamic) repositories if the server exists.
	* @param serverName - The name of the server
	* @param instructions - The instructions string fetched from the server
	*/
	public async updateServerInstructions(serverName: string, instructions: string): Promise<void> {
	let updated = false;
	// Update in cache repository (YAML-defined servers)
	const configFromCache = await this.cacheConfigsRepo.get(serverName);
	if (configFromCache) {
	configFromCache.serverInstructions = instructions;
	await this.cacheConfigsRepo.update(serverName, configFromCache);
	logger.debug(
	`[MCPServersRegistry] Updated serverInstructions for "${serverName}" in cache repository`,
	);
	updated = true;
	}
	// Update in DB repository (dynamic servers)
	const configFromDb = await this.dbConfigsRepo.get(serverName);
	if (configFromDb) {
	configFromDb.serverInstructions = instructions;
	await this.dbConfigsRepo.update(serverName, configFromDb);
	logger.debug(
	`[MCPServersRegistry] Updated serverInstructions for "${serverName}" in DB repository`,
	);
	updated = true;
	}
	if (updated) {
	// Clear read-through caches to ensure fresh data on next read
	await this.readThroughCache.clear();
	await this.readThroughCacheAll.clear();
	logger.debug(
	`[MCPServersRegistry] Cleared read-through caches after updating serverInstructions for "${serverName}"`,
	);
	} else {
	logger.warn(
	`[MCPServersRegistry] Failed to update serverInstructions: server "${serverName}" not found in cache or DB`,
	);

[Copilot]

The try-catch block for fetching server instructions swallows errors with only a warning log. If fetching instructions fails repeatedly for a particular server, users will never receive the instructions even though the server indicates they should be available. Consider adding retry logic, or updating the config to mark that instruction fetching was attempted but failed, so the system doesn't silently keep trying on every connection and users are informed of the issue.

[Copilot]

When updateServerInstructions is called and successfully updates the cache with the fetched instructions string, the read-through caches are cleared. However, there's a potential race condition: if another request calls getServerConfig for this server between the cache update (line 196) and the cache clear (lines 198-199), it might cache the old value in the read-through cache, which then persists even after the clear since it was added after the update. Consider clearing the cache before the update, or using a transactional approach to ensure consistency.

[Copilot]

The condition doesn't properly check if instructions have already been fetched. After the first successful OAuth connection by any user, updateServerInstructions updates the cache with the fetched string. However, on subsequent OAuth connections by other users, getServerConfig returns the cached config which now has the actual instruction string. Since isEnabled() only returns true for boolean true or string "true", this condition will correctly skip re-fetching. But if the update failed or returned null/empty, the config would still have serverInstructions: true, causing repeated fetch attempts. Consider adding a check for whether serverInstructions is specifically boolean true or string "true" to be more explicit about the intent.

Suggested change

	// For OAuth servers, fetch and update serverInstructions if enabled but not yet fetched
	if (config.requiresOAuth && isEnabled(config.serverInstructions)) {
	// For OAuth servers, fetch and update serverInstructions if explicitly enabled but not yet fetched
	if (
	config.requiresOAuth &&
	(config.serverInstructions === true || config.serverInstructions === 'true')
	) {

[Copilot]

Clearing the entire read-through cache when updating a single server's instructions will invalidate cached data for ALL servers and ALL users. This is inefficient and could cause unnecessary refetching of configs. Consider using targeted cache invalidation by computing and clearing only the specific cache keys affected by this server update using the getReadThroughCacheKey method pattern.

[Copilot]

The new updateServerInstructions method and the logic to fetch instructions after OAuth connection lack test coverage. Based on the existing test file at packages/api/src/mcp/__tests__/MCPManager.test.ts which has comprehensive tests for formatInstructionsForContext, tests should be added to verify that: 1) boolean true and string "true" values are properly filtered from instructions, 2) the updateServerInstructions method correctly updates the cache and clears the read-through cache, and 3) instructions are fetched and updated after OAuth connection establishment.

[Copilot]

The comment states "this means instructions should be fetched from server but weren't (e.g., OAuth server that couldn't be connected to at startup)" but doesn't accurately describe when this happens. According to the PR description and the code in MCPServerInspector.ts, when a server requires OAuth, the inspector skips fetching serverInstructions during startup inspection because no user is authenticated yet. The comment should clarify that this specifically happens for OAuth servers where instructions are marked to be fetched (serverInstructions: true) but haven't been fetched yet due to lack of authentication, not necessarily because connection "couldn't be" established.

Suggested change

	// Skip if serverInstructions is true/boolean or "true" string - this means
	// instructions should be fetched from server but weren't (e.g., OAuth server
	// that couldn't be connected to at startup)
	// Skip if serverInstructions is true/boolean or "true" string. In this case,
	// instructions are meant to be fetched dynamically from the server (e.g., for
	// OAuth servers where instructions are only available after user authentication).