feat(registry): agent asset catalog on AgentCore — provisioning, port/adapter, API, CLI (#246) - #664
feat(registry): agent asset catalog on AgentCore — provisioning, port/adapter, API, CLI (#246)#664Kalindi-Dev wants to merge 2 commits into
Conversation
…/adapter, API, CLI (#246) Introduces the read-side catalog for the central agent asset registry built on AWS Agent Registry (Bedrock AgentCore), with nothing upstream importing the AWS SDK directly: - Provisioning: `AgentRegistryStack` (NestedStack) creates the registry via a custom resource (async CreateRegistry, no L2 in preview); bootstrap IAM + resource-action-map updated (states, cognito group, cloudformation nested stack) with the golden DEPLOYMENT_ROLES.md kept in sync. - Ports & adapters: `RegistryClient` port (TS + Py) with a single `AgentCoreRegistryClient` adapter per language. Native descriptor storage — MCP server.json + `_meta`, AGENT_SKILLS markdown frontmatter, CUSTOM verbatim. - Grammar: `registry://kind/namespace/name@constraint` with mandatory semver pin, mirrored byte-for-byte across ref.ts / ref.py and enforced by the `contracts/registry-resolution/` parity corpus. - API: publish / resolve / list / show routes on TaskApi, gated by two Cognito groups (RegistryPublisher / RegistryApprover); `bgagent registry` CLI. - Wire types (shared/types.ts + cli/types.ts): resolved-asset triple stamped on TaskRecord/TaskDetail/TaskSummary for audit. Integration (orchestrator resolve-step, agent loaders, blueprint asset pins) lands in the follow-up PR that builds on this catalog.
#246) Review follow-ups on the catalog PR: - Python ref/semver regexes anchored with \Z instead of $. Python's $ also matches just before a trailing newline, so `registry://…@1.0.0\n` parsed in Python but was rejected by the JS mirror (ref.ts, no m flag) — a byte-for-byte parity break the grammar explicitly promises not to have. Added a `trailing-newline-rejected` case to the shared resolution corpus so CI catches any future regression on either side. - Bumped BOOTSTRAP_VERSION 1.2.0 → 1.3.0 (policy surface changed this PR: Cognito group, Step Functions, CloudFormation nested-stack ARN) and regenerated the bootstrap template so operators know their role is stale.
…-check (aws-samples#246) The ADR cited cdk/src/handlers/shared/registry/ref.ts and agent/src/registry/ref.py as relative-path links, but those files ship in the implementation PRs (aws-samples#664/aws-samples#665), not on this ADR branch or main — so //docs:link-check failed with 2 dead links. Demoted both to inline code spans (with a note that they land with aws-samples#664/aws-samples#665) until the implementation merges. Mirror regenerated via docs sync.
Self-review (principal-architect pass)Ran a Fixed in this branch (commit
Open nits (non-blocking, fast-follow): no-op try/catch in Verified clean: bootstrap synth-coverage (91/91) + golden-baseline, CDK↔CLI types sync, IAM least-privilege + fail-closed auth (401/403/422), docs mirror idempotent. All CI checks green. |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict: Approve (with minor nits)
A large, careful, well-scoped read-side registry catalog on AgentCore. I verified the four critical review foci empirically (tests run locally, not just read): bootstrap policy coverage is complete and the golden baseline + synth-coverage pass; CDK↔CLI shared types are in sync; the registry:// grammar and resolution semantics match byte-for-byte across TS/Py; auth is Cognito-group-gated and resolution fails closed; and IAM is least-privilege on both the custom-resource role and the handler roles. No blocking issues. The open items are the same nits the author already flagged in the self-review.
Vision alignment
Advances #246 / ADR-018 without eroding tenets. Fire-and-forget is untouched (this is a catalog + API/CLI surface; no live pairing). Blast radius is bounded: publish/approve are gated by RegistryPublisher/RegistryApprover groups, resolution is fail-closed (422 with a specific reason on any unresolved/invalid ref), and IAM is scoped to registry/* + registry/*/record/*. Outcomes stay reviewable — the resolved-asset triple is stamped on TaskRecord/Detail/Summary for audit. The L2-vs-custom-resource trade is documented inline (no AgentCore L2 in preview) with a GA-throwaway note and the RegistryClient seam confining the future swap.
Blocking issues
None.
Non-blocking suggestions / nits
- No-op try/catch in
cdk/src/handlers/shared/registry/agentcore-client.ts:180-183— both catch branches re-throw (if (err instanceof ConflictException) throw err; throw err;), so the whole try/catch is dead. Drop it. (Author already noted this.) - Double
parseConstraintcall incdk/src/handlers/registry-publish.ts:124—!parseConstraint(...) || parseConstraint(...)!.op !== 'exact'parses twice; compute once into a local. Cosmetic. (Author noted.) - O(n) resolve/list —
AgentCoreRegistryClient.listRecordsdoes List + N×Get per call, andresolve/show/getRecordall funnel through it. Acceptable for the MVP given the GA-throwaway construct, but worth a// TODO(GA)marker so it isn't forgotten when the native construct lands. showresponse is an inline anonymous type on both server (registry-show.ts:68) and client (api-client.ts::registryShow) rather than a namedRegistryShowResponse. The two inline shapes match today, but a named type would put it under the types-sync guard like the other envelopes. Coherence nit.- SKILL.md runtime round-trip via a single-quoted frontmatter line (
agentcore-client.ts::parseSkillRuntime↔agentcore_client.py::_SKILL_RUNTIME_RE): a runtime JSON payload containing a'or newline could mis-parse. Shared limitation on both sides (not a parity break), but consider base64-encoding thex-abca-runtimevalue to make it robust.
Documentation
Good. docs/design/REGISTRY.md (new) documents the grammar, storage modes, and group gating; docs/design/DEPLOYMENT_ROLES.md golden baseline updated to match the new bootstrap statements; contracts/registry-resolution/README.md documents the parity corpus. I regenerated the Starlight mirror locally (node docs/scripts/sync-starlight.mjs) and git status docs/src was clean — no drift. bgagent registry is self-documenting via commander help.
Tests & CI
- CDK: ran
registry-resolution-parity,registry-handlers,agentcore-client,registry-resolver,constructs/registry,bootstrap/synth-coverage,bootstrap/golden-baseline,bootstrap/version,bootstrap/policies— 111 passed. Bootstrap synth-coverage explicitly PASSES with the new CFN types (AWS::CloudFormation::Stack,AWS::Cognito::UserPoolGroup,AWS::StepFunctions::StateMachine,Custom::AgentCoreRegistry) all mapped inresource-action-map.tsand covered by the policy bundle.BOOTSTRAP_VERSIONbumped 1.2.0→1.3.0, hash + snapshot regenerated. - Agent:
test_registry_resolution_corpus+test_registry_agentcore_client— 24 passed; full workflow/validator suite — 182 passed (the widened_REGISTRY_REFregex is backward-compatible with the legacy 2-segment corpus). - Types-sync:
scripts/check-types-sync.ts— OK, 68 CLI exports validated. - Failure paths covered: 401 (no auth), 403 (missing group / auto_approve without approver), 422 (bad ref / no matching version), 404 (unknown asset), 409 (version collision). Tests exercise these, not just the happy path.
- GitHub CI: all checks green (build agentcore, CodeQL, secrets/deps scan).
- Test perf: the registry construct test does not re-enable bundling or synth-per-test; parity/resolver tests are pure-function.
Review agents run
The pr-review-toolkit sub-agents and /security-review are not separately dispatchable in this execution context, so I performed the equivalent analysis by hand and state so here:
- code-reviewer (by hand): routing correct per AGENTS.md (agent runtime in
agent/, API/Lambdas + bootstrap incdk/, CLI incli/); L2 preferred where available, custom resource justified for the preview-only AgentCore control plane. - silent-failure-hunter (by hand): error handling surfaces failures — resolve/publish fail closed with specific codes; the custom-resource
onEvent/isCompletehandle ConflictException/ResourceNotFoundException deliberately and re-throw the rest;FAILEDregistry status throws. One dead no-op try/catch (nit #1).drainRecordsbest-effort with isComplete retry is sound. - type-design-analyzer (by hand): new wire types mirrored cdk↔cli and guarded by the sync check; domain types (
RegistryRecord,ResolvedAsset, port) intentionally excluded from the CLI contract.ParseResultdiscriminated union is clean. - comment-analyzer (by hand): comments accurate; the
\Z-vs-$parity rationale inref.pyis correct and load-bearing. - pr-test-analyzer (by hand): happy + failure paths covered on both languages; parity corpus enforces cross-language agreement including the
trailing-newline-rejectedregression case. - /security-review scope (by hand): IAM least-privilege verified (custom-resource role uses
*only for account-level Create/List/WorkloadIdentity with documented rationale + nag suppression; handler roles scoped toregistry/*/registry/*/record/*; publish=read+write, resolve/list/show=read-only); Cognito group gating fail-closed; no secrets; no Cedar engine-pin movement in this PR (cedarpy/cedar-wasm untouched — confirmed).
Human heuristics
- Proportionality — Pass. The port/adapter split is justified by the documented GA substrate swap; not an over-abstraction. File sizes are essential, not accreted.
- Coherence — Pass (minor). Same concepts use the same terms across TS/Py; parity corpus enforces real substance, not copy-paste. Only nit: the
showresponse is an inline type on both sides rather than a named shared envelope (#4). - Clarity — Pass. Names communicate intent; errors surface rather than hide; magic values (poll intervals, group names) are named constants.
- Appropriateness — Pass. Adapter decisions (name-in-
nameOption A, native-vs-CUSTOM, 3-call publish) are grounded in the live AgentCore spike findings, not self-written mocks; tests assert intended behavior (fail-closed resolution, immutability) not just current behavior.
Governance: #246 is approved P0 (backing-issue gate satisfied); feat/246-registry-catalog is the sanctioned branch name.
| recordId = this.idFromArn(res.recordArn!); | ||
| } catch (err) { | ||
| if (err instanceof ConflictException) throw err; | ||
| throw err; |
There was a problem hiding this comment.
Nit: this try/catch is a no-op — both branches re-throw (if (err instanceof ConflictException) throw err; then throw err;). Drop the wrapper and let the CreateRegistryRecordCommand call throw directly.
| if (!body.name || !NAME_RE.test(body.name)) { | ||
| return 'name must match [a-z0-9][a-z0-9._-]*.'; | ||
| } | ||
| if (!body.asset_version || !parseConstraint(body.asset_version) || parseConstraint(body.asset_version)!.op !== 'exact') { |
There was a problem hiding this comment.
Nit: parseConstraint(body.asset_version) is evaluated twice here. Compute once into a local const c = parseConstraint(body.asset_version) and test !c || c.op !== 'exact'.
| return records.find((r) => r.name === name && r.version === version) ?? null; | ||
| } | ||
|
|
||
| async listRecords(filter?: ListFilter): Promise<readonly RegistryRecord[]> { |
There was a problem hiding this comment.
MVP-acceptable, but listRecords does List + N×Get and every read path (resolve/show/getRecord) funnels through it — O(n) per call. Worth a // TODO(GA) so this isn't carried past the native-construct swap.
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict: Request changes
I re-reviewed head d3e2b056 independently, including the areas already approved. The focused registry baseline is green (112/112 CDK assertions and 39/39 Python tests), but executable probes expose untested contract, security, and CloudFormation failures. The blocking cases and concrete reproductions are inline below; these are separate from the existing no-op catch, double-parse, and O(n) nits.
| // CreateRegistryRecord is async — wait until it leaves CREATING. | ||
| await this.waitPastCreating(recordId); | ||
|
|
||
| if (input.autoApprove) { |
There was a problem hiding this comment.
[P1] Submit normal publishes for approval. SubmitRegistryRecordForApprovalCommand is inside this autoApprove branch, so autoApprove: false executes only List/Create/Get/Get and returns DRAFT (reproduced with the adapter). REGISTRY.md section 6/9 and ADR-018 say a normal publisher creates a PENDING_APPROVAL record, and there is no standalone submit endpoint in this PR. As written, normal publishers create records that no ABCA surface can resolve or promote. Submit every successful create; gate only the final UpdateRegistryRecordStatus(APPROVED) call on autoApprove.
| if (!body.discovery || typeof body.discovery !== 'object') { | ||
| return 'discovery must be an object.'; | ||
| } | ||
| if (!body.runtime || typeof body.runtime !== 'object') { |
There was a problem hiding this comment.
[P1] Validate the per-kind runtime contract instead of only typeof object. Arrays satisfy this check, and direct handler probes returned 201 for MCP runtime: {}, runtime: [], {prompt_fragment: "x"}, custom: "false", and CUSTOM array discovery/runtime. Those records later resolve into loaders that skip them while the task audit still says the pin was used. Reject arrays, require booleans for the flags, and enforce each discriminated payload (transport plus URL/command invariants, cedar_text, or prompt_fragment) before publish.
| namespace: asset.namespace, | ||
| name: asset.name, | ||
| version: asset.version, | ||
| runtime: asset.runtime as unknown as Record<string, unknown>, |
There was a problem hiding this comment.
[P1 security] Do not return credential-bearing runtime data to every authenticated caller. MCP runtime explicitly allows arbitrary headers; a direct resolve-handler probe as a user with no registry groups returned 200 with Authorization: Bearer registry-secret, and the CLI prints this payload verbatim. That turns the catalog into a tenant-wide secret read endpoint. Store secret references/bindings rather than values and either redact this human-facing response or keep unredacted resolution on an orchestrator-only IAM path.
| `name: ${skillNameSlug(input.namespace, input.name)}`, | ||
| `description: ${description}`, | ||
| `version: ${input.version}`, | ||
| `${SKILL_RUNTIME_FM_KEY}: '${runtimeJson}'`, |
There was a problem hiding this comment.
[P1] Encode skill runtime as valid YAML. With prompt_fragment: "Don't skip tests", this emits x-abca-runtime: '{"prompt_fragment":"Don't skip tests"}'; js-yaml rejects the generated SKILL.md with bad indentation of a mapping entry. Ordinary skill text therefore passes this API but can fail native AgentCore descriptor validation. Use a YAML serializer or a safe encoding such as base64, and parse real frontmatter in both implementations.
| case 'Create': { | ||
| const { RegistryName, Description } = event.ResourceProperties; | ||
| const res = await client.send( | ||
| new CreateRegistryCommand({ name: RegistryName, description: Description }), |
There was a problem hiding this comment.
[P1] Make custom-resource create replay-safe. The AgentCore CreateRegistryRequest provides clientToken, but this request omits it and the event model omits CloudFormation RequestId. Provider handlers are delivered at least once; if the service creates the uniquely named registry but the response is lost, a retry sends a distinct create and can strand stack creation/rollback. Derive a stable token from the CloudFormation request identity and test duplicate delivery.
| // The registry name is immutable in this design; a name change would force | ||
| // replacement (new PhysicalResourceId) via CreateRegistry on the new value. | ||
| // Nothing to mutate in place, so echo the existing id back. | ||
| return { PhysicalResourceId: event.PhysicalResourceId }; |
There was a problem hiding this comment.
[P1] Do not report successful updates while ignoring desired state. Changing either exposed prop (registryName or description) invokes Update, but this branch sends no SDK command and returns the same physical ID. CloudFormation then reports success while the managed registry keeps the old values. The installed SDK supports UpdateRegistryCommand for both fields; otherwise a name change must explicitly return a replacement physical ID. The fix also needs UpdateRegistry IAM/bootstrap coverage.
| return errorResponse(403, ErrorCode.FORBIDDEN, `auto_approve requires the ${REGISTRY_APPROVER_GROUP} group.`, requestId); | ||
| } | ||
|
|
||
| const input: PublishInput = { |
There was a problem hiding this comment.
[P1] Persist the authenticated publisher identity. userId is extracted above but dropped from PublishInput; the adapter never writes or reads a publisher, so registry-show.ts emits publisher: null for every record created here despite the documented audit schema. CloudTrail sees the shared Lambda role, not the Cognito principal, so this cannot be reconstructed later. Stamp immutable ABCA metadata (or a durable audit record) and round-trip it for native and CUSTOM descriptors.
| if (!m) return null; | ||
| return { | ||
| op: OP_BY_PREFIX[m[1]], | ||
| major: Number(m[2]), |
There was a problem hiding this comment.
[P2] Keep numeric semver components exact across TypeScript and Python. The grammar accepts unbounded digits, but Number(...) rounds values above MAX_SAFE_INTEGER. For exact pin 9007199254740993.0.0 and sole candidate 9007199254740992.0.0, TypeScript rounds the constraint and selects that candidate; Python preserves the integer and returns no match. Reject out-of-range components in both languages or compare strings/bigint, then add this case to the parity corpus.
| const rec = await this.client.send( | ||
| new GetRegistryRecordCommand({ registryId: this.registryId, recordId }), | ||
| ); | ||
| if (!String(rec.status).includes('CREATING')) return; |
There was a problem hiding this comment.
[P1] Treat asynchronous create failure and timeout as publish failure. This returns on every status that is not textually CREATING, including CREATE_FAILED, and falling out of the loop after 30 polls is also treated as success. An adapter probe returning CREATE_FAILED here caused publish() to return that record, so the handler would send HTTP 201 for failed substrate validation. Continue through transient not-found, require the expected usable state (DRAFT), throw with statusReason on *_FAILED, and throw when the poll budget expires.
| const registryArn = Stack.of(this).formatArn({ | ||
| service: 'bedrock-agentcore', | ||
| resource: 'registry', | ||
| resourceName: '*', |
There was a problem hiding this comment.
[P1 security] Scope these roles to the registry wired into this construct. Both resource ARNs use *, so publish/resolve/list/show can operate on every AgentCore registry in the account, even though props.agentRegistryId is already available. This is unlike create-time provisioning, where the ID is genuinely unknown. Format registry/{agentRegistryId} and registry/{agentRegistryId}/record/*; otherwise compromise of any read handler can also retrieve runtime payloads from unrelated registries.
3 participants
Summary
Read-side catalog for the central agent asset registry (#246, ADR-018 #548) built on AWS Agent Registry (Bedrock AgentCore) — the chosen substrate. Nothing upstream imports the AWS SDK directly; the AgentCore control plane sits behind a
RegistryClientport.AgentRegistryStack(NestedStack) creates the registry via a custom resource (async CreateRegistry, no L2 in preview); bootstrap IAM + resource-action-map + goldenDEPLOYMENT_ROLES.mdin sync. Nested to keep the root stack under CloudFormation's 500-resource limit.RegistryClientport (TS + Py), oneAgentCoreRegistryClientadapter per language. Native descriptors — MCPserver.json+_meta, AGENT_SKILLS markdown frontmatter, CUSTOM verbatim.registry://kind/namespace/name@constraintwith mandatory semver pin, mirrored byte-for-byte acrossref.ts/ref.pyand enforced by thecontracts/registry-resolution/parity corpus.bgagent registryCLI.Integration (orchestrator resolve-step, agent loaders, blueprint pins) lands in the follow-up PR that builds on this catalog.
Test plan
mise run buildgreen (2583 CDK tests + agent quality)