-
Notifications
You must be signed in to change notification settings - Fork 57
fix: update KB and WS connector devEx #1681
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
Changes from all commits
00e8114
f298276
19251ca
64a5e96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -394,7 +394,9 @@ | |
| options.language = | ||
| (matchEnumValue(TargetLanguageSchema, options.language) as typeof options.language) ?? options.language; | ||
|
|
||
| if (!options.name) { | ||
| const kbConnectors = ['bedrock-knowledge-bases']; | ||
| const nameOptional = options.type === 'connector' && kbConnectors.includes(options.connector ?? ''); | ||
| if (!options.name && !nameOptional) { | ||
| return { valid: false, error: '--name is required' }; | ||
| } | ||
|
|
||
|
|
@@ -407,7 +409,6 @@ | |
| 'http-runtime', | ||
| 'connector', | ||
| 'passthrough', | ||
| 'web-search', | ||
| ].join(', '); | ||
|
|
||
| if (!options.type) { | ||
|
|
@@ -426,7 +427,6 @@ | |
| 'http-runtime': 'httpRuntime', | ||
| connector: 'connector', | ||
| passthrough: 'passthrough', | ||
| 'web-search': 'webSearch', | ||
| }; | ||
| const mappedType = typeMap[options.type]; | ||
| if (!mappedType) { | ||
|
|
@@ -437,11 +437,11 @@ | |
| } | ||
| options.type = mappedType; | ||
|
|
||
| // --exclude-domains is webSearch-target-only. Reject it on every other target type. | ||
| if (mappedType !== 'webSearch' && options.excludeDomains) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We should consider a cleaner way to do flag validation in our rf. |
||
| // --exclude-domains only applies to the web-search connector. | ||
| if (options.excludeDomains && !(mappedType === 'connector' && options.connector === 'web-search')) { | ||
| return { | ||
| valid: false, | ||
| error: '--exclude-domains only applies to --type web-search', | ||
| error: '--exclude-domains only applies to --connector web-search', | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -630,43 +630,6 @@ | |
| return { valid: true }; | ||
| } | ||
|
|
||
| // Web search targets (Amazon Web Search managed connector): validate early and return | ||
| if (mappedType === 'webSearch') { | ||
| const WEB_SEARCH_DISALLOWED_OPTIONS: [string, string][] = [ | ||
| ['connector', '--connector'], | ||
| ['knowledgeBaseId', '--knowledge-base-id'], | ||
| ['endpoint', '--endpoint'], | ||
| ['host', '--host'], | ||
| ['restApiId', '--rest-api-id'], | ||
| ['stage', '--stage'], | ||
| ['lambdaArn', '--lambda-arn'], | ||
| ['toolSchemaFile', '--tool-schema-file'], | ||
| ['toolFilterPath', '--tool-filter-path'], | ||
| ['toolFilterMethods', '--tool-filter-methods'], | ||
| ['schema', '--schema'], | ||
| ['schemaS3Account', '--schema-s3-account'], | ||
| ['outboundAuthType', '--outbound-auth'], | ||
| ['credentialName', '--credential-name'], | ||
| ['oauthClientId', '--oauth-client-id'], | ||
| ['oauthClientSecret', '--oauth-client-secret'], | ||
| ['oauthDiscoveryUrl', '--oauth-discovery-url'], | ||
| ['oauthScopes', '--oauth-scopes'], | ||
| ]; | ||
| for (const [key, flag] of WEB_SEARCH_DISALLOWED_OPTIONS) { | ||
| const v = (options as unknown as Record<string, unknown>)[key]; | ||
| // knowledgeBaseId is a string[]; treat empty array as absent | ||
| const present = Array.isArray(v) ? v.length > 0 : !!v; | ||
| if (present) { | ||
| return { valid: false, error: `${flag} is not applicable for web-search type` }; | ||
| } | ||
| } | ||
| if (options.language && options.language !== 'Other') { | ||
| return { valid: false, error: '--language is not applicable for web-search type' }; | ||
| } | ||
| options.language = 'Other'; | ||
| return { valid: true }; | ||
| } | ||
|
|
||
| // Connector targets (Bedrock KB, agentic-retrieve): validate early and return | ||
| if (mappedType === 'connector') { | ||
| const validConnectors = CONNECTOR_ID_VALUES.join(', '); | ||
|
|
@@ -682,17 +645,22 @@ | |
| error: `Invalid --connector "${options.connector}". Valid: ${validConnectors}`, | ||
| }; | ||
| } | ||
| if (!options.knowledgeBaseId || options.knowledgeBaseId.length === 0) { | ||
| if (options.connector === 'web-search' && options.knowledgeBaseId && options.knowledgeBaseId.length > 0) { | ||
| return { | ||
| valid: false, | ||
| error: '--knowledge-base-id is not applicable for --connector web-search', | ||
| }; | ||
| } | ||
| if (options.connector !== 'web-search' && (!options.knowledgeBaseId || options.knowledgeBaseId.length === 0)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctness — lost early error] The connector
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added |
||
| return { | ||
| valid: false, | ||
| error: `--knowledge-base-id is required for --connector ${options.connector}`, | ||
| }; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This skips the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added explicit check & rejection |
||
| if (options.connector === 'bedrock-knowledge-bases' && options.knowledgeBaseId.length > 1) { | ||
| if (options.connector === 'bedrock-knowledge-bases' && (options.knowledgeBaseId?.length ?? 0) > 1) { | ||
| return { | ||
| valid: false, | ||
| error: | ||
| '--knowledge-base-id may only be specified once for --connector bedrock-knowledge-bases. Use --connector bedrock-agentic-retrieve for fan-out.', | ||
| error: '--knowledge-base-id may only be specified once for --connector bedrock-knowledge-bases.', | ||
| }; | ||
| } | ||
| const irrelevant: [string, string][] = [ | ||
|
|
@@ -704,6 +672,8 @@ | |
| ['toolSchemaFile', '--tool-schema-file'], | ||
| ['toolFilterPath', '--tool-filter-path'], | ||
| ['toolFilterMethods', '--tool-filter-methods'], | ||
| ['schema', '--schema'], | ||
| ['schemaS3Account', '--schema-s3-account'], | ||
| ['outboundAuthType', '--outbound-auth'], | ||
| ['credentialName', '--credential-name'], | ||
| ['oauthClientId', '--oauth-client-id'], | ||
|
|
@@ -729,7 +699,7 @@ | |
| if (!passthroughEndpoint) { | ||
| return { valid: false, error: '--passthrough-endpoint is required for passthrough type' }; | ||
| } | ||
| if (!/^https:\/\/[a-zA-Z0-9\-.]+(:[0-9]{1,5})?(\/.*)?$/.test(passthroughEndpoint)) { | ||
| return { valid: false, error: '--passthrough-endpoint must be a valid HTTPS URL' }; | ||
| } | ||
| if (options.language && options.language !== 'Other') { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| export { translateConnector } from './translators'; | ||
| export type { | ||
| ConfigurationEntry, | ||
| ConnectorTranslatorInput, | ||
| ParameterOverride, | ||
| WebSearchTranslatorInput, | ||
| KnowledgeBasesTranslatorInput, | ||
| } from './translators'; |
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.
nameOptionalonly covers the two KB connectors, so--connector web-searchwithout--namestill hits the--name is requiredbranch even though the primitive atGatewayTargetPrimitive.ts:665-669has default-name logic for the KB connectors only. If the PR intent is "defaults for KB, name required for web-search", that's consistent — but the PR description says "A default value is given for both KB creation, and attaching connector if none is provided", which reads as "for all connectors". Please either extend the default to web-search too (e.g.web-searchorweb-search-${gateway}) or clarify the description.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.
This is intentional, I've updated PR description to clarify