[onechat] ensure built-in tools have a valid id#227835
[onechat] ensure built-in tools have a valid id#227835pgayvallet merged 4 commits intoelastic:mainfrom
Conversation
|
/ci |
|
/ci |
| export type BuiltinToolId = `.${string}`; | ||
|
|
||
| /** | ||
| * Onechat tool, as registered by built-in tool providers. | ||
| */ | ||
| export interface BuiltinToolDefinition< | ||
| RunInput extends ZodObject<any> = ZodObject<any>, | ||
| RunOutput = unknown | ||
| > extends Omit<ToolDefinition, 'type' | 'configuration'> { | ||
| > extends Omit<ToolDefinition, 'id' | 'type' | 'configuration'> { | ||
| /** | ||
| * Built-in tool ID following the {@link BuiltinToolId} pattern | ||
| */ | ||
| id: BuiltinToolId; |
There was a problem hiding this comment.
Some TS magic to add TS type validation in addition to runtime validation
There was a problem hiding this comment.
shouldn't type property be always set as builtin type? is tagged union appropriate here?
(if builtin is enum not type not sure it will work)
There was a problem hiding this comment.
There's no type property on BuiltinToolDefinition, we add it automatically in the internals when we convert it to tool definition (we have a few different hybrid types for tool registration)
| }; | ||
|
|
||
| export const sanitizeToolId = (toolId: string): string => { | ||
| return toolId.replace(/[^a-zA-Z0-9_-]/g, ''); |
There was a problem hiding this comment.
observation: I think this is the 3rd place in onechat plugin where we define a very similar regex (here as exclusion but still the same), we already have sth like that for tools and agents
| export type BuiltinToolId = `.${string}`; | ||
|
|
||
| /** | ||
| * Onechat tool, as registered by built-in tool providers. | ||
| */ | ||
| export interface BuiltinToolDefinition< | ||
| RunInput extends ZodObject<any> = ZodObject<any>, | ||
| RunOutput = unknown | ||
| > extends Omit<ToolDefinition, 'type' | 'configuration'> { | ||
| > extends Omit<ToolDefinition, 'id' | 'type' | 'configuration'> { | ||
| /** | ||
| * Built-in tool ID following the {@link BuiltinToolId} pattern | ||
| */ | ||
| id: BuiltinToolId; |
There was a problem hiding this comment.
shouldn't type property be always set as builtin type? is tagged union appropriate here?
(if builtin is enum not type not sure it will work)
| import { createBadRequestError, isReservedToolId } from '@kbn/onechat-common'; | ||
|
|
||
| const idRegexp = /^[a-z0-9](?:[a-z0-9_-]*[a-z0-9])?$/; | ||
| const builtinToolIdRegexp = /^[.][a-z0-9](?:[a-z0-9_-]*[a-z0-9])?$/; |
There was a problem hiding this comment.
some regex duplication, but as const so should be fine
There was a problem hiding this comment.
I already forgot why we can't have id start an end with _ or -. I copied the idRegexp from somewhere
i tested in ES rn, and it seems that we are fine creating ids starting with those chars. So perhaps this regex can be simplified to good old a-zA-Z0-9_-
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
History
|
## Summary Follow-up of elastic#227452 Enforce `builtin` tools follow the expected convention for ids (starting with `.`) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary Follow-up of elastic#227452 Enforce `builtin` tools follow the expected convention for ids (starting with `.`) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Follow-up of #227452
Enforce
builtintools follow the expected convention for ids (starting with.)