Ally remote indexing #193

manoj-k04 · 2025-12-08T12:28:24Z

No description provided.

…case-functionality feat: Add updateTestCase functionality to Test Management

Copilot

Pull request overview

This PR introduces remote repository indexing functionality that enables the BrowserStack MCP Server to fetch and index files from GitHub repositories. The implementation uses Octokit to integrate with GitHub's API and provides tools for accessing remote repository content.

Key Changes

Added GitHub repository integration using Octokit SDK for fetching remote repository files
Implemented indexing system to cache repository structure for efficient file lookups
Created fetchFromRepo tool and design repository context resource for accessing indexed files

Reviewed changes

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

Show a summary per file

File	Description
`src/tools/remote-repo.ts`	Main integration point that initializes remote repository tools using environment variables
`src/tools/fetch-from-repo.ts`	Implements tool for fetching individual files from indexed repositories
`src/resources/design-repo-context.ts`	Provides resource for aggregating design-related files from the repository
`src/repo/repo-setup.ts`	Setup function that orchestrates fetcher and indexer initialization
`src/repo/RepositoryFetcher.ts`	Interface defining the contract for repository fetching operations
`src/repo/RemoteIndexer.ts`	Implements in-memory indexing of repository file structure
`src/repo/GitHubRepositoryFetcher.ts`	GitHub-specific implementation of the RepositoryFetcher interface
`src/server-factory.ts`	Registers the new remote repository tools with the MCP server
`package.json`	Adds octokit v5.0.5 dependency for GitHub API integration
`package-lock.json`	Lock file updates for octokit and its transitive dependencies

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

Copilot · 2025-12-08T13:19:03Z

src/tools/fetch-from-repo.ts

+      } catch (error: any) {
+        return {
+          content: [
+            {
+              type: "text" as const,
+              text: `Error fetching file: ${error.message}`,


Using any type for the error weakens type safety. Consider using error: unknown and then type-narrowing to check for .message property, or use a more specific error type.

Suggested change

} catch (error: any) {

return {

content: [

{

type: "text" as const,

text: `Error fetching file: ${error.message}`,

} catch (error: unknown) {

let errorMessage = "Unknown error";

if (

typeof error === "object" &&

error !== null &&

"message" in error &&

typeof (error as { message?: unknown }).message === "string"

) {

errorMessage = (error as { message: string }).message;

}

return {

content: [

{

type: "text" as const,

text: `Error fetching file: ${errorMessage}`,

Copilot · 2025-12-08T13:19:04Z

src/tools/remote-repo.ts

+  if (!githubToken || !repoOwner || !repoName) {
+    logger.info(
+      "Remote repository integration not configured. Skipping remote repo tools.",
+    );
+    return tools;
+  }


Missing validation for the GitHub token and repository configuration. If the token is invalid or doesn't have the necessary permissions, the error will only surface when setupRemoteRepoMCP fails. Consider validating the token format and checking basic repository access before proceeding with setup.

Copilot · 2025-12-08T13:19:04Z

src/repo/RemoteIndexer.ts

+    const tree = await this.fetcher.getTree();
+    tree.forEach((item: RepoFileMeta) => this.index.set(item.path, item));


The buildIndex() method lacks error handling. If fetcher.getTree() fails (e.g., due to network issues, authentication problems, or rate limiting), the error will propagate uncaught. Consider adding try-catch with appropriate error handling or documenting that callers must handle errors.

Suggested change

const tree = await this.fetcher.getTree();

tree.forEach((item: RepoFileMeta) => this.index.set(item.path, item));

try {

const tree = await this.fetcher.getTree();

tree.forEach((item: RepoFileMeta) => this.index.set(item.path, item));

} catch (error) {

// Handle error appropriately: log and rethrow, or handle as needed

console.error("Failed to build index:", error);

throw error;

}

Copilot · 2025-12-08T13:19:04Z

src/repo/RepositoryFetcher.ts

+export interface RepoFileMeta {
+  path: string;
+  sha: string;
+  type: "blob" | "tree";
+  size?: number;
+}
+
+export interface RepositoryFetcher {
+  getTree(): Promise<RepoFileMeta[]>; // indexing
+  getFileContent(path: string): Promise<string>; // actual file fetching


The RepositoryFetcher interface and RepoFileMeta interface lack documentation. Consider adding JSDoc comments to explain:

The purpose of each method

What each property in RepoFileMeta represents

When size might be undefined

Expected error conditions and how they should be handled

Suggested change

export interface RepoFileMeta {

path: string;

sha: string;

type: "blob" | "tree";

size?: number;

}

export interface RepositoryFetcher {

getTree(): Promise<RepoFileMeta[]>; // indexing

getFileContent(path: string): Promise<string>; // actual file fetching

/**

* Metadata about a file or directory in a repository.

*/

export interface RepoFileMeta {

/**

* The path of the file or directory relative to the repository root.

*/

path: string;

/**

* The SHA hash of the file or directory.

*/

sha: string;

/**

* The type of the entry: "blob" for files, "tree" for directories.

*/

type: "blob" | "tree";

/**

* The size of the file in bytes. May be undefined for directories ("tree") or if the size is not available.

*/

size?: number;

}

/**

* Interface for fetching repository data such as file trees and file contents.

*/

export interface RepositoryFetcher {

/**

* Fetches the file tree of the repository.

* @returns A promise that resolves to an array of RepoFileMeta objects representing files and directories.

* @throws May reject if the repository cannot be accessed or the tree cannot be fetched.

*/

getTree(): Promise<RepoFileMeta[]>;

/**

* Fetches the content of a file at the given path.

* @param path - The path to the file relative to the repository root.

* @returns A promise that resolves to the file content as a string.

* @throws May reject if the file does not exist, cannot be accessed, or if there is a network or permission error.

*/

getFileContent(path: string): Promise<string>;

Copilot · 2025-12-08T13:19:05Z

src/repo/RemoteIndexer.ts

+export class RemoteIndexer {
+  private index: Map<string, RepoFileMeta> = new Map();
+
+  constructor(private fetcher: RepositoryFetcher) {}


The RemoteIndexer class lacks documentation. Consider adding JSDoc comments to explain:

The purpose and lifecycle of the class

When buildIndex() should be called

Whether buildIndex() can be called multiple times to refresh the index

Thread safety considerations if any

Copilot · 2025-12-08T13:19:07Z

src/tools/remote-repo.ts

+  // Set up remote repo asynchronously
+  setupRemoteRepoMCP({
+    owner: repoOwner,
+    repo: repoName,
+    branch: repoBranch,
+    token: githubToken,
+  })
+    .then(({ fetchFromRepoTool, contextResource, indexer }) => {
+      // Register the fetch tool
+      const registeredTool = server.tool(
+        fetchFromRepoTool.name,
+        fetchFromRepoTool.description,
+        fetchFromRepoTool.inputSchema.shape,
+        fetchFromRepoTool.handler,
+      );
+
+      tools[fetchFromRepoTool.name] = registeredTool;
+
+      // Register the context resource
+      server.resource(contextResource.name, contextResource.uri, async () =>
+        contextResource.handler(),
+      );
+
+      logger.info(
+        "Remote repository tools registered successfully. Indexed %d files.",
+        indexer.getIndex().size,
+      );
+    })
+    .catch((error) => {
+      logger.error(
+        "Failed to set up remote repository integration: %s",
+        error.message,
+      );
+    });
+
+  return tools;


The setupRemoteRepoMCP function is called asynchronously with .then()/.catch(), but the addRemoteRepoTools function returns immediately with an empty tools object. This means that when the server tries to use these tools, they won't be registered yet, leading to a race condition. The tools will only be added to the local tools object after the promise resolves, but the returned object is already passed to the caller.

Consider either:

Making addRemoteRepoTools async and awaiting the setup

Returning a promise that resolves when tools are ready

Registering tools synchronously and deferring only the indexing operation

Copilot · 2025-12-08T13:19:07Z

src/repo/GitHubRepositoryFetcher.ts

+      recursive: "true",
+    });
+
+    return treeRes.data.tree.map((item: any) => ({


Using any type weakens type safety. The GitHub API's tree response has a well-defined structure. Consider using the proper Octokit types instead of any, such as RestEndpointMethodTypes["git"]["getTree"]["response"]["data"]["tree"][number].

Copilot · 2025-12-08T13:19:07Z

src/repo/GitHubRepositoryFetcher.ts

+  async getTree(): Promise<RepoFileMeta[]> {
+    const treeRes = await this.client.rest.git.getTree({
+      owner: this.owner,
+      repo: this.repo,
+      tree_sha: this.branch,
+      recursive: "true",
+    });
+
+    return treeRes.data.tree.map((item: any) => ({
+      path: item.path!,
+      sha: item.sha!,
+      type: item.type as "blob" | "tree",
+      size: item.size,
+    }));


The getTree() method fetches the entire repository tree recursively, which could be very slow and memory-intensive for large repositories. Consider:

Adding pagination support

Implementing lazy loading or filtering at the API level

Adding a timeout mechanism

Documenting the performance implications in the interface comments

Suggested change

async getTree(): Promise<RepoFileMeta[]> {

const treeRes = await this.client.rest.git.getTree({

owner: this.owner,

repo: this.repo,

tree_sha: this.branch,

recursive: "true",

});

return treeRes.data.tree.map((item: any) => ({

path: item.path!,

sha: item.sha!,

type: item.type as "blob" | "tree",

size: item.size,

}));

/**

* Fetches the entire repository tree recursively.

* WARNING: For large repositories, this can be very slow and memory-intensive.

* Consider using the `filter` argument to limit results, and be aware of possible timeouts.

* @param filter Optional filter function to select which files to include.

* @param timeoutMs Optional timeout in milliseconds (default: 10000ms).

*/

async getTree(

filter?: (item: RepoFileMeta) => boolean,

timeoutMs: number = 10000

): Promise<RepoFileMeta[]> {

// Add a timeout to the API call

const controller = new AbortController();

const timeout = setTimeout(() => controller.abort(), timeoutMs);

let treeRes;

try {

treeRes = await this.client.rest.git.getTree({

owner: this.owner,

repo: this.repo,

tree_sha: this.branch,

recursive: "true",

request: { signal: controller.signal }

});

} catch (err: any) {

if (err.name === "AbortError") {

throw new Error(`getTree request timed out after ${timeoutMs}ms`);

}

throw err;

} finally {

clearTimeout(timeout);

}

let files = treeRes.data.tree.map((item: any) => ({

path: item.path!,

sha: item.sha!,

type: item.type as "blob" | "tree",

size: item.size,

}));

if (filter) {

files = files.filter(filter);

}

return files;

Copilot · 2025-12-08T13:19:07Z

src/resources/design-repo-context.ts

+        } catch (error: any) {
+          parts.push(`### File: ${path}\nError: ${error.message}`);


Using any type for the error weakens type safety. Consider using error: unknown and then type-narrowing to check for .message property, or use a more specific error type.

Suggested change

} catch (error: any) {

parts.push(`### File: ${path}\nError: ${error.message}`);

} catch (error: unknown) {

let errorMessage = "Unknown error";

if (typeof error === "object" && error !== null && "message" in error && typeof (error as any).message === "string") {

errorMessage = (error as { message: string }).message;

}

parts.push(`### File: ${path}\nError: ${errorMessage}`);

Copilot · 2025-12-08T13:19:08Z

src/tools/remote-repo.ts

+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  _config: BrowserStackConfig,


[nitpick] The _config parameter is declared but unused, with an eslint-disable comment. If the config parameter is not needed for this function, consider removing it entirely rather than keeping it with a disable comment.

Suggested change

// eslint-disable-next-line @typescript-eslint/no-unused-vars

_config: BrowserStackConfig,

gaurav-singh-9227 and others added 2 commits December 3, 2025 18:26

Merge pull request browserstack#186 from manoj-k04/feature/edit-test-…

764f5b7

…case-functionality feat: Add updateTestCase functionality to Test Management

feat: Add remote repository integration for MCP

628293d

manoj-k04 requested a review from Copilot December 8, 2025 13:12

Copilot started reviewing on behalf of manoj-k04 December 8, 2025 13:12 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ally remote indexing #193

Ally remote indexing #193

Uh oh!

manoj-k04 commented Dec 8, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const tree = await this.fetcher.getTree();
		tree.forEach((item: RepoFileMeta) => this.index.set(item.path, item));

		} catch (error: any) {
		parts.push(`### File: ${path}\nError: ${error.message}`);

-        } catch (error: any) {
-          parts.push(`### File: ${path}\nError: ${error.message}`);
+        } catch (error: unknown) {
+          let errorMessage = "Unknown error";
+          if (typeof error === "object" && error !== null && "message" in error && typeof (error as any).message === "string") {
+            errorMessage = (error as { message: string }).message;
+          }
+          parts.push(`### File: ${path}\nError: ${errorMessage}`);

		// eslint-disable-next-line @typescript-eslint/no-unused-vars
		_config: BrowserStackConfig,

Ally remote indexing #193

Are you sure you want to change the base?

Ally remote indexing #193

Uh oh!

Conversation

manoj-k04 commented Dec 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants