refactor(toolsets): flatten directory structure and clean up tests #204
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge the toolsets directory from a multi-file inheritance structure into a single flat file. Since StackOne is currently the only toolset provider, the abstract base class pattern adds unnecessary complexity. Changes: - Merge base.ts + stackone.ts into src/toolsets.ts - Merge base.test.ts + stackone.test.ts + stackone.mcp-fetch.test.ts into src/toolsets.test.ts - Remove src/toolsets/ directory entirely - Update import paths from '../x' to './x' The public API remains unchanged - consumers still import from '@stackone/ai' with the same exports (StackOneToolSet, ToolSetError, ToolSetConfigError, ToolSetLoadError, AuthenticationConfig, BaseToolSetConfig, StackOneToolSetConfig). If multiple providers are needed in future, the inheritance structure can be re-introduced.
Add tests for the createMCPClient function covering: - Client creation with required options - Client creation with custom headers - asyncDispose cleanup functionality - Integration test for connecting and listing tools This provides direct test coverage for mcp-client.ts, complementing the integration tests in toolsets.test.ts.
Remove tests that only verify object instantiation without testing actual behaviour. These tests provide no value as they only check that constructors return defined objects or have expected property types, which is already guaranteed by TypeScript compilation. Removed tests: - StackOneTool: 'should initialize with correct properties' - Tools: 'should initialize with tools array' - Tool: 'should initialize with correct properties' - StackOneToolSet: 'should initialise with default values' - Module Exports: entire file (src/index.test.ts) Also renamed RequestBuilder test from 'should initialize with correct properties' to 'should initialise with headers from constructor' to better describe its actual assertion.
commit: |
There was a problem hiding this comment.
Pull request overview
This PR refactors the toolsets module by consolidating the directory structure from toolsets/base.ts and toolsets/stackone.ts into a single toolsets.ts file. The refactor improves code maintainability by simplifying imports while preserving all functionality. Additionally, it removes trivial tests that only verified object instantiation and adds comprehensive unit tests for the MCP client factory.
Key changes:
- Merged toolset classes into a single module
- Added dedicated MCP client factory tests
- Removed meaningless initialization tests that only checked
toBeDefined()
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/toolsets.ts | New consolidated module containing StackOneToolSet class and all related types, replacing the previous directory structure |
| src/toolsets.test.ts | Comprehensive test suite covering initialization, authentication, glob matching, MCP integration, and filtering |
| src/mcp-client.test.ts | New test file with thorough coverage of MCP client factory creation and lifecycle |
| src/tool.test.ts | Removed 3 trivial initialization tests that only verified toBeDefined() |
| src/modules/requestBuilder.test.ts | Renamed test to better describe its assertion |
| src/index.test.ts | Removed file containing only trivial export verification tests |
| src/toolsets/stackone.ts | Deleted file (moved to toolsets.ts) |
| src/toolsets/stackone.test.ts | Deleted file (consolidated into toolsets.test.ts) |
| src/toolsets/stackone.mcp-fetch.test.ts | Deleted file (consolidated into toolsets.test.ts) |
| src/toolsets/base.test.ts | Deleted file (consolidated into toolsets.test.ts) |
| src/toolsets/index.ts | Deleted file (no longer needed) |
Comments suppressed due to low confidence (2)
src/toolsets.ts:266
- Race condition in concurrent header mutation. When fetching tools for multiple accounts in parallel (lines 250-266), the code mutates the shared
this.headersproperty and then restores it in a finally block. This creates a race condition where concurrent calls can interfere with each other:
const toolsPromises = effectiveAccountIds.map(async (accountId) => {
const originalHeaders = this.headers;
this.headers = tempHeaders; // Multiple promises mutating same property!
try {
const tools = await this.fetchToolsFromMcp();
return tools.toArray();
} finally {
this.headers = originalHeaders; // Can restore wrong headers
}
});
await Promise.all(toolsPromises);Instead of mutating instance state, pass headers as a parameter to fetchToolsFromMcp() or create copies of the headers object to avoid shared mutable state.
src/toolsets.ts:257
- Redundant variable assignment. The variable
tempHeadersis assigned the value ofmergedHeadersbut serves no purpose:
const tempHeaders = mergedHeaders;
const originalHeaders = this.headers;
this.headers = tempHeaders;This can be simplified to directly use mergedHeaders:
const originalHeaders = this.headers;
this.headers = mergedHeaders;💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/mcp-client.test.ts">
<violation number="1" location="src/mcp-client.test.ts:48">
P1: This test makes real network calls to an external server without mocking, making it flaky and inconsistent with other tests in the project that use MSW. Consider mocking the MCP server using MSW's `server.use()` pattern as done in `toolsets.test.ts`.</violation>
</file>
<file name="src/toolsets.ts">
<violation number="1" location="src/toolsets.ts:257">
P1: Rule violated: **Flag Security Vulnerabilities**
Race condition in `fetchTools` causes potential cross-account data leakage. The code mutates `this.headers` inside concurrent `Promise.all` iterations, which can cause one account's headers (including `x-account-id`) to leak into another account's MCP request. Instead of mutating instance state, pass headers as a parameter to `fetchToolsFromMcp()` or create separate instances for each account.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| expect(transportCloseSpy).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| test('createMCPClient can connect and list tools from MCP server', async () => { |
Contributor
There was a problem hiding this comment.
P1: This test makes real network calls to an external server without mocking, making it flaky and inconsistent with other tests in the project that use MSW. Consider mocking the MCP server using MSW's server.use() pattern as done in toolsets.test.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client.test.ts, line 48:
<comment>This test makes real network calls to an external server without mocking, making it flaky and inconsistent with other tests in the project that use MSW. Consider mocking the MCP server using MSW's `server.use()` pattern as done in `toolsets.test.ts`.</comment>
<file context>
@@ -0,0 +1,68 @@
+ expect(transportCloseSpy).toHaveBeenCalledOnce();
+});
+
+test('createMCPClient can connect and list tools from MCP server', async () => {
+ await using mcpClient = await createMCPClient({
+ baseUrl: 'https://api.stackone-dev.com/mcp',
</file context>
| // Create a temporary toolset instance with the account-specific headers | ||
| const tempHeaders = mergedHeaders; | ||
| const originalHeaders = this.headers; | ||
| this.headers = tempHeaders; |
Contributor
There was a problem hiding this comment.
P1: Rule violated: Flag Security Vulnerabilities
Race condition in fetchTools causes potential cross-account data leakage. The code mutates this.headers inside concurrent Promise.all iterations, which can cause one account's headers (including x-account-id) to leak into another account's MCP request. Instead of mutating instance state, pass headers as a parameter to fetchToolsFromMcp() or create separate instances for each account.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/toolsets.ts, line 257:
<comment>Race condition in `fetchTools` causes potential cross-account data leakage. The code mutates `this.headers` inside concurrent `Promise.all` iterations, which can cause one account's headers (including `x-account-id`) to leak into another account's MCP request. Instead of mutating instance state, pass headers as a parameter to `fetchToolsFromMcp()` or create separate instances for each account.</comment>
<file context>
@@ -141,57 +226,71 @@ export abstract class ToolSet {
+ // Create a temporary toolset instance with the account-specific headers
+ const tempHeaders = mergedHeaders;
+ const originalHeaders = this.headers;
+ this.headers = tempHeaders;
+
+ try {
</file context>
Merge the separate 'Tool' describe block into 'StackOneTool' block to eliminate test duplication. The 'Tool' block contained: - 4 unique tests (headers, authentication) - moved to StackOneTool - 2 duplicated tests (execute, toOpenAI) - removed This consolidation improves test organisation and removes redundant coverage of the same BaseTool functionality.
…irectory Move feedback and requestBuilder modules from subdirectories (src/tools/ and src/modules/) directly into src/ to simplify the project structure. This reduces directory nesting and improves discoverability of core modules. Files moved: - src/tools/feedback.ts → src/feedback.ts - src/tools/feedback.test.ts → src/feedback.test.ts - src/modules/requestBuilder.ts → src/requestBuilder.ts - src/modules/requestBuilder.test.ts → src/requestBuilder.test.ts Updated import paths in affected files: - src/index.ts: export path for createFeedbackTool - src/toolsets.ts: import path for createFeedbackTool - src/tool.ts: import path for RequestBuilder - Both test files updated for new relative import paths The empty src/tools/ and src/modules/ directories have been removed. All tests pass successfully.
Merge tool.meta-tools.test.ts and tool.json-schema.test.ts into tool.test.ts to improve test file organisation. This creates a single, comprehensive test file for all tool-related functionality including: - StackOneTool and Tools class tests - Meta search tools tests (BM25 and hybrid search strategies) - Schema validation tests (array items, nested objects, AI SDK integration) Kept tool.test-d.ts separate as it contains type-level tests using Vitest's `expectTypeOf()` for type assertions. All 272 tests passing successfully with no type errors.
- Rename src/schemas/rpc.ts to src/schema.ts - Update import paths in rpc-client.ts to use new location - Remove schemas/ subdirectory This simplifies the module structure by moving schema definitions directly into the src root directory.
NicolasBelissent
approved these changes
Dec 10, 2025
- Resolve merge conflicts adapting new headers module to flat structure - Flatten src/schemas/headers.ts and src/utils/headers.ts into src/headers.ts - Update imports in rpc-client.ts, rpc-client.test.ts, and toolsets.ts - Add normaliseHeaders function and tests to consolidated headers module
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
toolsets/directory structure into a singletoolsets.tsmodule for simpler imports and maintenancetoBeDefined()without testing actual behaviourChanges
toolsets/base.tsandtoolsets/stackone.tsinto a singletoolsets.tsfilemcp-client.test.tsTest plan
Summary by cubic
Flattened the toolsets into a single toolsets.ts module, moved feedback/requestBuilder to src, and consolidated headers/schema modules to simplify imports and maintenance, plus added focused MCP client tests. Public API remains unchanged.
Refactors
Tests
Written for commit 7365e0b. Summary will update automatically on new commits.