fix(toolsets): add accountIds option to StackOneToolSetConfig#252
Merged
fix(toolsets): add accountIds option to StackOneToolSetConfig#252
Conversation
Allow passing multiple account IDs when initialising StackOneToolSet
via the constructor, eliminating the need to call setAccounts()
separately after instantiation.
- Add accountIds property to StackOneToolSetConfig interface with
JSDoc documentation
- Initialise accountIds from config in constructor
- Update constructor JSDoc to reflect support for multiple account IDs
This change enables a more ergonomic API for multi-account setups:
```typescript
const toolset = new StackOneToolSet({
apiKey: 'key',
accountIds: ['acc1', 'acc2', 'acc3'],
});
```
Closes #251
Add comprehensive tests verifying the new accountIds configuration: - Test initialising with multiple account IDs from constructor - Test default empty array when accountIds not provided - Test coexistence of accountId and accountIds in constructor - Test fetchTools uses constructor accountIds when present - Test setAccounts() overrides constructor accountIds These tests ensure accountIds from the constructor integrates correctly with existing setAccounts() and fetchTools() behaviours.
commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a convenient accountIds option to the StackOneToolSetConfig interface, enabling single-step initialization of a toolset with multiple account IDs. Previously, users needed to instantiate the toolset and then call setAccounts() separately. This change maintains full backward compatibility with existing code while providing a more ergonomic API.
Key Changes
- Added
accountIdsarray property toStackOneToolSetConfiginterface with comprehensive JSDoc - Constructor now initializes
this.accountIdsfrom config, defaulting to empty array - Updated constructor JSDoc to reference "account ID(s)" plural form
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/toolsets.ts | Adds accountIds optional property to config interface with JSDoc and initializes it in constructor (line 185) |
| src/toolsets.test.ts | Comprehensive test coverage including constructor initialization, default values, coexistence with accountId, and interaction with setAccounts() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+97
to
+98
| * Array of account IDs for filtering tools across multiple accounts | ||
| * When provided, tools will be fetched for all specified accounts |
There was a problem hiding this comment.
The JSDoc states "When provided, tools will be fetched for all specified accounts" but this is imprecise. The accountIds are stored during construction, but tools are only fetched when fetchTools() is called later. Consider rephrasing to something like "When provided, these account IDs will be used when fetching tools" or "Array of account IDs to use for filtering tools during fetchTools() calls".
Suggested change
| * Array of account IDs for filtering tools across multiple accounts | |
| * When provided, tools will be fetched for all specified accounts | |
| * Array of account IDs to use for filtering tools during fetchTools() calls. | |
| * Only tools available on these accounts will be returned when fetchTools() is called. |
… options
Use type-fest's MergeExclusive to enforce that either accountId (single)
or accountIds (multiple) can be provided, but not both simultaneously.
This improves type safety by catching invalid configurations at compile
time rather than runtime. The API now clearly communicates the intended
usage pattern through the type system.
Before (both allowed - confusing behaviour):
```typescript
new StackOneToolSet({
accountId: 'acc1',
accountIds: ['acc2', 'acc3'], // Which takes precedence?
});
```
After (type error - clear contract):
```typescript
// Valid: single account
new StackOneToolSet({ accountId: 'acc1' });
// Valid: multiple accounts
new StackOneToolSet({ accountIds: ['acc1', 'acc2'] });
// Type error: cannot use both
new StackOneToolSet({ accountId: 'acc1', accountIds: ['acc2'] });
```
Update test to verify that the type system enforces mutual exclusivity between accountId and accountIds. Replace the test that allowed both with a new test demonstrating the correct API usage patterns. - Test single accountId configuration works correctly - Test multiple accountIds configuration works correctly - Document that combining both is a type error (prevented at compile time)
Add comprehensive type tests using vitest's expectTypeOf to verify the MergeExclusive behaviour of account configuration options: - Test that accountId alone is valid - Test that accountIds alone is valid - Test that neither is valid (optional) - Test that both together is rejected by the type system - Verify accountId is typed as string | undefined - Verify accountIds is typed as string[] | undefined
ryoppippi
force-pushed
the
feat/multi-account-initialisation
branch
from
e6fbcc6 to
cac507c
Compare
… options Add runtime check to throw ToolSetConfigError when both accountId and accountIds are provided simultaneously. This protects JavaScript users and scenarios where TypeScript is bypassed (e.g., using 'as any'). The validation uses != null to check for both null and undefined in a single comparison, ensuring the error is thrown regardless of how the invalid configuration is constructed.
…counts Add test to verify ToolSetConfigError is thrown when both accountId and accountIds are provided at runtime. Uses 'as never' to bypass TypeScript type checking and simulate JavaScript usage scenarios.
ryoppippi
changed the title
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
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
accountIdsoption toStackOneToolSetConfigfor passing multiple accounts during class initialisationMergeExclusiveto enforce mutual exclusivity betweenaccountIdandaccountIdsat compile timeToolSetConfigErrorwhen both options are provided (protects JavaScript users)expectTypeOfThis enables a more ergonomic API with type safety:
Closes #251
Test plan
accountIdaloneaccountIdsaloneToolSetConfigErrorwhen both providedexpectTypeOf().toExtend()for valid configsexpectTypeOf().not.toExtend()for invalid config (both options)fetchToolsusing constructoraccountIdssetAccounts()overrides constructoraccountIds