feat(registry): resolve + load registry assets into tasks (#246) - #665
feat(registry): resolve + load registry assets into tasks (#246)#665Kalindi-Dev wants to merge 1 commit into
Conversation
…-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.
Builds on the catalog PR to actually consume registry assets at task time:
- Orchestrator resolve-step (`resolveRegistryAssets`): resolves a blueprint's
`registry://` mcp_server / cedar_policy_module / skill refs at task start,
fail-closed; stamps the `{kind,id,version}` triples on the TaskRecord for
audit, merges resolved cedar_text into `cedar_policies`, and threads the
runtime bundle into the agent payload.
- Blueprint asset props + onUpdate fix: `assets.{mcpServers,cedarPolicyModules,
skills}` with `RegistryRefValidation`; the three onUpdate helpers now write
the asset-ref columns so redeploying an onboarded repo no longer drops them.
- Agent loaders (registry.loader): mcp_server merges into `.mcp.json`;
cedar_policy_module flows through PolicyEngine's unannotated `extra_policies`;
skill prompt fragments append to the system prompt (build_skill_prompt_fragment).
- TaskOrchestrator IAM: read-only bedrock-agentcore registry access so the
orchestrator can resolve refs.
Depends on the catalog PR (feat/246-registry-catalog): imports the RegistryClient
port, ref grammar, and resolver from that branch.
d3dde44 to
e05bfce
Compare
Self-review (principal-architect pass)Ran a Verified correct:
One finding worth stating explicitly (documented tradeoff, not a blocker): a resolved Rebased onto |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict
Approve with nits. Stacked on #664 (base feat/246-registry-catalog, present locally at d3e2b056). The three critical foci — fail-closed resolution, Cedar merge safety, and least-privilege registry IAM — all hold. The onUpdate column-drop fix is real and has a regression test. Nits below are non-blocking (two are pre-existing shared patterns). Merge order: #664 first, then retarget to main.
Vision alignment
Advances bounded blast radius and reviewable outcomes (VISION.md): registry pins are semver-locked, resolved fail-closed at admission, and stamped as an immutable {kind,id,version} audit triple on the TaskRecord. Cedar modules ride the same cedar_policies payload as inline blueprint policies, so the PolicyEngine's soft-tier force-wrap invariant is preserved by construction — a registry module cannot widen authorization. The documented tradeoff (registry MCP url is bounded only by the VPC DNS firewall, same posture as channel MCP) is a coherent tenet trade, not a regression; registry publish is correctly treated as a privileged, IAM-gated operation. No undocumented tenet trade.
Blocking issues
None.
Non-blocking suggestions / nits
- Agent-side MCP load fails open (fail-closed is resolution-only).
agent/src/registry/loader.py:189-195— on an.mcp.jsonwrite error,apply_mcp_assetslogs ERROR, returns 0, and the task proceeds without the operator-pinned MCP server. The fail-closed guarantee is entirely at the orchestrator resolve step (resolveRegistryAssets→failTask(HYDRATING)); once a bundle is threaded, a load failure is silent. cedar_policy_module still fails-closed (PolicyEngine raises at construction) and skill is pure prompt text, so onlymcp_serverdegrades silently — and it mirrors the existingconfigure_channel_mcpposture. Consider surfacing a task-visible warning (progress event) when a resolved MCP asset fails to load, so a pinned tool silently missing is observable. - Registry cedar_text bypasses the 64 KB blueprint cap.
agent/src/policy.py:884-889counts onlyblueprint_hard_policies + blueprint_soft_policies; registry cedar modules arrive via the legacyextra_policiespath (runner.py:288), which is uncapped. Pre-existing property of theextra_policieskwarg, not introduced here, but the registry now makes it operator-reachable at scale — worth a follow-up to foldextra_policiesbyte-length into the cap. - onUpdate does not REMOVE dropped asset columns.
cdk/src/constructs/blueprint.ts:363-365,400-402gate the write onlength > 0, so redeploying an onboarded repo that removed all its asset refs leaves the stalemcp_servers/cedar_policy_modules/skillscolumns in DDB. This exactly matches the siblingcedar_policies/egress_allowlistbehavior, so it is consistent — but the PR's own framing ("redeploy must not drop asset refs") is the mirror-image gap. Non-blocking; note it as a known limitation. forkBlueprintRepodemo hook is undocumented.cdk/src/stacks/agent.ts:176-192adds an opt-in context/env flag pinning hardcodedacme/*refs. It is clearly commented and opt-in, but a one-line mention in the deployment guide would help operators reproduce the E2E test plan.
Documentation
No docs changed in this PR, and none are strictly required: the resolve-step + loader behavior and the registry:// grammar were documented ahead of implementation in docs/design/Registry.md (mirror docs/src/content/docs/architecture/Registry.md, lines 176/189), which already describes "PR 2" resolve+load and the fail-closed pin semantics. Mirror is in sync (this PR touches no doc sources). Only gap is the undocumented forkBlueprintRepo demo flag (nit 4). Issue #246 is approved (P0) — governance satisfied.
Tests & CI
Strong coverage on both sides. Agent: agent/tests/test_registry_loader.py (15 tests) covers merge/preserve/multi-server, non-mcp skip, empty-runtime skip, missing repo_dir, malformed-existing-treated-as-absent, skill fragment assembly + tool_hints + ordering + blank skip. CDK: cdk/test/handlers/shared/registry-orchestrator.test.ts covers happy path, multi-ref order, fail-closed on malformed ref (resolve never called), fail-closed on NO_MATCHING_VERSION, DEPRECATED-warns-but-resolves, and all three kinds together; cdk/test/constructs/blueprint.test.ts adds onCreate mapping, omit-when-empty, the onUpdate regression guard, and synth-time rejection of floating/malformed refs. I ran both locally: agent 15/15 pass; CDK registry-orchestrator + blueprint 54/54 pass. All PR CI checks green (build agentcore, secrets/deps scan, dead-code advisory, PR title). No new CDK per-test synth or re-enabled bundling. No bootstrap update needed — the IAM change is a runtime data-plane grant on the orchestrator Lambda role, not a new CFN resource type (the registry construct + BOOTSTRAP_VERSION bump landed in #664, d3e2b056).
Review agents run
Specialized pr-review-toolkit subagents were not separately dispatchable in this execution context, so I performed the equivalent analysis by hand and state that explicitly:
- code-reviewer (by hand): routing correct (agent runtime in
agent/, orchestration/IAM incdk/); L2/IAM idioms clean; ArnFormat.SLASH_RESOURCE_NAME wildcards justified. - silent-failure-hunter (by hand): resolution path fail-closed and verified against the caller (
orchestrate-task.ts:154); found the one fail-open surface (nit 1, agent MCP write) and confirmed the other two kinds fail-closed. - type-design-analyzer (by hand):
ResolvedAssetTripleparity CDK↔CLI confirmed; agentresolved_assets: list[dict[str,Any]]is a pragmatic passthrough shape. - comment-analyzer (by hand): comments accurate; the "byte-identical from PolicyEngine's view" claim verified against
policy.pyextra_policies handling. - pr-test-analyzer (by hand): happy + failure paths both covered; ran the suites.
- security-review scope (by hand): IAM least-privilege + Cedar privilege-surface reviewed; no secrets/network regressions.
Human heuristics
- Proportionality — Pass. Per-kind loaders are small and single-purpose; no over-abstraction. Resolve-step reuses the existing cedar_policies threading rather than inventing a parallel channel.
- Coherence — Pass.
registry://grammar,ResolvedAsset, and the triple are spelled consistently across TS/Py/CLI; resolve-step mirrors howcedar_policiesalready flows (orchestrator.ts:748-758). - Clarity — Mostly pass; one concern:
loader.py:189-195fail-open on write hides a missing pinned tool behind an ERROR log (nit 1). - Appropriateness — Pass. Cedar merge verified against real PolicyEngine behavior (
policy.py:856-905), not a self-written mock; tests assert intended fail-closed semantics, not just current output.
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict: Request changes
I re-reviewed approved head e05bfce3 independently against ADR-018 and the composed #664 base. The focused suites remain green (112/112 CDK, 39/39 Python), but live in-process probes show that typed Blueprint refs, detach/update behavior, loader completion, MCP identity, Cedar limits, and deprecation audit are not enforced. These let the task record claim a pinned asset that was not actually applied, so they block the advertised reproducibility/fail-closed contract.
| this.node.addValidation(new RepoFormatValidation(props.repo)); | ||
| this.node.addValidation(new DomainFormatValidation(this.egressAllowlist)); | ||
| this.node.addValidation(new ApprovalGateCapValidation(this.approvalGateCap)); | ||
| this.node.addValidation(new RegistryRefValidation('assets.mcpServers', this.mcpServerRefs)); |
There was a problem hiding this comment.
[P1] Validate the expected kind for each typed Blueprint field. A synth probe accepted a skill under assets.mcpServers, an mcp_server under assets.cedarPolicyModules, and a cedar_policy_module under assets.skills, then stored each under that wrong DynamoDB column. The orchestrator later flattens the arrays and dispatches by the ref embedded kind, so a field typo silently activates a different class of runtime behavior. Pass an expected kind into each validator and reject mismatches at synth.
| if (this.approvalGateCap !== undefined) fields.push(', #approval_gate_cap = :approval_gate_cap'); | ||
| // Registry asset refs (#246) — must mirror onCreate's item, else a redeploy | ||
| // of an already-onboarded repo silently drops asset-ref changes. | ||
| if (this.mcpServerRefs.length > 0) fields.push(', #mcp_servers = :mcp_servers'); |
There was a problem hiding this comment.
[P1] Make empty arrays remove previously stored refs. These clauses only emit SET fields for non-empty arrays. A synth probe with all three arrays empty produced no asset attributes and no REMOVE clause, so a redeploy after deleting the last MCP/policy/skill leaves the old DynamoDB refs active. Operators therefore cannot detach a pinned asset through the declared Blueprint API. Generate REMOVE #mcp_servers, ... (as applicable) and cover populated-to-empty transitions.
| def _server_key(asset: dict[str, Any]) -> str: | ||
| namespace = asset.get("namespace", "") | ||
| name = asset.get("name", "") | ||
| return f"{namespace}__{name}".replace("-", "_") |
There was a problem hiding this comment.
[P1] Use an injective MCP server key. Both foo-bar and foo_bar are legal asset names, but replacing hyphens with underscores maps both to acme__foo_bar. The loader probe reported two writes while .mcp.json contained one key and only the second URL survived. That silently changes the tool surface relative to the resolved audit bundle. Preserve/escape the original components or reject a collision before writing.
| """ | ||
| if not resolved_assets: | ||
| return | ||
| apply_mcp_assets(repo_dir, resolved_assets) |
There was a problem hiding this comment.
[P1] Fail the task when a resolved asset cannot be applied. This wrapper discards the MCP loader result, and its pipeline caller ignores completion entirely. Probes show missing repo directories, empty runtime, and write errors all return 0; malformed skills collapse to an empty fragment. The orchestrator has already stamped {kind,id,version}, so execution proceeds with an audit record claiming assets that are absent. Validate the whole bundle and raise before agent execution (or return structured results that the pipeline must verify).
| // SAME cedar_policies payload field as inline blueprint policies, so they are | ||
| // byte-identical from the PolicyEngine's view (the cedar-parity contract holds | ||
| // by construction). Inline blueprint policies come first, then resolved modules. | ||
| const cedarText = [ |
There was a problem hiding this comment.
[P1] Include registry Cedar bytes in the enforced aggregate cap. These modules are appended to cedar_policies, but runner.py passes that list as PolicyEngine(extra_policies=...); the 64 KiB check in policy.py counts only blueprint_hard_policies + blueprint_soft_policies. A direct engine probe accepted a 65,574-byte registry-style policy with POLICIES_MAX_BYTES=65,536. Count all operator-supplied policy text before parsing so registry publication cannot bypass the safety bound.
| } | ||
| const asset = await client.resolve(parsed.ref); | ||
| if (asset.warnings.length > 0) { | ||
| log.warn('Registry asset resolved with warnings', { ref, warnings: asset.warnings }); |
There was a problem hiding this comment.
[P2] Persist deprecation warnings in the task audit surface. This only writes a Lambda log. The later resolved_assets stamp drops warnings, and the agent payload drops them too, while ADR-018 sub-decision 4 promises a warning event on the task record. Users inspecting the task cannot tell that a deprecated asset ran. Emit a durable TaskEvent and/or persist warnings with the resolved audit metadata.
| if not isinstance(runtime, dict) or not runtime: | ||
| log("WARN", f"apply_mcp_assets: skipping {_server_key(asset)} — empty runtime payload") | ||
| continue | ||
| servers[_server_key(asset)] = runtime |
There was a problem hiding this comment.
[P1] Write the MCP config schema the runtime actually consumes. The base PR declares transport: "http" | "sse" | "stdio", and the registry tests pass that shape here unchanged. However, both the existing channel_mcp.py entries and the installed Claude Agent SDK McpHttpServerConfig/McpSSEServerConfig require the discriminant key type (type: "http" or "sse"; stdio optionally uses type). A publisher following the documented registry contract therefore gets a .mcp.json entry the agent does not recognize. Align the shared type/docs with native McpServerConfig or explicitly validate and normalize transport -> type; update tests to assert a consumable config rather than byte-for-byte persistence.
3 participants
Summary
Builds on the catalog PR (#664) to consume registry assets at task time.
resolveRegistryAssets): resolves a blueprint'sregistry://mcp_server/cedar_policy_module/skill refs at task start, fail-closed; stamps{kind,id,version}triples on the TaskRecord, merges resolved cedar_text intocedar_policies, threads the runtime bundle into the agent payload.assets.{mcpServers,cedarPolicyModules,skills}withRegistryRefValidation; the three onUpdate helpers now write the asset-ref columns so redeploying an onboarded repo no longer drops them.registry.loader): mcp_server →.mcp.json; cedar_policy_module → PolicyEngine unannotatedextra_policies; skill prompt fragments → system prompt.Test plan
mise run buildgreen (2595 tests total)--context forkBlueprintRepo=owner/repo; submit a task that pins all three asset kinds01KYJ5FT0RDZJ72J7R5BKCDM38)