feat(registry): orchestrator + agent + MCP server E2E (#246)

[Kalindi-Dev]

Area

• cdk — infrastructure, handlers, constructs
• agent — Python runtime / Docker image
• cli — bgagent client

Related

• Progresses feat(registry): central agent asset registry for capabilities, skills, plugins, and MCP servers #246 (central agent asset registry).
• Decision record: ADR-018 (PR docs(decisions): ADR-018 propose central agent asset registry (#246) #548).
• Stacked series — merge in order: ADR docs(decisions): ADR-018 propose central agent asset registry (#246) #548 → PR 1 (feat(registry): design + storage + publish/resolve API (#246) #632) → PR 2 (this) → PR 3 (feat/246-registry-cedar-skills).
• Built on PR 1 (feat(registry): design + storage + publish/resolve API (#246) #632). Until feat(registry): design + storage + publish/resolve API (#246) #632 merges, this PR's diff-against-main also shows PR 1's commits — review the commit 59fe23d for PR-2-only changes.

Changes

Wires the registry (PR 1) end-to-end for one asset kind: a blueprint pins an MCP server, the orchestrator resolves it at the create-task boundary, stamps the resolved {kind,id,version} on the TaskRecord, and the agent merges the resolved server config into .mcp.json.

• Blueprint: assets.mcpServers prop → RepoConfig.mcp_servers column, with a synth-time RegistryRefValidation (rejects floating/malformed refs). Grammar extracted to a dependency-free registry-ref.ts so the construct layer validates without pulling aws-sdk into the synth graph.
• Orchestrator: resolveRegistryAssetsForTask resolves the blueprint's pins before hydration (fail-fast/fail-closed), stamps resolved_assets, threads the bundle into the agent payload. resolved_assets added to TaskRecord/TaskDetail/TaskSummary (+ CLI mirror).
• Agent: registry_loader.py merges the resolved MCP server_config into .mcp.json alongside the channel MCP; threaded server.py → pipeline.py → TaskConfig.
• CLI: bgagent registry publish/resolve/list/show.

Deploy-hardening fixes found validating against a live stack: create RegistryPublisher/RegistryApprover Cognito groups (bootstrap policy + action-map + DEPLOYMENT_ROLES.md updated); CLI --asset-version flag (--version collided with commander's global); memorySize: 256 on the registry Lambdas (128 MB OOM'd on init); mcp_server publish accepts inline server_config in lieu of an artifact; orchestrator Lambda gets REGISTRY_ASSETS_TABLE_NAME env + read grant.

Validated live: resolved_assets correctly stamped on a real TaskRecord.

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

[scottschreckengaust]

Principal Architect Review — PR #633 (registry orchestrator + agent + MCP E2E, #246, PR 2 of 3)

Review re-pinned to head df3c0f6 (a merge of main into the branch, post-49ba04a); the merge did not touch the reviewed incremental files or my inline anchors.

STACK / MERGE-ORDER (read first)

This PR is stacked on #632 — commit 49ba04a sits directly on #632's 773764f. GitHub reports base=main, so the full diff-vs-merge-base re-includes all of #632. I reviewed the incremental layer (orchestrator wiring + agent registry_loader.py + MCP E2E + blueprint assets.mcpServers + CLI registry commands + Cognito groups) and only sanity-checked integration against the #632 base.
Merge order is mandatory: ADR #548 → #632 (PR 1) → this (#632-dependent) → PR 3. Do NOT merge this before #632; its diff/tests assume #632's constructs (registryAssetsTable, resolver, publish/resolve handlers) are present.

1. Verdict — APPROVE (with nits)

The incremental layer is well-structured, fail-closed, and matches the #246 MVP acceptance criteria (blueprint pins one asset kind → orchestrator resolves at create-task boundary → stamps resolved_assets on the TaskRecord → agent merges resolved MCP server_config into .mcp.json). Grammar parity, IAM, and bootstrap coverage are handled. The one CI failure is inherited and by-design (see §6). Nothing blocking in this layer.

2. Vision alignment

• Bounded blast radius / fail-closed — resolveRegistryAssetsForTask resolves BEFORE hydration; a RegistryResolutionError propagates (no partial resolution), emits a registry_resolution_failed audit event, and fails the task the same way a guardrail block does (ADR-018 sub-decision 6, VISION fail-closed tenet). resolveAll refuses reserved kinds that have no loader rather than silently dropping them. Good.
• Reviewable outcomes — the {kind,id,version} audit triples on the TaskRecord + registry_assets_resolved/…failed events keep 'what asset versions ran' inspectable (issue AC: reproducibility).
• Policy-gated escalation — publish/auto-approve is gated on RegistryPublisher/RegistryApprover Cognito groups; the fire-and-forget submit path is unchanged (registry resolution is a no-op returning the empty bundle when a blueprint pins nothing).
• Tenet trade-offs — the DDB+S3 substrate choice (vs AgentCore Registry) is documented in ADR-018/REGISTRY.md (#632). No undocumented tenet trade in this layer.

3. Blocking issues

None in the incremental layer.

4. Non-blocking suggestions / nits

1. BOOTSTRAP_VERSION not bumped (advisory). cdk/src/bootstrap/version.ts stays at 1.2.0 while this PR adds 4 cognito-idp:*Group actions to the application policy and a new AWS::Cognito::UserPoolGroup resource-action-map entry. The review bar (ADR-002) asks for a minor bump when adding permissions (→ 1.3.0). The generated artifacts (application.json, bootstrap-template.yaml, BOOTSTRAP_HASH) WERE correctly regenerated and the hash/golden-baseline tests pass — so this is a semver-hygiene nit, not a functional gap (the deploy role will work). Recommend bumping so operators can tell the bundle changed. (Applies to the stack: #632 also added DDB/S3-backed constructs, though those reuse resource types already in main's action map.)
2. agent/src/registry_loader.py:52 _mcp_server_key docstring drift (comment-analyzer). The docstring says it 'Prefer[s] the descriptor's tool_prefix stripped of the mcp__ scaffolding … falling back to namespace-name', but the implementation only ever computes namespace-name — tool_prefix is never read. Either wire the tool_prefix preference or drop that sentence so the comment matches the code.
3. Non-paginated resolver Query (base #632 — note only, not a #633 blocker). cdk/src/handlers/shared/registry-resolver.ts resolveRef issues a single QueryCommand over all versions of a pk with no LastEvaluatedKey loop. DynamoDB caps a Query page at 1 MB; an asset that accumulates enough version rows to exceed a page could resolve to a lower version or NO_MATCHING_VERSION silently. Realistically far off (needs thousands of versions), and the file is base-layer — raise on #632, not here.
4. MCP key collision surface (minor). Registry keys derive from namespace-name; the channel MCP uses fixed keys (linear-server, etc.), so a collision is unlikely today, but a published acme-linear-server could shadow a channel entry in .mcp.json. A brief note or a registry- prefix would make the namespaces provably disjoint.

5. Documentation

• docs/design/DEPLOYMENT_ROLES.md (golden baseline) updated with the 4 cognito group actions; the Starlight mirror docs/src/content/docs/architecture/Deployment-roles.md is regenerated and in sync (ran node docs/scripts/sync-starlight.mjs → no drift; git status clean under docs/src/content/docs/).
• REGISTRY.md + Registry.md mirror are #632 files. No new docs required for this incremental layer beyond the deployment-roles delta, which is present.

6. Tests & CI (incl. bootstrap synth-coverage)

• CI red is INHERITED & by-design. The failing build (agentcore) job fails at //docs:link-check: design/REGISTRY.md → ../decisions/ADR-018-agent-asset-registry.md returns Status 400 because ADR-018 lives in unmerged PR #548 (docs/decisions/ADR-018* does not exist on this branch). REGISTRY.md is a #632 base-layer file, unchanged in the incremental diff — this is NOT introduced by #633. It self-resolves once #548 merges to main (the PR description acknowledges this). Not a #633 blocker; it IS a merge-ordering gate.
• Incremental layer builds & tests clean (verified locally in the worktree): tsc --noEmit (cdk) clean; CDK registry suites 252 tests pass (blueprint, task-api incl. the two new Cognito-group assertions, orchestrator-registry, registry-descriptor/-resolver/-publish/-list-show, registry-assets-table, registry-artifacts-bucket, agent stack); bootstrap suite 91 tests pass (synth-coverage, golden-baseline, artifact-sync hash, version); CLI registry.test.ts 7 pass; agent test_registry_loader.py + grammar-corpus + workflow-validator 64 pass; scripts/check-types-sync.ts OK (CLI ↔ CDK type mirror validated; the 6 new registry envelopes are allowlisted).
• Bootstrap synth-coverage: PASS. New AWS::Cognito::UserPoolGroup → cognito-idp:CreateGroup mapped; Create/Delete/Get/Update group actions added to application.ts + regenerated application.json + bootstrap-template.yaml; BOOTSTRAP_HASH regenerated (artifact-sync test green). Only gap is the version-string bump (§4.1).
• Test quality: covers happy path + failure paths — fail-closed propagation + failure-event emission, deprecated-resolution warning, empty-bundle no-op (no DDB write), inline-server_config-without-artifact accept vs cedar-module-still-requires-artifact reject, malformed/absent .mcp.json tolerance, missing-server_config skip, unknown-kind + malformed-id CLI rejection, and synth-time rejection of floating/malformed refs. Grammar parity is corpus-enforced across Python and TS.

7. Review agents run

• Manual principal-architect deep review — full incremental diff (orchestrator, registry_loader, registry-ref/-descriptor, blueprint, task-api, gateway, CLI, types).
• Security review — performed directly against the incremental code (the packaged /security-review skill misfired on a stale git context — it captured main/#596 instead of this worktree — so I did the pass by hand): traced the .mcp.json merge (no path traversal — fixed write target, JSON-key not a path, operator-published approved content), Cognito authz (extractGroups fail-closed → 403; auto_approve requires Approver; JWT claims server-verified), IAM least-privilege (group actions on the deploy role only; orchestrator gets read-only grantReadData), and fail-closed resolution. No HIGH/MEDIUM findings.
• code-reviewer / silent-failure-hunter / type-design-analyzer / comment-analyzer / pr-test-analyzer — folded into the manual pass (findings: nit §4.2 comment drift from comment-analyzer; type-design confirmed sound — payload carries full ResolvedAssetBundle, TaskRecord stamps lightweight ResolvedAssetSummary[], CLI mirror in sync; silent-failure — the resolved_assets stamp write failure is logged loudly and deliberately non-fatal, resolution failures propagate; test-analyzer — coverage is thorough incl. failure paths). The plugin agents' own subprocess dispatch inherited the same stale-git context as the security skill, so I could not get clean isolated plugin output; I state that here per the mandatory-agents requirement rather than parrot a misfired run.

8. Human heuristics

• Proportionality — PASS. Extracting the grammar into a dependency-free registry-ref.ts (so the construct layer validates at synth without dragging aws-sdk into the synth graph) is proportional and well-justified; the re-export from registry-resolver.ts keeps existing importers working. Cedar/skill loaders are honest logged-only stubs deferred to PR 3, not speculative machinery.
• Coherence — PASS. Same concept, same term: resolved_assets bundle vs summary shapes are consistent CDK↔CLI↔agent; the .mcp.json read-merge-write mirrors channel_mcp.py intentionally (documented). Registry work lands in the right packages per the AGENTS.md routing table.
• Clarity — mostly PASS. Names communicate intent; error handling surfaces failures (fail-closed + audit events) rather than hiding them. One drift at _mcp_server_key docstring (§4.2).
• Appropriateness — PASS. Integration verified against real DDB semver semantics (in-code semver ranking because DDB sorts version SKs lexicographically — a real, correctly-handled hazard); tests assert what the code should do (fail-closed, immutability, grammar rejection), not just what it does. Validated live per the PR description (resolved_assets stamped on a real TaskRecord).

[scottschreckengaust]

Nit (comment-analyzer): this docstring says the key 'Prefer[s] the descriptor's tool_prefix stripped of the mcp__ scaffolding … falling back to namespace-name', but the implementation below only ever computes namespace-name — tool_prefix is never read. Either wire the tool_prefix preference or drop that sentence so the comment matches the code. Non-blocking.

[scottschreckengaust]

Bootstrap coverage is otherwise correct here (new AWS::Cognito::UserPoolGroup → cognito-idp:CreateGroup, group actions added to the application policy, artifacts + BOOTSTRAP_HASH regenerated, golden baseline updated — 91 bootstrap tests green). One advisory gap: BOOTSTRAP_VERSION in cdk/src/bootstrap/version.ts stays at 1.2.0 despite added permissions; ADR-002 asks for a minor bump (→1.3.0). Not test-enforced, so non-blocking, but recommended so operators can see the bundle changed.

[isadeks]

Verdict: Request changes

I re-reviewed current head df3c0f6 without relying on the prior approval. The incremental layer has several fail-open paths that invalidate the MCP E2E and audit guarantees.

Blocking findings

1. [P1] A valid artifact-backed MCP asset is resolved and then silently skipped (agent/src/registry_loader.py:93-117). The publish contract accepts an MCP descriptor without inline server_config when artifact_b64 is present (the branch's own validMcp fixture has exactly that shape). The orchestrator bundle never fetches or carries those artifact bytes, while this loader reads only descriptor.server_config, logs a warning, and continues. Missing repo directories and .mcp.json write failures also return 0, and the caller ignores the result. A task can therefore be stamped and run without a pinned MCP server. Materialize and validate the artifact before dispatch (or require inline config), and raise when any resolved pin cannot be applied.

2. [P1] MCP keys are collision-prone and can replace channel configuration (agent/src/registry_loader.py:52-61, :104). The required tool_prefix is ignored and namespace-name is not injective: a-b/c and a/b-c both become a-b-c. linear/server becomes linear-server, the fixed channel key. Registry application runs after channel setup and assigns directly, so that asset overwrites the channel server despite the module claiming they always coexist. Use a canonical collision-free key (or the validated tool prefix), and reject collisions with existing entries instead of overwriting.

3. [P1] Both durable audit writes can fail while execution proceeds (cdk/src/handlers/shared/orchestrator.ts:428-449). Failure to stamp resolved_assets is caught, and failure to emit the fallback registry_assets_resolved event is swallowed. The bundle is still dispatched. This violates the ADR guarantee that every execution records the versions it ran and makes incident reconstruction impossible. Require at least one durable audit record before dispatch, ideally through the same atomic/outbox path as task state, and surface event failures.

4. [P1] Removing the last Blueprint pin leaves the old pin active (cdk/src/constructs/blueprint.ts:345-388). The update expression only mentions mcp_servers when the new list is non-empty. Updating a Blueprint from one pin to []/omitted therefore leaves the old DynamoDB attribute in place, and subsequent tasks keep resolving an asset the operator removed. Emit a REMOVE #mcp_servers clause or always set an explicit empty list; cover non-empty -> empty in a construct test.

5. [P2] assets.mcpServers validates grammar but not kind (cdk/src/constructs/blueprint.ts:236-240, :451-470). A registry://skill/... or registry://cedar_policy_module/... ref passes synth in the MCP field. resolveAll then routes it by its actual kind, bypassing the field's intended contract and potentially turning a reviewed MCP-only change into a different asset surface. Validate the expected kind per Blueprint field.

Verification

The agent registry/workflow suites pass. Direct probes reproduced both key collisions and the linear-server overwrite key; no test covers artifact-only MCP application or an application failure that must abort the task.