-
Notifications
You must be signed in to change notification settings - Fork 3
feat!: remove deprecated OAS-based getTools, migrate to fetchTools only #148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Remove the entire src/openapi/ directory containing: - Generated OpenAPI spec TypeScript files for all verticals (ATS, CRM, Documents, HRIS, IAM, LMS, Marketing, Messaging, Screening, Ticketing) - OpenAPI parser that converted OAS to tool definitions - OpenAPI loader for loading specs from files - Associated tests and snapshots This code is no longer needed as the SDK now exclusively uses fetchTools() to retrieve tool definitions via MCP. BREAKING CHANGE: OpenAPI spec imports from src/openapi/ are removed
Remove scripts/fetch-specs.ts and its test file that were used to download OpenAPI specifications from StackOne's API and generate TypeScript files. These scripts are no longer needed as tool definitions are now fetched dynamically via MCP using fetchTools().
Remove the GitHub Actions workflow that checked for OpenAPI spec updates. This workflow is no longer needed as the SDK no longer bundles pre-generated OpenAPI specifications.
Remove the OpenAPIToolSet class that provided toolset functionality based on OpenAPI specifications: - src/toolsets/openapi.ts - the main OpenAPIToolSet class - src/toolsets/tests/openapi.spec.ts - associated tests - src/toolsets/tests/fixtures/petstore.json - test fixture - src/toolsets/tests/__snapshots__/stackone.spec.ts.snap - snapshots BREAKING CHANGE: OpenAPIToolSet is no longer exported. Use StackOneToolSet.fetchTools() instead.
Remove deprecated methods from StackOneToolSet and base ToolSet: StackOneToolSet: - Remove loadTools() that loaded tools from bundled OAS specs - Remove getStackOneTools() alias - Remove getTools()/getTool() inherited methods - Keep only fetchTools() for MCP-based tool retrieval ToolSet (base): - Remove this.tools property for storing loaded tools - Remove getTools() and getTool() methods - Keep _matchesFilter() and _matchGlob() for filtering BREAKING CHANGE: getTools(), getTool(), loadTools(), and getStackOneTools() methods are removed. Use fetchTools() instead.
Update public exports to remove OpenAPI-related items: src/index.ts: - Remove OpenAPIToolSet export - Remove OpenAPIToolSetConfig type export - Keep StackOneToolSet and related exports src/toolsets/index.ts: - Remove OpenAPIToolSet re-export - Remove OpenAPIToolSetConfig type re-export BREAKING CHANGE: OpenAPIToolSet and OpenAPIToolSetConfig are no longer exported from the package.
Remove the SPECS constant array that listed all supported OpenAPI specification verticals (ats, crm, documents, hris, iam, lms, marketing, messaging, screening, stackone, ticketing). This constant was used by the spec fetching scripts and is no longer needed with the MCP-based approach.
Remove npm scripts that were used for OpenAPI spec management: - fetch:specs - fetched latest OAS from StackOne API - rebuild - ran fetch:specs followed by build - test:scripts - ran tests for the fetch scripts These scripts are no longer needed as tool definitions are now retrieved dynamically via MCP.
Remove the onSuccess hook that cleaned up sourcemap files for the generated OpenAPI spec files. This cleanup step is no longer needed as the OAS generated files have been removed from the codebase.
Remove example files that demonstrated the deprecated OAS-based getTools() API: - openapi-toolset.ts - OpenAPIToolSet usage example - account-id-usage.ts - account ID configuration example - custom-base-url.ts - custom base URL example - error-handling.ts - error handling patterns - experimental-document-handling.ts - document handling example - filters.ts - tool filtering example These examples used the deprecated getTools()/getStackOneTools() methods. See the updated examples for fetchTools() usage.
Update remaining examples to use the MCP-based fetchTools() API: - ai-sdk-integration.ts - AI SDK integration example - human-in-the-loop.ts - human validation workflow example - meta-tools.ts - meta tools for dynamic discovery - openai-integration.ts - OpenAI integration example - index.ts - example index file Changes include: - Replace getTools() with fetchTools() - Update imports to remove OpenAPI dependencies - Use async/await for tool fetching - Update comments to reflect MCP-based approach
Update test files to reflect the removal of OAS-based methods: src/tests/exports.spec.ts: - Remove assertions for OpenAPIToolSet export - Keep assertions for StackOneToolSet and other exports src/toolsets/tests/base.spec.ts: - Remove tests for getTools() and getTool() methods - Keep tests for filtering functionality src/toolsets/tests/stackone.spec.ts: - Remove tests for loadTools(), getTools(), getStackOneTools() - Keep tests for fetchTools() functionality src/toolsets/tests/stackone.mcp-fetch.spec.ts: - Update expected tool counts to include feedback tool (+1) - Add assertions for meta_collect_tool_feedback presence
Rewrite README.md to document the MCP-based fetchTools() API: - Remove all references to getTools() and OpenAPI-based methods - Update Quick Start to use fetchTools() with async/await - Update AI SDK, OpenAI, and MCP integration examples - Add meta tools documentation for dynamic tool discovery - Remove deprecated configuration sections - Simplify authentication documentation BREAKING CHANGE: Documentation now only covers the fetchTools() API. The deprecated getTools() and OpenAPIToolSet APIs are no longer documented.
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes all deprecated OpenAPI Specification (OAS) based tool generation infrastructure and migrates exclusively to the MCP-based fetchTools() API. This is a significant breaking change that simplifies the codebase by removing over 8,000 lines of code related to OAS parsing, generated specifications, and associated tooling.
Key changes:
- Removed
getTools(),getTool(),loadTools(), andgetStackOneTools()methods - Deleted
OpenAPIToolSetclass and all OpenAPI parser/loader infrastructure - Removed
src/openapi/directory entirely (parser, loader, generated specs, tests) - Updated all examples and tests to use
fetchTools()API - Removed build hooks for OAS-generated code cleanup
Reviewed changes
Copilot reviewed 33 out of 48 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsdown.config.ts | Removed build hooks for cleaning up OAS-generated sourcemaps |
| src/toolsets/tests/stackone.spec.ts | Removed OAS-based tests, updated to test only MCP configuration |
| src/toolsets/tests/stackone.mcp-fetch.spec.ts | Updated test expectations to account for feedback tool inclusion |
| src/toolsets/tests/openapi.spec.ts | Deleted entire OAS toolset test file |
| src/toolsets/tests/fixtures/petstore.json | Deleted test fixture for OAS parsing |
| src/toolsets/tests/base.spec.ts | Removed tests for deprecated getTools() method |
| src/toolsets/stackone.ts | Removed OAS loading methods, kept only fetchTools() |
| src/toolsets/openapi.ts | Deleted entire OAS toolset implementation |
| src/toolsets/index.ts | Removed OpenAPI toolset exports |
| src/toolsets/base.ts | Removed getTools(), getTool(), and tools property |
| src/tests/exports.spec.ts | Updated to check for BaseTool and createFeedbackTool instead of OAS exports |
| src/openapi/* | Deleted parser, loader, and all generated specifications |
| src/index.ts | Removed OpenAPI-related exports |
| src/constants.ts | Removed SPECS array, updated comment spelling to British English |
| scripts/fetch-specs.ts | Deleted OAS fetching script |
| package.json | Removed fetch:specs, rebuild, and test:scripts commands |
| examples/* | Updated to use fetchTools() API, deleted OAS-specific examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * | ||
| * - alpha=0.2 means: 20% BM25 + 80% TF-IDF | ||
| * - This value was optimized through validation testing | ||
| * - This value was optimised through validation testing |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment uses British English spelling "optimised" while other parts of the codebase use American English spelling "optimized". For consistency, this should use "optimized" to match the original.
| * - This value was optimised through validation testing | |
| * - This value was optimized through validation testing |
| * - Lower values favour BM25 scoring (better keyword matching) | ||
| * - Higher values favour TF-IDF scoring (better semantic matching) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment uses British English spelling "favour" while other parts of the codebase use American English spelling "favor". For consistency, this should use "favor".
| * - Lower values favour BM25 scoring (better keyword matching) | |
| * - Higher values favour TF-IDF scoring (better semantic matching) | |
| * - Lower values favor BM25 scoring (better keyword matching) | |
| * - Higher values favor TF-IDF scoring (better semantic matching) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 48 files
| // Initialise StackOne | ||
| const toolset = new StackOneToolSet({ | ||
| accountId: ACCOUNT_IDS.HRIS, | ||
| baseUrl: process.env.STACKONE_BASE_URL ?? 'https://api.stackone.com', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a required parameter? we should just use default. I don't want to expose this baseUrl if we don't have to - it seems only relevant to our internal teams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's required actually.
we gonna fix it in other pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#149
i'll do it later
| // Use max steps to automatically call the tool if it's needed | ||
| const { text } = await generateText({ | ||
| model: openai('gpt-5'), | ||
| model: openai('gpt-4o'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do something more recent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh bad, it's vibe coded...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glebedel we have lots of gpt-4 models including examles and readme, i'll do it in another PR
| import process from 'node:process'; | ||
| import OpenAI from 'openai'; | ||
| import { StackOneToolSet } from '../src'; | ||
| import { ACCOUNT_IDS } from './constants'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense to ever have account_ids in constants vs fetching them from some kind of function or just setting them inline for the purpose of having examples, eg const accountId =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So they have already been implemented before. This PR just deletes all traditional getTools, so it should be dealt with in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to create another pr for it
| const isPlaceholderKey = !apiKey || apiKey === 'test-stackone-key'; | ||
| const shouldSkip = process.env.SKIP_FETCH_TOOLS_EXAMPLE !== '0' && isPlaceholderKey; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this should be a a per tool configuration rather than a based on an env variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it is not my implementation. SKIP_FETCH_TOOLS_EXAMPLE is already there. i gonna create another example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/StackOneHQ/stackone-ai-node/tree/feat/migrate-to-pnpm-vitest
i'm working on getting rid of this "anti pattern of env variable usage" in this branch, so i gonna fix it soon.
|
@glebedel Overall, the aim of this PR is to get rid of the
In this PR, we keep the existing behavior. We should fix them in separate PRs. |
glebedel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ders Remove the centralised constants.ts file containing hardcoded account IDs and replace with inline placeholder values in each example file. This change addresses feedback from PR #148 review by @glebedel: - Account IDs should not be stored in shared constants - Examples are clearer with inline values that users replace directly - Avoids exposing internal implementation details Changes: - Delete examples/constants.ts with hardcoded account IDs - Update all example files to use inline placeholder accountId variables - Each example now has a clear comment: "Replace with your actual account ID" - Update README.md to reflect new pattern and remove constants.ts references Closes #196
…ders (#198) Remove the centralised constants.ts file containing hardcoded account IDs and replace with inline placeholder values in each example file. This change addresses feedback from PR #148 review by @glebedel: - Account IDs should not be stored in shared constants - Examples are clearer with inline values that users replace directly - Avoids exposing internal implementation details Changes: - Delete examples/constants.ts with hardcoded account IDs - Update all example files to use inline placeholder accountId variables - Each example now has a clear comment: "Replace with your actual account ID" - Update README.md to reflect new pattern and remove constants.ts references Closes #196
Summary
Remove all deprecated OpenAPI Specification (OAS) based tool generation and migrate exclusively to the MCP-based
fetchTools()API.Breaking Changes
getTools(),getTool(),loadTools(), andgetStackOneTools()methods removedOpenAPIToolSetclass removedsrc/openapi/removedOpenAPIToolSetConfigtype removedChanges
Deleted:
src/openapi/directory (parser, loader, generated specs, tests)scripts/fetch-specs.tsand tests.github/workflows/check-oas-updates.ymlsrc/toolsets/openapi.tsgetTools()Modified:
StackOneToolSet- now only exposesfetchTools()ToolSet- removedthis.tools,getTools(),getTool()fetchTools()Migration
Before:
After:
Test plan
Summary by cubic
Removed all OpenAPI (OAS) tooling and moved the SDK to a single MCP-based fetchTools() API. This breaking change simplifies the API, removes OAS maintenance, and updates docs, examples, tests, and CI.
Refactors
Migration
Written for commit 84d7750. Summary will update automatically on new commits.