fix: update KB and WS connector devEx#1681
Conversation
github-actions
Bot
added
the
agentcore-harness-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
Package TarballHow to installgh release download pr-1681-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.22.0.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
agentcore-cli-automation
left a comment
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Nice cleanup — removing the standalone web-search command tree and folding it into --connector reads well. A few things need attention before merge:
Blocking: unrelated test files were not updated for the schema/primitive changes
The PR removes 'webSearch' from GatewayTargetTypeSchema and changes createWebSearchGatewayTarget() to persist targetType: 'connector', connectorId: 'web-search', but two existing test suites still assert the old shape and are not touched by this PR:
src/schema/schemas/__tests__/mcp.test.ts— the entiredescribe('AgentCoreGatewayTargetSchema with webSearch', …)block (11+ cases) parsestargetType: 'webSearch'. SincewebSearchis no longer in the enum, the "accepts a minimal webSearch target" and "accepts … with excludeDomains" cases will fail at the enum stage, and the various "rejects …" cases will now fail for the wrong reason (enum rejection rather than the targeted per-field checks).src/cli/primitives/__tests__/GatewayTargetPrimitive.test.ts— thedescribe('GatewayTargetPrimitive — createWebSearchGatewayTarget', …)block still assertsexpect(target?.targetType).toBe('webSearch'), but the primitive now writes'connector'.
These need to be updated (or deleted and replaced) to match the new connector + connectorId: 'web-search' shape. The PR description says test:unit passed, so I'd double-check that on a clean checkout — this looks like it would fail.
Blocking: userPassedAnyFlag for add gateway-target is under-specified
See the inline comment on GatewayTargetPrimitive.ts.
Non-blocking observations
- The web-search connector still requires
--name(thenameOptionalshortcut invalidate.ts:397-398only covers the KB connectors). If the intent per the PR description was that connector attaches get a default name,web-searchis inconsistent. If the intent was KB-only, ignore this — but worth stating in the description. --connector web-search --knowledge-base-id …is silently accepted rather than rejected. Not blocking, but nicer to reject with "not applicable for --connector web-search".
| !!cliOptions.type || | ||
| !!cliOptions.connector || | ||
| !!cliOptions.endpoint || | ||
| !!cliOptions.json; |
There was a problem hiding this comment.
This interactive-fallback check is under-specified for this command. add gateway-target exposes many other non-interactive flags (--gateway, --knowledge-base-id, --exclude-domains, --lambda-arn, --rest-api-id, --stage, --schema, --host, --runtime, --runtime-endpoint, --passthrough-endpoint, --outbound-auth, --credential-name, --oauth-*, --tool-*, etc.) that aren't in the || chain. A user running e.g.
agentcore add gateway-target --gateway my-gw --lambda-arn arn:aws:lambda:…
will be silently dropped into the TUI wizard (ignoring the flags they passed) instead of getting the previous behavior (validation error / non-interactive execution). That's a subtle regression that will bite scripts and CI.
Options:
- Expand the
||chain to enumerate every user-facing flag (compare withKnowledgeBasePrimitivewhich does this for its smaller flag set). - Compute it off
Object.keys(rawOptions)filtered against Commander defaults (e.g. subtractknowledgeBaseId: []when it's still the empty default). - Simply gate on "no flags at all were passed" — i.e.
Object.keys(rawOptions).length === 0.
There was a problem hiding this comment.
expanded || chain to avoid unintentional drop into TUI
| if (!cliOptions.name) { | ||
| throw new ValidationError('A --name is required for `agentcore add knowledge-base`.'); | ||
| } | ||
| const resolvedName = cliOptions.name ?? `kb-quick-start-${Math.random().toString(36).slice(2, 7)}`; |
There was a problem hiding this comment.
Two small concerns with Math.random().toString(36).slice(2, 7) as a default-name generator:
- It's non-deterministic — the same command produces a different KB name each time, which makes scripting / retries awkward (a partial-failure retry creates a second KB instead of appending to the first).
- If a user runs
agentcore add knowledge-basetwice without--name, they'll silently get two distinct KBs (kb-quick-start-abc12,kb-quick-start-xyz34) rather than either an error or an append, which is probably not what they intended.
Consider either a deterministic default (e.g. kb-quick-start + collision check → fail with a helpful message) or a monotonic suffix based on existing KB names. If the random suffix is intentional to mirror console behavior, ignore — but at minimum please add a short comment explaining the choice.
There was a problem hiding this comment.
The randomness here is intentional. Follows console convention.
for 1) -> Producing a new KB in this case would be the expectation. If the first attempt succeeds, name is printed in the output and user can proceed. If attempt fails -- nothing is persisted from first attempt, so running again and getting a fresh resource is warranted.
for 2) -> This is the correct behavior. agentcore add knowledge-base is creating a new resource per run. If a user creates a KB, and wants to append on the next command, they can pas sin the name output from first run. Default behavior creating two disjointed KBs is the expectation.
|
|
||
| if (!options.name) { | ||
| const kbConnectors = ['bedrock-knowledge-bases', 'bedrock-agentic-retrieve']; | ||
| const nameOptional = options.type === 'connector' && kbConnectors.includes(options.connector ?? ''); |
There was a problem hiding this comment.
nameOptional only covers the two KB connectors, so --connector web-search without --name still hits the --name is required branch even though the primitive at GatewayTargetPrimitive.ts:665-669 has 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-search or web-search-${gateway}) or clarify the description.
There was a problem hiding this comment.
This is intentional, I've updated PR description to clarify
| valid: false, | ||
| error: `--knowledge-base-id is required for --connector ${options.connector}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This skips the --knowledge-base-id is required check for web-search (correct), but nothing rejects --knowledge-base-id being supplied when --connector web-search. Suggest adding an explicit rejection so --connector web-search --knowledge-base-id foo fails with a clear message rather than silently ignoring the flag.
There was a problem hiding this comment.
Added explicit check & rejection
github-actions
Bot
removed
the
agentcore-harness-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
Coverage Report
|
tejaskash
left a comment
tejaskash
left a comment
There was a problem hiding this comment.
Automated correctness + cleanup review (high effort). 10 findings; the first three are potential merge blockers because they cross the CLI↔L3-CDK boundary or reverse a deliberate gating decision. Inline comments below. One additional finding could not be anchored inline because its file is not in this diff:
e2e-tests/httpgateway-all-targets.test.ts:290— the assertionexpect(types.has('webSearch')).toBe(true)still expects awebSearchtarget, but the CLI now writes web-search astargetType: 'connector'(connectorId: 'web-search'). This test will fail; it wasn't updated to the new shape.
| name: config.name, | ||
| targetType: 'webSearch', | ||
| targetType: 'connector', | ||
| connectorId: 'web-search', |
There was a problem hiding this comment.
[Blocker — cross-repo contract break] This now writes web-search targets as targetType: 'connector' + connectorId: 'web-search', but the synth/deploy engine lives in the separate @aws/agentcore-cdk L3 construct (pinned ^0.1.0-alpha.19 in src/assets/cdk/package.json), which has no handler for this shape:
- Its
ConnectorIdSchemaenum only allowsbedrock-knowledge-bases/bedrock-agentic-retrieve, so the project fails schema validation at deploy. - Even past that,
Gateway.tsroutes everytargetType === 'connector'into a branch that handles only the two KB connectors and ends inthrow new Error('Unknown connectorId "web-search"'). - The dedicated
targetType === 'webSearch'synth branch (InvokeWebSearch grant +domainFilter) is now dead for CLI-authored targets.
Net: agentcore add gateway-target --connector web-search succeeds but the next agentcore deploy fails. The L3 construct must be updated in lockstep (route connectorId: 'web-search' on the connector path) and released before this ships.
| 'passthrough', | ||
| 'webSearch', | ||
| ]); | ||
| export type GatewayTargetType = z.infer<typeof GatewayTargetTypeSchema>; |
There was a problem hiding this comment.
[Blocker — backward-compat break] Dropping 'webSearch' from GatewayTargetTypeSchema with no migration means any existing agentcore.json that already contains a targetType: 'webSearch' target (written by a prior/preview CLI) now fails AgentCoreProjectSpecSchema.parse in readProjectSpec. Every command that loads the project — status, deploy, add, remove, export — throws ConfigValidationError with no upgrade path. Consider keeping 'webSearch' accepted on read (normalize to the connector shape on load) even if the CLI no longer writes it.
There was a problem hiding this comment.
Web-search was behind ENABLE_GATED_FEATURES and never released. Zero customer impact
| .option( | ||
| '--connector <id>', | ||
| 'Connector id (for connector type): bedrock-knowledge-bases or bedrock-agentic-retrieve [non-interactive]' | ||
| 'Connector id (for connector type): bedrock-knowledge-bases, bedrock-agentic-retrieve, or web-search [non-interactive]' |
There was a problem hiding this comment.
[Blocker — confirm intent] This PR removes all ENABLE_GATED_FEATURES gating on web-search (the isGatedFeaturesEnabled import, the 'not yet available' throws, the hidden: !isGatedFeaturesEnabled() shortcuts, and the TUI 'Coming soon'/disabled state). Commit #1625 (chore: re-gate web search) deliberately re-gated this. Meanwhile KnowledgeBasePrimitive still gates its add command. Given finding #1 (web-search can't currently deploy), ungating it by default looks like an unintended leak rather than a coordinated launch. Please confirm this is intentional.
There was a problem hiding this comment.
Intentional. Ungated web-search experience is shipping with this release
| @@ -800,10 +799,10 @@ export const AgentCoreGatewayTargetSchema = z | |||
| }); | |||
There was a problem hiding this comment.
[Correctness — lost validation] The deleted targetType === 'webSearch' block used to reject knowledgeBaseId, knowledgeBaseIds, httpRuntime, and passthrough on a web-search target. In the new connector branch, the KB-field rejections live only inside the connectorId === BEDROCK_KNOWLEDGE_BASES / BEDROCK_AGENTIC_RETRIEVE guards, and httpRuntime/passthrough aren't rejected at all — so { targetType: 'connector', connectorId: 'web-search', knowledgeBaseId: 'ABCDE12345' } (or a passthrough/httpRuntime block) now passes validation silently and is carried into synth. Add an explicit connectorId === 'web-search' guard that rejects knowledgeBaseId/knowledgeBaseIds (and ideally the sub-configs).
There was a problem hiding this comment.
added explicit rejections for passthrough and httpRuntime
| const resolvedName = | ||
| cliOptions.name ?? | ||
| (connectorId === 'bedrock-knowledge-bases' | ||
| ? `kb-${cliOptions.gateway}-${kbRefs[0]}` |
There was a problem hiding this comment.
[Correctness — invalid auto-name reaches deploy] kb-${cliOptions.gateway}-${kbRefs[0]} embeds the raw KB reference. KB names permit underscores (/^[a-zA-Z][a-zA-Z0-9_-]{0,47}$/), but gateway tool names forbid them (ToolNameSchema = /^[a-zA-Z][a-zA-Z0-9-]*$/). The target is written with only z.string().min(1) validation, so --knowledge-base-id my_kb yields kb-gw-my_kb, persists locally, then is rejected server-side at agentcore deploy — a confusing failure long after add succeeded. Long gateway+kb names can also exceed the 128-char limit. Validate the derived name against ToolNameSchema (and sanitize/truncate) before writing. This is newly introduced — the base code required an explicit --name.
There was a problem hiding this comment.
Added logic to sanitize & truncate name before writing
| process.exit(1); | ||
| } | ||
|
|
||
| const userPassedAnyFlag = |
There was a problem hiding this comment.
[Correctness — incomplete flag detection] This allowlist omits several real non-interactive flags: --tool-schema-file, --credential-name, --stage, --oauth-client-id/secret/discovery-url/scopes, --tool-filter-path/methods, --schema-s3-account, --description. Invoking with only one of those sets userPassedAnyFlag = false, so in a TTY the command silently drops into the interactive AddFlow (discarding the flags), and in CI requireTTY() throws — instead of validating and erroring on the missing --type. Consider deriving "any flag passed" from the presence of any option key rather than an explicit hand-maintained OR-chain.
There was a problem hiding this comment.
Replaced OR-chain with a generic check as recommended
| if (!cliOptions.name) { | ||
| throw new ValidationError('A --name is required for `agentcore add knowledge-base`.'); | ||
| } | ||
| const resolvedName = cliOptions.name ?? `kb-quick-start-${Math.random().toString(36).slice(2, 7)}`; |
There was a problem hiding this comment.
[Correctness — silent data merge on collision] The auto-generated name has no uniqueness check. On a collision with an existing KB, add() finds that KB and routes to appendToExisting(), silently merging the new data sources into an unrelated KB instead of creating the intended new one — with no error surfaced. generateUniqueName(base, existingNames) already exists in tui/utils and the project's KB names are available here; use it to guarantee uniqueness.
There was a problem hiding this comment.
updated logic to validate no collision with existing name before proceeding
| error: '--knowledge-base-id is not applicable for --connector web-search', | ||
| }; | ||
| } | ||
| if (options.connector !== 'web-search' && (!options.knowledgeBaseId || options.knowledgeBaseId.length === 0)) { |
There was a problem hiding this comment.
[Correctness — lost early error] The connector irrelevant list (just below) dropped ['schema','--schema'] and ['schemaS3Account','--schema-s3-account'], both of which were in the deleted WEB_SEARCH_DISALLOWED_OPTIONS. So --connector web-search --schema s3://... no longer gets the clean early CLI error '--schema is not applicable', and a bare --schema-s3-account (no --schema) slips past the CLI entirely. Lower severity since a materialized schemaSource may still trip the schema layer, but the early feedback is gone.
There was a problem hiding this comment.
I've added --schema/--schema-s3-account to the connector irrelevant list
|
|
||
| await this.writeProjectSpec(project); | ||
|
|
||
| if (!options.json) for (const w of warnings) console.warn(w); |
There was a problem hiding this comment.
[Cleanup — dead code + defunct field] Removing the --gateway wiring here leaves appendConnectorTargets and its only callee upsertRetrieveTarget (~lines 238–278) with no callers (git grep appendConnectorTargets returns only the definition), and the kb.gateway schema field (primitives/knowledge-base.ts) is now write-dead — only read in remove/preview branches that are unreachable for CLI-created projects. noUnusedLocals is false, so nothing flags this. Also: an existing project whose KB already has gateway set no longer gets its connector target re-ensured on a subsequent add (that if (existing.gateway) appendConnectorTargets(...) block was deleted). Delete the dead cluster + field, or confirm the legacy remove-cascade still matters.
There was a problem hiding this comment.
valid. deleted dead code in latest commit
nborges-aws
force-pushed
the
connectors-updates
branch
from
7f61b21 to
813f6e0
Compare
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
nborges-aws
force-pushed
the
connectors-updates
branch
from
813f6e0 to
64a5e96
Compare
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
jariy17
left a comment
jariy17
left a comment
There was a problem hiding this comment.
LGTM just small nits:
▎ few low-pri cleanups from the #1681 review, none are blockers:
▎ - parameterValues/parameterOverrides are optional in the zod schema (mcp.ts:473) but
▎ required in the ConfigurationEntry type — should reconcile those
▎ - no tests for the kb-{gateway}-{kb-id} auto-name path or the bare-command TUI fallback
▎ detection
▎ - the reverse-index KB-walk in status/action.ts is duplicated (~L265 and ~L563), might be
▎ worth extracting
| options.type = mappedType; | ||
|
|
||
| // --exclude-domains is webSearch-target-only. Reject it on every other target type. | ||
| if (mappedType !== 'webSearch' && options.excludeDomains) { |
There was a problem hiding this comment.
nit: We should consider a cleaner way to do flag validation in our rf.
| try { | ||
| requireTTY(); | ||
| const [{ render }, { default: React }, { AddFlow }] = await Promise.all([ | ||
| import('ink'), |
There was a problem hiding this comment.
nit: why are you import within a function?
4 participants
Description
Update CLI connector DevEx for web-search and knowledge base. This PR restructures how connector targets are stored and managed.
Web-search moves from standalone targetType to connector
agentcore add web-search/agentcore remove web-searchtop-level shortcuts--typevalid values; added to--connectoragentcore add gateway-target --type connector --connector web-searchconfigurations[] array replaces flat typed fields
{ name, parameterValues, parameterOverrides }--exclude-domains,--knowledge-base-id) translate to the correct parameterValues shape via a connector translator modulebedrock-agentic-retrieve removed as standalone connector
--knowledge-base-idflag produces both Retrieve and AgenticRetrieveStream configurationsKB --gateway flag removed
agentcore add gateway-target --connector bedrock-knowledge-basesappendConnectorTargetsdead code--name optional for KB paths
agentcore add gateway-target --connector bedrock-knowledge-bases:defaults to kb-{gateway}-{kb-id}Related Issue
Closes #
Documentation PR
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.