diff --git a/cmd/aws_devops_agent/confirm_test.go b/cmd/aws_devops_agent/confirm_test.go index 41e54377..cb519186 100644 --- a/cmd/aws_devops_agent/confirm_test.go +++ b/cmd/aws_devops_agent/confirm_test.go @@ -43,6 +43,12 @@ func (a countingAgent) CreateAgentSpace(context.Context, *devopsagent.CreateAgen return &devopsagent.CreateAgentSpaceOutput{AgentSpace: &agenttypes.AgentSpace{AgentSpaceId: aws.String("space-new")}}, nil } +func (a countingAgent) CreateAsset(context.Context, *devopsagent.CreateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.CreateAssetOutput, error) { + a.writes.add("CreateAsset") + + return &devopsagent.CreateAssetOutput{Asset: &agenttypes.Asset{AssetId: aws.String("ki-1")}}, nil +} + func (a countingAgent) DeregisterService(context.Context, *devopsagent.DeregisterServiceInput, ...func(*devopsagent.Options)) (*devopsagent.DeregisterServiceOutput, error) { a.writes.add("DeregisterService") @@ -83,6 +89,12 @@ func (a countingAgent) RegisterService(context.Context, *devopsagent.RegisterSer return &devopsagent.RegisterServiceOutput{ServiceId: aws.String("mcp-1")}, nil } +func (a countingAgent) UpdateAsset(context.Context, *devopsagent.UpdateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.UpdateAssetOutput, error) { + a.writes.add("UpdateAsset") + + return &devopsagent.UpdateAssetOutput{}, nil +} + type countingIAM struct{ writes *writes } func (c countingIAM) AttachRolePolicy(context.Context, *iam.AttachRolePolicyInput, ...func(*iam.Options)) (*iam.AttachRolePolicyOutput, error) { diff --git a/cmd/aws_devops_agent/plan_print_test.go b/cmd/aws_devops_agent/plan_print_test.go index 814f5fe0..adab4be7 100644 --- a/cmd/aws_devops_agent/plan_print_test.go +++ b/cmd/aws_devops_agent/plan_print_test.go @@ -65,6 +65,17 @@ func TestSetupPlanNamesRolesAndOperatorApp(t *testing.T) { assert.Contains(t, out.String(), awsdevops.AgentSpaceRoleName) assert.Contains(t, out.String(), awsdevops.OperatorAppRoleName) assert.Contains(t, out.String(), "operator app") + assert.Contains(t, out.String(), "Skill: Create skill "+awsdevops.ExperimentSkillName) +} + +// A canceled plan does not create the skill. +func TestSetupCanceledCreatesNoSkill(t *testing.T) { + cmd, _ := setupTestCmd(t, "n\n") + clients, w := countingClients() + + require.Error(t, setupWith(cmd, clients, newSpaceOptions(), true, true)) + + assert.NotContains(t, w.calls, "CreateAsset") } func noTokenOptions() awsdevops.SetupOptions { @@ -478,6 +489,43 @@ func TestSetupSaysWhenTheSecondPlanReusesAServer(t *testing.T) { assert.Contains(t, stderrOf(cmd), "--access-token and --replace-mcp-token") } +type failingSkillAgent struct{ countingAgent } + +func (failingSkillAgent) CreateAsset(context.Context, *devopsagent.CreateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.CreateAssetOutput, error) { + return nil, &agenttypes.AccessDeniedException{Message: aws.String("not authorized to perform: aidevops:CreateAsset")} +} + +// A failed skill step warns at the end, and setup still prints the app URL. +func TestSetupWarnsWhenSkillFails(t *testing.T) { + cmd, errOut := setupTestCmd(t, "") + cmd.SetIn(noReadStdin{t}) + clients, w := countingClients() + clients.Agent = failingSkillAgent{clients.Agent.(countingAgent)} + + require.NoError(t, setupWith(cmd, clients, newSpaceOptions(), true, false)) + + assert.Contains(t, w.calls, "RegisterService") + assert.Contains(t, stdoutOf(cmd), "Warning: unable to create skill "+awsdevops.ExperimentSkillName) + assert.Contains(t, stdoutOf(cmd), "aidevops:CreateAsset") + assert.Contains(t, stdoutOf(cmd), "Open the DevOps Agent at:") + // The warning shows one time. + assert.Equal(t, 1, strings.Count(errOut.String()+stdoutOf(cmd), "unable to create skill")) +} + +// stdout holds only the JSON, so the warning goes to stderr. +func TestSetupJSONWarnsOnStderrWhenSkillFails(t *testing.T) { + cmd, errOut := setupTestCmd(t, "") + cmd.SetIn(noReadStdin{t}) + clients, _ := countingClients() + clients.Agent = failingSkillAgent{clients.Agent.(countingAgent)} + + require.NoError(t, setupWith(cmd, clients, newSpaceOptions(), false, false)) + + assert.True(t, json.Valid([]byte(stdoutOf(cmd))), "stdout is not JSON: %s", stdoutOf(cmd)) + assert.Contains(t, stdoutOf(cmd), `"skillAction": "failed"`) + assert.Contains(t, errOut.String(), "Warning: unable to create skill "+awsdevops.ExperimentSkillName) +} + // Without a name from AWS, the warning gives only the ID, which identifies // the server, and does not guess a name. func TestSetupNamesANamelessServerByItsID(t *testing.T) { diff --git a/cmd/aws_devops_agent/select_test.go b/cmd/aws_devops_agent/select_test.go index 3d0555c9..9c538089 100644 --- a/cmd/aws_devops_agent/select_test.go +++ b/cmd/aws_devops_agent/select_test.go @@ -32,6 +32,22 @@ func (s stubAgent) AssociateService(context.Context, *devopsagent.AssociateServi return nil, nil } +func (s stubAgent) CreateAsset(context.Context, *devopsagent.CreateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.CreateAssetOutput, error) { + return &devopsagent.CreateAssetOutput{Asset: &agenttypes.Asset{AssetId: aws.String("ki-1")}}, nil +} + +func (s stubAgent) GetAssetFile(context.Context, *devopsagent.GetAssetFileInput, ...func(*devopsagent.Options)) (*devopsagent.GetAssetFileOutput, error) { + return nil, &agenttypes.ResourceNotFoundException{} +} + +func (s stubAgent) ListAssets(context.Context, *devopsagent.ListAssetsInput, ...func(*devopsagent.Options)) (*devopsagent.ListAssetsOutput, error) { + return &devopsagent.ListAssetsOutput{}, nil +} + +func (s stubAgent) UpdateAsset(context.Context, *devopsagent.UpdateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.UpdateAssetOutput, error) { + return &devopsagent.UpdateAssetOutput{}, nil +} + func (s stubAgent) CreateAgentSpace(context.Context, *devopsagent.CreateAgentSpaceInput, ...func(*devopsagent.Options)) (*devopsagent.CreateAgentSpaceOutput, error) { return nil, nil } @@ -97,6 +113,10 @@ func (s stubAgent) ListAssociations(context.Context, *devopsagent.ListAssociatio return &devopsagent.ListAssociationsOutput{Associations: s.associations}, nil } +func (s stubAgent) UpdateAssociation(context.Context, *devopsagent.UpdateAssociationInput, ...func(*devopsagent.Options)) (*devopsagent.UpdateAssociationOutput, error) { + return &devopsagent.UpdateAssociationOutput{}, nil +} + func (s stubAgent) ListServices(context.Context, *devopsagent.ListServicesInput, ...func(*devopsagent.Options)) (*devopsagent.ListServicesOutput, error) { return &devopsagent.ListServicesOutput{Services: s.services}, nil } diff --git a/cmd/aws_devops_agent/setup.go b/cmd/aws_devops_agent/setup.go index 3ff70ec2..b869add8 100644 --- a/cmd/aws_devops_agent/setup.go +++ b/cmd/aws_devops_agent/setup.go @@ -28,6 +28,8 @@ const ( replaceMCPTokenFlag = "replace-mcp-token" mcpServiceIDFlag = "mcp-service-id" mcpEndpointFlag = "mcp-endpoint" + replaceSkillFlag = "replace-skill" + addMCPToolsFlag = "add-mcp-tools" yesFlag = "yes" ) @@ -35,17 +37,18 @@ func NewSetupCmd(analyticsTrackerFn analytics.TrackerFn) *cobra.Command { cmd := &cobra.Command{ Use: "setup", Short: "Provision the AWS DevOps Agent for LaunchDarkly", - Long: `Create the IAM roles, agent space, AWS account association, operator app and -LaunchDarkly MCP server connection the AWS DevOps Agent needs. + Long: `Create the IAM roles, agent space, AWS account association, operator app, +LaunchDarkly MCP server connection and experimentation skill the AWS DevOps +Agent needs. When the account already has agent spaces, setup asks which one to connect LaunchDarkly to. On a space that setup did not make, setup only registers the -MCP server. On a space that setup made, or with --full-setup, setup runs the -full setup. A full setup turns on elevated actions on the space. Pass ---agent-space-id to pick a space without the list, or --new-agent-space to -provision another space. A new space needs a name: pass --agent-space-name, -or type one when setup asks. The run ends with the operator app URL, where -the agent is used. +MCP server and adds the skill. On a space that setup made, or with +--full-setup, setup runs the full setup. A full setup turns on elevated +actions on the space. Pass --agent-space-id to pick a space without the list, +or --new-agent-space to provision another space. A new space needs a name: +pass --agent-space-name, or type one when setup asks. The run ends with the +operator app URL, where the agent is used. Each agent space gets its own LaunchDarkly MCP server registration, so each space can use a different token. The MCP server is connected with @@ -55,7 +58,19 @@ session token from 'ldcli login' stops working once it expires. If the agent space already has an MCP server, setup reuses it and does not change its token. If it has none, setup asks for a service token in a terminal and checks it before it changes AWS. Without a terminal, or with --yes, it stops and asks -for --access-token.`, +for --access-token. + +A space that reuses a server keeps its tool allowlist. If the allowlist misses +tools that this ldcli version allows, setup warns. To add them, pass +--add-mcp-tools. AWS does not send back the classification of each tool, so +the rewrite gives every tool the ldcli classification. + +The experiment-orchestration skill tells the agent how to run an experiment +from a goal to a report. Every agent type in the space can load it. If the +skill has the text of an older ldcli version, setup updates it. If someone +edited it, setup does not change it. To overwrite the edits, pass +--replace-skill. The skill is optional: if its step fails, setup warns and +does the other steps.`, Args: cobra.NoArgs, PreRun: trackRun(analyticsTrackerFn), RunE: runSetup, @@ -76,6 +91,9 @@ for --access-token.`, cmd.Flags().Bool(replaceMCPTokenFlag, false, "Give the agent space a new LaunchDarkly MCP server with the token passed with --access-token. Other agent spaces keep their server") cmd.Flags().String(mcpServiceIDFlag, "", "Registered MCP server to connect, instead of the server of the agent space") cmd.Flags().String(mcpEndpointFlag, "", "MCP endpoint to register. Setup reuses only a server at this endpoint. Defaults to "+awsdevops.MCPServerEndpoint) + cmd.Flags().Bool(replaceSkillFlag, false, "Overwrite the "+awsdevops.ExperimentSkillName+" skill if someone edited it") + cmd.Flags().Bool(addMCPToolsFlag, false, "Add the ldcli tools that the MCP server allowlist of an existing agent space misses. "+ + "Every tool gets the ldcli classification") cmd.Flags().Bool(yesFlag, false, "Ask no questions and accept every step") @@ -174,6 +192,14 @@ func setupWith( } if !plaintext { + // stdout holds only the JSON, so a CI log shows the warning on stderr. + if result.MCPToolsError != "" { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", result.MCPToolsError) + } + if result.SkillError != "" { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", result.SkillError) + } + return printJSON(cmd, result) } @@ -184,6 +210,13 @@ func setupWith( awsdevops.AccessTokenURL(opts.LDBaseURI)) } + if result.MCPToolsError != "" { + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nWarning: %s\n", result.MCPToolsError) + } + if result.SkillError != "" { + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nWarning: %s\n", result.SkillError) + } + // Only a connect run leaves the app off, on a space that setup did not // make. The full setup would change that space, so point to the console. if !result.OperatorAppEnabled { @@ -507,7 +540,8 @@ func confirmSetup(cmd *cobra.Command, plan awsdevops.SetupPlan, ask bool) error _, _ = fmt.Fprintf(out, " Agent space: %s\n", text.AgentSpace) _, _ = fmt.Fprintf(out, " Association: %s\n", text.Association) _, _ = fmt.Fprintf(out, " Operator app: %s\n", text.OperatorApp) - _, _ = fmt.Fprintf(out, " MCP server: %s\n\n", text.MCPServer) + _, _ = fmt.Fprintf(out, " MCP server: %s\n", text.MCPServer) + _, _ = fmt.Fprintf(out, " Skill: %s\n\n", text.Skill) if !ask { return nil } @@ -695,6 +729,8 @@ func setupOptions(cmd *cobra.Command) (awsdevops.SetupOptions, error) { ReplaceMCPToken: mustBool(cmd, replaceMCPTokenFlag), MCPServiceID: mustString(cmd, mcpServiceIDFlag), MCPEndpoint: mustString(cmd, mcpEndpointFlag), + ReplaceSkill: mustBool(cmd, replaceSkillFlag), + AddMCPTools: mustBool(cmd, addMCPToolsFlag), } idcSet := opts.IdcInstanceARN != "" diff --git a/internal/awsdevops/allowlist_test.go b/internal/awsdevops/allowlist_test.go new file mode 100644 index 00000000..db496935 --- /dev/null +++ b/internal/awsdevops/allowlist_test.go @@ -0,0 +1,162 @@ +package awsdevops_test + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + agenttypes "github.com/aws/aws-sdk-go-v2/service/devopsagent/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/launchdarkly/ldcli/internal/awsdevops" +) + +// mcpAssociation is the association of server mcp-1 with space-2 that allows +// these tools. +func mcpAssociation(tools ...string) agenttypes.Association { + return agenttypes.Association{ + AgentSpaceId: aws.String("space-2"), + AssociationId: aws.String("assoc-mcp-1"), + ServiceId: aws.String("mcp-1"), + Configuration: &agenttypes.ServiceConfigurationMemberMcpserver{ + Value: agenttypes.MCPServerConfiguration{Tools: tools}, + }, + Capabilities: map[string]agenttypes.CapabilityConfiguration{"kept": {}}, + } +} + +func reuseServerOptions() awsdevops.SetupOptions { + opts := reuseOptions() + opts.LDAccessToken = "" + opts.AddMCPTools = true + + return opts +} + +// An older ldcli allowed fewer tools. A re-run adds the new ones, so the skill +// can use them. +func TestSetupAddsMissingToolsToAnExistingAllowlist(t *testing.T) { + agent := &fakeAgent{ + services: []agenttypes.RegisteredService{launchDarklyMCPService("mcp-1")}, + associations: []agenttypes.Association{mcpAssociation("list-projects", "get-flag", "toggle-flag")}, + } + clients := newTestClients(agent, newFakeIAM()) + + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseServerOptions(), true) + require.NoError(t, err) + require.Equal(t, awsdevops.MCPReuse, plan.MCP.Action) + assert.Contains(t, plan.Text().MCPServer, "and add get-project, get-environment, list-flags,") + assert.Contains(t, plan.Text().MCPServer, "update-flag-settings") + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseServerOptions(), plan) + require.NoError(t, err) + + require.Len(t, agent.updateAssociations, 1) + in := agent.updateAssociations[0] + assert.Equal(t, "assoc-mcp-1", aws.ToString(in.AssociationId)) + config, ok := in.Configuration.(*agenttypes.ServiceConfigurationMemberMcpserver) + require.True(t, ok) + assert.Contains(t, config.Value.Tools, "update-flag-settings") + assert.Contains(t, config.Value.ToolDetails, agenttypes.MCPToolDetail{ + Name: aws.String("update-flag-settings"), + ToolClassification: agenttypes.ToolClassificationMutative, + }) + // AWS replaces the whole association, so setup sends the capabilities back. + assert.Contains(t, in.Capabilities, "kept") + assert.Contains(t, result.MCPToolsAdded, "create-metric") + assert.NotContains(t, result.MCPToolsAdded, "get-flag") + assert.Empty(t, result.MCPToolsError) +} + +func TestSetupKeepsACompleteAllowlist(t *testing.T) { + agent := &fakeAgent{ + services: []agenttypes.RegisteredService{launchDarklyMCPService("mcp-1")}, + associations: []agenttypes.Association{mcpAssociation(awsdevops.MCPTools()...)}, + } + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseServerOptions()) + require.NoError(t, err) + + assert.Empty(t, agent.updateAssociations) + assert.Empty(t, result.MCPToolsAdded) +} + +// AWS does not send the classification of each tool back. Rewriting an +// allowlist with a tool that ldcli does not know would make that tool +// read-only, so setup warns instead. +func TestSetupLeavesAnAllowlistWithForeignTools(t *testing.T) { + agent := &fakeAgent{ + services: []agenttypes.RegisteredService{launchDarklyMCPService("mcp-1")}, + associations: []agenttypes.Association{mcpAssociation("get-flag", "delete-everything")}, + } + clients := newTestClients(agent, newFakeIAM()) + + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseServerOptions(), true) + require.NoError(t, err) + assert.Contains(t, plan.Text().MCPServer, "which setup cannot add") + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseServerOptions(), plan) + require.NoError(t, err) + + assert.Empty(t, agent.updateAssociations) + assert.Contains(t, result.MCPToolsError, "Add the missing tools in the AWS console") +} + +// Setup cannot tell an older ldcli allowlist from one that someone trimmed, +// so without --add-mcp-tools it only warns. +func TestSetupWarnsAboutMissingToolsWithoutTheFlag(t *testing.T) { + agent := &fakeAgent{ + services: []agenttypes.RegisteredService{launchDarklyMCPService("mcp-1")}, + associations: []agenttypes.Association{mcpAssociation("get-flag")}, + } + clients := newTestClients(agent, newFakeIAM()) + opts := reuseServerOptions() + opts.AddMCPTools = false + + plan, err := awsdevops.PlanSetup(context.Background(), clients, opts, true) + require.NoError(t, err) + assert.Contains(t, plan.Text().MCPServer, "Pass --add-mcp-tools to add them") + + result, err := awsdevops.RunPlan(context.Background(), clients, opts, plan) + require.NoError(t, err) + + assert.Empty(t, agent.updateAssociations) + assert.Contains(t, result.MCPToolsError, "--add-mcp-tools") +} + +// The run makes only the change that the plan showed. +func TestSetupSkipsAnAllowlistThatChangedAfterThePlan(t *testing.T) { + agent := &fakeAgent{ + services: []agenttypes.RegisteredService{launchDarklyMCPService("mcp-1")}, + associations: []agenttypes.Association{mcpAssociation("get-flag", "delete-everything")}, + } + clients := newTestClients(agent, newFakeIAM()) + + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseServerOptions(), true) + require.NoError(t, err) + agent.associations = []agenttypes.Association{mcpAssociation("get-flag")} + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseServerOptions(), plan) + require.NoError(t, err) + + assert.Empty(t, agent.updateAssociations) + assert.Contains(t, result.MCPToolsError, "changed after the plan") +} + +// The server works with the tools that it has, so a failed update does not +// fail setup. +func TestSetupWarnsWhenTheAllowlistUpdateFails(t *testing.T) { + agent := &fakeAgent{ + services: []agenttypes.RegisteredService{launchDarklyMCPService("mcp-1")}, + associations: []agenttypes.Association{mcpAssociation("get-flag")}, + updateAssociationErr: errors.New("AccessDeniedException: not allowed"), + } + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseServerOptions()) + require.NoError(t, err) + + assert.Contains(t, result.MCPToolsError, "unable to add") + assert.Empty(t, result.MCPToolsAdded) +} diff --git a/internal/awsdevops/awsdevops.go b/internal/awsdevops/awsdevops.go index ae234bd8..f913c661 100644 --- a/internal/awsdevops/awsdevops.go +++ b/internal/awsdevops/awsdevops.go @@ -51,17 +51,22 @@ var ( type AgentAPI interface { AssociateService(context.Context, *devopsagent.AssociateServiceInput, ...func(*devopsagent.Options)) (*devopsagent.AssociateServiceOutput, error) CreateAgentSpace(context.Context, *devopsagent.CreateAgentSpaceInput, ...func(*devopsagent.Options)) (*devopsagent.CreateAgentSpaceOutput, error) + CreateAsset(context.Context, *devopsagent.CreateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.CreateAssetOutput, error) DeregisterService(context.Context, *devopsagent.DeregisterServiceInput, ...func(*devopsagent.Options)) (*devopsagent.DeregisterServiceOutput, error) DisassociateService(context.Context, *devopsagent.DisassociateServiceInput, ...func(*devopsagent.Options)) (*devopsagent.DisassociateServiceOutput, error) EnableOperatorApp(context.Context, *devopsagent.EnableOperatorAppInput, ...func(*devopsagent.Options)) (*devopsagent.EnableOperatorAppOutput, error) GetOperatorApp(context.Context, *devopsagent.GetOperatorAppInput, ...func(*devopsagent.Options)) (*devopsagent.GetOperatorAppOutput, error) ListTagsForResource(context.Context, *devopsagent.ListTagsForResourceInput, ...func(*devopsagent.Options)) (*devopsagent.ListTagsForResourceOutput, error) GetAgentSpace(context.Context, *devopsagent.GetAgentSpaceInput, ...func(*devopsagent.Options)) (*devopsagent.GetAgentSpaceOutput, error) + GetAssetFile(context.Context, *devopsagent.GetAssetFileInput, ...func(*devopsagent.Options)) (*devopsagent.GetAssetFileOutput, error) ListAgentSpaces(context.Context, *devopsagent.ListAgentSpacesInput, ...func(*devopsagent.Options)) (*devopsagent.ListAgentSpacesOutput, error) + ListAssets(context.Context, *devopsagent.ListAssetsInput, ...func(*devopsagent.Options)) (*devopsagent.ListAssetsOutput, error) ListAssociations(context.Context, *devopsagent.ListAssociationsInput, ...func(*devopsagent.Options)) (*devopsagent.ListAssociationsOutput, error) ListServices(context.Context, *devopsagent.ListServicesInput, ...func(*devopsagent.Options)) (*devopsagent.ListServicesOutput, error) RegisterService(context.Context, *devopsagent.RegisterServiceInput, ...func(*devopsagent.Options)) (*devopsagent.RegisterServiceOutput, error) UpdateAgentSpace(context.Context, *devopsagent.UpdateAgentSpaceInput, ...func(*devopsagent.Options)) (*devopsagent.UpdateAgentSpaceOutput, error) + UpdateAsset(context.Context, *devopsagent.UpdateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.UpdateAssetOutput, error) + UpdateAssociation(context.Context, *devopsagent.UpdateAssociationInput, ...func(*devopsagent.Options)) (*devopsagent.UpdateAssociationOutput, error) } // IAMAPI is the subset of the IAM API this package uses. diff --git a/internal/awsdevops/awsdevops_test.go b/internal/awsdevops/awsdevops_test.go index 8882e8d1..6d8df018 100644 --- a/internal/awsdevops/awsdevops_test.go +++ b/internal/awsdevops/awsdevops_test.go @@ -14,6 +14,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/devopsagent" + "github.com/aws/aws-sdk-go-v2/service/devopsagent/document" agenttypes "github.com/aws/aws-sdk-go-v2/service/devopsagent/types" "github.com/aws/aws-sdk-go-v2/service/iam" iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types" @@ -68,6 +69,88 @@ type fakeAgent struct { // paginate makes every list API return one item per page, so callers are // exercised against the NextToken AWS sends for a long list. paginate bool + // assets holds the skill assets of every space. + assets []agenttypes.Asset + // skillFiles holds the SKILL.md text by asset ID. + skillFiles map[string]string + // skillBytes holds a SKILL.md that AWS sends as bytes, by asset ID. + skillBytes map[string][]byte + listAssetsErr error + getFileErr error + // onGetFile runs at the start of each GetAssetFile call. + onGetFile func() + createAssetErr error + createAssets []*devopsagent.CreateAssetInput + // updateAssociations holds the allowlist updates. + updateAssociations []*devopsagent.UpdateAssociationInput + updateAssociationErr error + updateAssets []*devopsagent.UpdateAssetInput +} + +// skillAsset is a skill asset named name. +func skillAsset(id, name string) agenttypes.Asset { + return agenttypes.Asset{ + AssetId: aws.String(id), + AssetType: aws.String("skill"), + Metadata: document.NewLazyDocument(map[string]any{"name": name, "agent_types": []string{"GENERIC"}}), + } +} + +func (f *fakeAgent) CreateAsset(_ context.Context, in *devopsagent.CreateAssetInput, _ ...func(*devopsagent.Options)) (*devopsagent.CreateAssetOutput, error) { + f.calls = append(f.calls, "CreateAsset") + f.createAssets = append(f.createAssets, in) + if f.createAssetErr != nil { + return nil, f.createAssetErr + } + asset := agenttypes.Asset{AssetId: aws.String("ki-created"), AssetType: in.AssetType, Metadata: in.Metadata} + f.assets = append(f.assets, asset) + + return &devopsagent.CreateAssetOutput{Asset: &asset}, nil +} + +func (f *fakeAgent) GetAssetFile(_ context.Context, in *devopsagent.GetAssetFileInput, _ ...func(*devopsagent.Options)) (*devopsagent.GetAssetFileOutput, error) { + f.calls = append(f.calls, "GetAssetFile") + if f.onGetFile != nil { + f.onGetFile() + } + if f.getFileErr != nil { + return nil, f.getFileErr + } + if aws.ToString(in.Path) != "SKILL.md" { + return nil, &agenttypes.ResourceNotFoundException{} + } + if body, ok := f.skillBytes[aws.ToString(in.AssetId)]; ok { + return &devopsagent.GetAssetFileOutput{File: &agenttypes.AssetFile{ + Path: in.Path, + Content: &agenttypes.AssetFileBodyMemberBytes{Value: body}, + }}, nil + } + text, ok := f.skillFiles[aws.ToString(in.AssetId)] + if !ok { + return nil, &agenttypes.ResourceNotFoundException{} + } + + return &devopsagent.GetAssetFileOutput{File: &agenttypes.AssetFile{ + Path: in.Path, + Content: &agenttypes.AssetFileBodyMemberText{Value: text}, + }}, nil +} + +func (f *fakeAgent) ListAssets(_ context.Context, in *devopsagent.ListAssetsInput, _ ...func(*devopsagent.Options)) (*devopsagent.ListAssetsOutput, error) { + f.calls = append(f.calls, "ListAssets") + if f.listAssetsErr != nil { + return nil, f.listAssetsErr + } + items, next := page(f.assets, in.NextToken, f.paginate) + + return &devopsagent.ListAssetsOutput{Items: items, NextToken: next}, nil +} + +func (f *fakeAgent) UpdateAsset(_ context.Context, in *devopsagent.UpdateAssetInput, _ ...func(*devopsagent.Options)) (*devopsagent.UpdateAssetOutput, error) { + f.calls = append(f.calls, "UpdateAsset") + f.updateAssets = append(f.updateAssets, in) + + return &devopsagent.UpdateAssetOutput{Asset: &agenttypes.Asset{AssetId: in.AssetId}}, nil } func page[T any](items []T, token *string, paginate bool) ([]T, *string) { @@ -241,6 +324,16 @@ func (f *fakeAgent) ListAssociations(_ context.Context, in *devopsagent.ListAsso return &devopsagent.ListAssociationsOutput{Associations: items, NextToken: next}, nil } +func (f *fakeAgent) UpdateAssociation(_ context.Context, in *devopsagent.UpdateAssociationInput, _ ...func(*devopsagent.Options)) (*devopsagent.UpdateAssociationOutput, error) { + f.calls = append(f.calls, "UpdateAssociation") + f.updateAssociations = append(f.updateAssociations, in) + if f.updateAssociationErr != nil { + return nil, f.updateAssociationErr + } + + return &devopsagent.UpdateAssociationOutput{}, nil +} + func (f *fakeAgent) RegisterService(_ context.Context, in *devopsagent.RegisterServiceInput, _ ...func(*devopsagent.Options)) (*devopsagent.RegisterServiceOutput, error) { f.calls = append(f.calls, "RegisterService") f.registerInputs = append(f.registerInputs, in) @@ -384,6 +477,7 @@ func TestSetupCreatesRolesAgentSpaceAndAssociations(t *testing.T) { assert.Equal(t, "mcp-1", result.MCPServiceID) assert.Equal(t, "assoc-mcp-1", result.MCPAssociationID) assert.Equal(t, "https://space-1.aidevops.global.app.aws", result.OperatorAppURL) + assert.Equal(t, "ki-created", result.SkillAssetID) assert.Equal(t, map[string]string{awsdevops.ManagedTagKey: awsdevops.ManagedTagValue}, agent.createdTags) assert.Equal(t, []string{awsdevops.AgentSpaceRoleName, awsdevops.OperatorAppRoleName}, iamClient.createdRoles) assert.Equal(t, awsdevops.AgentSpacePolicyARN, iamClient.attachedPolicy[awsdevops.AgentSpaceRoleName]) @@ -392,8 +486,11 @@ func TestSetupCreatesRolesAgentSpaceAndAssociations(t *testing.T) { assert.Equal( t, // The plan does not look for the server of a new space. The check - // before the MCP step looks, because the space exists by then. - []string{"CreateAgentSpace", "AssociateService", "EnableOperatorApp", "ListServices", "RegisterService", "AssociateService"}, + // before the MCP step looks, because the space exists by then, + // then the skill last. + []string{ + "CreateAgentSpace", "AssociateService", "EnableOperatorApp", "ListServices", "RegisterService", "AssociateService", "ListAssets", "CreateAsset", + }, agent.calls, ) } @@ -916,6 +1013,7 @@ func TestMCPAssociationClassifiesTheSkillTools(t *testing.T) { "toggle-flag", "create-flag", "update-rollout", "archive-flag", "create-experiment", "start-experiment-iteration", "stop-experiment-iteration", "start-guarded-rollout", "stop-guarded-rollout", "update-targeting-rules", "update-individual-targets", "create-approval-request", + "apply-approval-request", } { assert.Equal(t, agenttypes.ToolClassificationMutative, classes[tool], tool) } diff --git a/internal/awsdevops/export_test.go b/internal/awsdevops/export_test.go index 1bf7e317..66dabb10 100644 --- a/internal/awsdevops/export_test.go +++ b/internal/awsdevops/export_test.go @@ -22,6 +22,20 @@ func ConnectExisting(ctx context.Context, clients Clients, opts SetupOptions) (S return ConnectPlan(ctx, clients, opts, plan) } +// ShipSkill adds text to the shipped skill versions until the test ends. +func ShipSkill(text string) func() { + saved := shippedSkillHashes + shippedSkillHashes = append(append([]string(nil), saved...), skillHash(text)) + + return func() { shippedSkillHashes = saved } +} + +// ShippedSkillHashes are the hashes of the shipped skill versions. +func ShippedSkillHashes() []string { return shippedSkillHashes } + +// SkillHash is the hash that setup compares. +func SkillHash(text string) string { return skillHash(text) } + // MCPTools lists every tool in the allowlist, for tests. func MCPTools() []string { return append(append([]string{}, mcpReadOnlyTools...), mcpMutativeTools...) diff --git a/internal/awsdevops/lookup.go b/internal/awsdevops/lookup.go index a8efbdae..feb104f5 100644 --- a/internal/awsdevops/lookup.go +++ b/internal/awsdevops/lookup.go @@ -348,3 +348,25 @@ func serviceName(service agenttypes.RegisteredService) string { return aws.ToString(service.Name) } + +func listAssets(ctx context.Context, clients Clients, agentSpaceID, assetType string) ([]agenttypes.Asset, error) { + var ( + assets []agenttypes.Asset + nextToken *string + ) + for { + page, err := clients.Agent.ListAssets(ctx, &devopsagent.ListAssetsInput{ + AgentSpaceId: aws.String(agentSpaceID), + AssetType: aws.String(assetType), + NextToken: nextToken, + }) + if err != nil { + return nil, fmt.Errorf("unable to list the %s assets of agent space %s: %w", assetType, agentSpaceID, err) + } + assets = append(assets, page.Items...) + if page.NextToken == nil { + return assets, nil + } + nextToken = page.NextToken + } +} diff --git a/internal/awsdevops/setup.go b/internal/awsdevops/setup.go index e3deb7e1..ebb37eaf 100644 --- a/internal/awsdevops/setup.go +++ b/internal/awsdevops/setup.go @@ -52,9 +52,13 @@ const ( // The agent invokes read-only tools without asking for approval, so anything // that changes LaunchDarkly state has to be classified explicitly. The lists // cover the steps of the experiment-orchestration skill: find a flag, create -// it, run an experiment, read its results, and ramp or clean up the flag. +// it, run an experiment, read its results, and ramp or clean up the flag. The +// skill keeps its experiment record in the flag description and creates a +// missing primary metric, so the lists include update-flag-settings and +// create-metric. // They also cover incident work: turn a change off for one segment or -// context, and ask for approval where an environment needs it. +// context, and ask for approval where an environment needs it, then apply +// the request after a reviewer approves it. var ( mcpReadOnlyTools = []string{ "list-projects", @@ -80,6 +84,8 @@ var ( mcpMutativeTools = []string{ "toggle-flag", "create-flag", + "update-flag-settings", + "create-metric", "update-rollout", "archive-flag", "create-experiment", @@ -90,6 +96,7 @@ var ( "update-targeting-rules", "update-individual-targets", "create-approval-request", + "apply-approval-request", } ) @@ -121,6 +128,15 @@ type SetupOptions struct { // registration, so a self-hosted one is never confused with ours. MCPEndpoint string + // ReplaceSkill overwrites the SKILL.md of an existing + // experiment-orchestration skill that someone edited. Setup updates a + // SKILL.md from an older ldcli version without this option. + ReplaceSkill bool + // AddMCPTools adds the ldcli tools that the allowlist of an existing MCP + // server association misses. AWS does not send back the classification of + // each tool, so the rewrite gives every tool the ldcli classification. + AddMCPTools bool + // Logf, when set, reports progress as each step completes. Logf func(format string, args ...any) } @@ -138,6 +154,18 @@ type SetupResult struct { OperatorAppEnabled bool `json:"operatorAppEnabled"` MCPServiceID string `json:"mcpServiceId,omitempty"` MCPAssociationID string `json:"mcpAssociationId,omitempty"` + // MCPToolsAdded lists the ldcli tools that setup added to the allowlist of + // an existing MCP server association. + MCPToolsAdded []string `json:"mcpToolsAdded,omitempty"` + // MCPToolsError is set if the allowlist of an existing association misses + // ldcli tools that setup could not add. The rest of the setup ran. + MCPToolsError string `json:"mcpToolsError,omitempty"` + SkillAssetID string `json:"skillAssetId,omitempty"` + // SkillAction is what the skill step did, for example "kept" or "left". + SkillAction string `json:"skillAction,omitempty"` + // SkillError is set if the skill step failed or was skipped. The skill is + // optional, so the rest of the setup ran. + SkillError string `json:"skillError,omitempty"` } // ErrPlanChanged means AWS changed after the plan. @@ -372,6 +400,13 @@ type MCPStep struct { // space uses. A replace on a shared server removes it first, so that the // new server can have the name. An earlier run that failed leaves one. Unused string + // MissingTools are the ldcli tools that the allowlist of the existing + // association in the space misses. CanAddTools says whether setup can add + // them. + MissingTools []string + CanAddTools bool + // AddTools is set by --add-mcp-tools. + AddTools bool } // shared reports whether other spaces use the server. @@ -408,6 +443,7 @@ type SetupPlan struct { // AuthFlow is the sign-in that a full setup sets. AuthFlow string MCP MCPStep + Skill SkillStep } // PlanText is the plan as lines for a person to read. @@ -419,11 +455,12 @@ type PlanText struct { Roles []string OperatorApp string MCPServer string + Skill string } // Text describes each step of the plan. func (p SetupPlan) Text() PlanText { - text := PlanText{Scope: "Full setup", AgentSpace: p.Space.describe(), MCPServer: p.MCP.describe()} + text := PlanText{Scope: "Full setup", AgentSpace: p.Space.describe(), MCPServer: p.MCP.describe(), Skill: p.Skill.describe()} if p.Space.Action == SpaceConnect { text.Scope = "Connect the MCP server only. To also update the IAM roles, " + "account association, and operator app of the space, run setup again with --full-setup" @@ -508,6 +545,19 @@ func PlanSetup(ctx context.Context, clients Clients, opts SetupOptions, provisio return plan, err } + if plan.MCP.Action == MCPReuse && plan.Space.Action != SpaceCreate { + association, err := findMCPAssociation(ctx, clients, plan.Space.ID, plan.MCP.ServiceID) + if err != nil { + return plan, err + } + if association != nil { + plan.MCP.MissingTools, plan.MCP.CanAddTools = missingTools(*association) + plan.MCP.AddTools = opts.AddMCPTools + } + } + + plan.Skill = planSkill(ctx, clients, opts, plan.Space) + if !provision { // Read the app, so the run does not print a URL that does not work. plan.App, err = appState(ctx, clients, plan.Space.ID) @@ -825,6 +875,18 @@ func (m MCPStep) describe() string { strings.Join(m.otherSpaces(), ", "), ) } + switch { + case len(m.MissingTools) == 0: + case m.CanAddTools && m.AddTools: + line += ", and add " + strings.Join(m.MissingTools, ", ") + + " to its allowlist. The rewrite gives each tool the ldcli classification" + case m.CanAddTools: + line += ". Its allowlist misses " + strings.Join(m.MissingTools, ", ") + + ". Pass --add-mcp-tools to add them" + default: + line += ". Its allowlist misses " + strings.Join(m.MissingTools, ", ") + + ", which setup cannot add because the allowlist has tools that ldcli does not classify" + } return line case MCPRegister: @@ -1031,6 +1093,9 @@ func RunPlan(ctx context.Context, clients Clients, opts SetupOptions, plan Setup if err := registerMCPServer(ctx, clients, opts, plan.MCP, &result, logf); err != nil { return result, err } + // The skill goes last, so a failure of the optional step cannot stop the + // MCP server step. + installSkill(ctx, clients, plan.Skill, &result, logf) return result, nil } @@ -1109,6 +1174,7 @@ func ConnectPlan(ctx context.Context, clients Clients, opts SetupOptions, plan S if err := registerMCPServer(ctx, clients, opts, plan.MCP, &result, logf); err != nil { return result, err } + installSkill(ctx, clients, plan.Skill, &result, logf) return result, nil } @@ -1241,7 +1307,7 @@ func registerMCPServer( logf("Reused MCP server %s (%s) with its current token", step.name(), existing) logf("Pass --access-token and --replace-mcp-token to change the token") - return associateMCPServer(ctx, clients, step.name(), result, logf) + return associateMCPServer(ctx, clients, opts, step, step.name(), result, logf) } // A run after a failure finds an unconnected server by this name. name := MCPServerNameFor(result.AgentSpaceID) @@ -1330,7 +1396,7 @@ func registerMCPServer( } } - return associateMCPServer(ctx, clients, name, result, logf) + return associateMCPServer(ctx, clients, opts, step, name, result, logf) } // leaveSharedServer disconnects a shared server from this space only, after @@ -1482,17 +1548,20 @@ func disassociateEverywhere( func associateMCPServer( ctx context.Context, clients Clients, + opts SetupOptions, + step MCPStep, name string, result *SetupResult, logf func(string, ...any), ) error { - existing, err := FindAssociation(ctx, clients, result.AgentSpaceID, result.MCPServiceID) + existing, err := findMCPAssociation(ctx, clients, result.AgentSpaceID, result.MCPServiceID) if err != nil { return err } - if existing != "" { - result.MCPAssociationID = existing + if existing != nil { + result.MCPAssociationID = aws.ToString(existing.AssociationId) logf("MCP server %s is already associated with the agent space", name) + extendAllowlist(ctx, clients, opts, step, *existing, result, logf) return nil } @@ -1507,6 +1576,93 @@ func associateMCPServer( return nil } +func findMCPAssociation(ctx context.Context, clients Clients, agentSpaceID, serviceID string) (*agenttypes.Association, error) { + associations, err := listAssociations(ctx, clients, agentSpaceID) + if err != nil { + return nil, err + } + for _, association := range associations { + if aws.ToString(association.ServiceId) == serviceID { + return &association, nil + } + } + + return nil, nil +} + +// missingTools returns the ldcli tools that the allowlist of an association +// misses, and whether setup can add them. AWS replaces the whole configuration +// on an update and does not send the classification of each tool back, so +// setup can rewrite the allowlist only if it holds ldcli tools alone. An +// unclassified tool counts as read-only, so a rewrite would let a mutative +// tool that the customer added run without approval. +func missingTools(association agenttypes.Association) (missing []string, canAdd bool) { + if _, ok := association.Configuration.(*agenttypes.ServiceConfigurationMemberMcpserver); !ok { + // Not an allowlist that setup knows how to read, so leave it. + return nil, false + } + allowed := associationTools(association.Configuration) + ldcli := slices.Concat(mcpReadOnlyTools, mcpMutativeTools) + for _, tool := range ldcli { + if !slices.Contains(allowed, tool) { + missing = append(missing, tool) + } + } + canAdd = !slices.ContainsFunc(allowed, func(tool string) bool { return !slices.Contains(ldcli, tool) }) + + return missing, canAdd +} + +// extendAllowlist adds the ldcli tools that an existing association misses, +// for example after an ldcli upgrade that allows more tools. Setup cannot tell +// an older ldcli allowlist from one that someone trimmed, so it adds tools only +// with --add-mcp-tools. A failure does not stop setup, because the server works +// with the tools that it has. +func extendAllowlist( + ctx context.Context, + clients Clients, + opts SetupOptions, + step MCPStep, + association agenttypes.Association, + result *SetupResult, + logf func(string, ...any), +) { + missing, canAdd := missingTools(association) + tools := strings.Join(missing, ", ") + switch { + case len(missing) == 0: + return + case !canAdd: + result.MCPToolsError = fmt.Sprintf("the MCP server allowlist of the agent space misses %s, and it has tools "+ + "that ldcli does not classify, so setup did not change it. Add the missing tools in the AWS console", tools) + + return + case !opts.AddMCPTools: + result.MCPToolsError = fmt.Sprintf("the MCP server allowlist of the agent space misses %s, which the "+ + "experiment-orchestration skill uses. To add them, run setup again with --add-mcp-tools", tools) + + return + case !slices.Equal(missing, step.MissingTools) || canAdd != step.CanAddTools: + // The plan showed another change, so do not make this one. + result.MCPToolsError = "the MCP server allowlist changed after the plan, so setup did not change it. Run setup again" + + return + } + _, err := clients.Agent.UpdateAssociation(ctx, &devopsagent.UpdateAssociationInput{ + AgentSpaceId: association.AgentSpaceId, + AssociationId: association.AssociationId, + Configuration: &agenttypes.ServiceConfigurationMemberMcpserver{Value: mcpServerConfiguration()}, + Capabilities: association.Capabilities, + }) + if err != nil { + result.MCPToolsError = fmt.Sprintf("unable to add %s to the MCP server allowlist: %v", strings.Join(missing, ", "), err) + + return + } + result.MCPToolsAdded = missing + logf("Added %s to the MCP server allowlist", strings.Join(missing, ", ")) +} + func associateMCPServerWith(ctx context.Context, clients Clients, agentSpaceID, serviceID string) (string, error) { association, err := clients.Agent.AssociateService(ctx, &devopsagent.AssociateServiceInput{ AgentSpaceId: aws.String(agentSpaceID), diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go new file mode 100644 index 00000000..959e1a06 --- /dev/null +++ b/internal/awsdevops/skill.go @@ -0,0 +1,362 @@ +package awsdevops + +import ( + "context" + "crypto/sha256" + _ "embed" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "slices" + "strings" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/devopsagent" + "github.com/aws/aws-sdk-go-v2/service/devopsagent/document" + agenttypes "github.com/aws/aws-sdk-go-v2/service/devopsagent/types" +) + +// ExperimentSkill is the SKILL.md of the experimentation skill. Its front +// matter gives the name and the description that setup sends as metadata. +// +//go:embed skills/experiment-orchestration/SKILL.md +var ExperimentSkill string + +var ( + ExperimentSkillName = frontMatterField(ExperimentSkill, "name") + experimentSkillDescription = frontMatterField(ExperimentSkill, "description") +) + +// shippedSkillHashes holds the SHA-256 of each SKILL.md that an ldcli release +// sent. A skill with one of these texts has no customer edits, so setup +// updates it without --replace-skill. When you change SKILL.md, add the hash +// of the new text. Never remove a hash. +var shippedSkillHashes = []string{ + "57844bc3c5aca292ef88b5f949cd791206074b5cfc850ea52ebdd40020ead695", + "c4c4e12aeff900402dc17ff370bc1e78eadcb34abac5b256319c7cd793bd73f3", +} + +const ( + skillAssetType = "skill" + skillFilePath = "SKILL.md" +) + +// frontMatterField reads one key from the front matter of a SKILL.md. +func frontMatterField(text, key string) string { + lines := strings.Split(text, "\n") + if len(lines) == 0 || strings.TrimSpace(lines[0]) != "---" { + return "" + } + for _, line := range lines[1:] { + if strings.TrimSpace(line) == "---" { + return "" + } + if value, ok := strings.CutPrefix(line, key+":"); ok { + return strings.Trim(strings.TrimSpace(value), `"`) + } + } + + return "" +} + +func skillHash(text string) string { + sum := sha256.Sum256([]byte(text)) + + return hex.EncodeToString(sum[:]) +} + +// SkillAction is the planned skill action. +type SkillAction int + +const ( + SkillCreate SkillAction = iota + SkillKeep + // SkillUpdate replaces a SKILL.md that an older ldcli sent, or one that + // is empty. + SkillUpdate + // SkillLeave means that someone edited the SKILL.md, or that the skill has + // none, so setup does not change it without --replace-skill. + SkillLeave + SkillReplace + // SkillSkip means that setup could not read the skills of the space. The + // skill is optional, so the run continues without it. + SkillSkip +) + +// SkillStep is the planned experimentation skill step. +type SkillStep struct { + Action SkillAction + AssetID string + // Version is the asset version at plan time. A write stops if it changed. + Version int32 + // NoFile means that the skill has no SKILL.md, for example a skill that + // someone made from a PDF. + NoFile bool + // Err says why the step is skipped. + Err error +} + +func (s SkillStep) describe() string { + label := fmt.Sprintf("%s (%s)", ExperimentSkillName, s.AssetID) + why := "someone edited it" + if s.NoFile { + why = "it has no " + skillFilePath + } + switch s.Action { + case SkillKeep: + return "Keep skill " + label + case SkillUpdate: + return fmt.Sprintf("Update skill %s to the instructions of this ldcli version", label) + case SkillLeave: + return fmt.Sprintf("Leave skill %s unchanged because %s. Pass --replace-skill to overwrite it", label, why) + case SkillReplace: + return fmt.Sprintf("Overwrite skill %s, which is not the ldcli version because %s", label, why) + case SkillSkip: + return skillFailure(fmt.Errorf("Skip skill %s: %w", ExperimentSkillName, s.Err)) + default: + return fmt.Sprintf("Create skill %s for every agent type in the space", ExperimentSkillName) + } +} + +// skillFailure is the text of a failed skill step. A missing permission is +// the most likely cause, so it names the permissions that the step needs. +func skillFailure(err error) string { + if errorCode(err) != "AccessDeniedException" { + return err.Error() + } + + return fmt.Sprintf("%v. The skill step needs aidevops:ListAssets, aidevops:GetAssetFile, "+ + "aidevops:CreateAsset, and aidevops:UpdateAsset", err) +} + +func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space SpaceStep) SkillStep { + if space.Action == SpaceCreate { + return SkillStep{Action: SkillCreate} + } + asset, err := findSkill(ctx, clients, space.ID, ExperimentSkillName) + if err != nil { + return SkillStep{Action: SkillSkip, Err: err} + } + if asset == nil { + return SkillStep{Action: SkillCreate} + } + step := SkillStep{AssetID: aws.ToString(asset.AssetId), Version: aws.ToInt32(asset.Version)} + text, found, err := skillText(ctx, clients, space.ID, step.AssetID) + if err == nil && !found { + // AWS also sends ResourceNotFoundException for an asset that someone + // deleted after the list. A deleted skill gets a new one. + asset, err = findSkill(ctx, clients, space.ID, ExperimentSkillName) + switch { + case err != nil: + return SkillStep{Action: SkillSkip, AssetID: step.AssetID, Err: err} + case asset == nil: + return SkillStep{Action: SkillCreate} + case aws.ToString(asset.AssetId) != step.AssetID: + return SkillStep{Action: SkillSkip, Err: planChanged("skill %s changed during the plan", ExperimentSkillName)} + } + } + step.NoFile = !found + switch { + case err != nil: + return SkillStep{Action: SkillSkip, AssetID: step.AssetID, Err: err} + case text == ExperimentSkill: + step.Action = SkillKeep + case found && (text == "" || slices.Contains(shippedSkillHashes, skillHash(text))): + // An empty file has nothing to lose. + step.Action = SkillUpdate + case opts.ReplaceSkill: + step.Action = SkillReplace + default: + step.Action = SkillLeave + } + + return step +} + +// findSkill returns the skill with this name in the space, or nil if there is +// none. AWS keeps skill names unique in a space. +func findSkill(ctx context.Context, clients Clients, spaceID, name string) (*agenttypes.Asset, error) { + assets, err := listAssets(ctx, clients, spaceID, skillAssetType) + if err != nil { + return nil, err + } + for _, asset := range assets { + if assetName(asset) == name { + return &asset, nil + } + } + + return nil, nil +} + +// assetName reads the name from the metadata of an asset. It decodes the JSON +// of the document, because UnmarshalSmithyDocument fails on a document from +// NewLazyDocument. +func assetName(asset agenttypes.Asset) string { + if asset.Metadata == nil { + return "" + } + encoded, err := asset.Metadata.MarshalSmithyDocument() + if err != nil { + return "" + } + var metadata struct { + Name string `json:"name"` + } + if err := json.Unmarshal(encoded, &metadata); err != nil { + return "" + } + + return metadata.Name +} + +// skillText returns the SKILL.md of the skill. found is false if the skill +// has no SKILL.md. +func skillText(ctx context.Context, clients Clients, spaceID, assetID string) (text string, found bool, err error) { + file, err := clients.Agent.GetAssetFile(ctx, &devopsagent.GetAssetFileInput{ + AgentSpaceId: aws.String(spaceID), + AssetId: aws.String(assetID), + Path: aws.String(skillFilePath), + }) + var missing *agenttypes.ResourceNotFoundException + switch { + case errors.As(err, &missing): + return "", false, nil + case err != nil: + // The caller names the skill. + return "", false, fmt.Errorf("unable to read %s of asset %s: %w", skillFilePath, assetID, err) + case file.File == nil: + return "", false, nil + } + switch body := file.File.Content.(type) { + case *agenttypes.AssetFileBodyMemberText: + return body.Value, true, nil + case *agenttypes.AssetFileBodyMemberBytes: + return string(body.Value), true, nil + } + + return "", false, nil +} + +// checkSkill compares the space with a step that writes. A change to the +// version means that someone edited the skill after the plan. +func checkSkill(ctx context.Context, clients Clients, spaceID string, step SkillStep) error { + asset, err := findSkill(ctx, clients, spaceID, ExperimentSkillName) + switch { + case err != nil: + return err + case step.Action == SkillCreate && asset != nil: + return planChanged("skill %s appeared after the plan", ExperimentSkillName) + case step.Action == SkillCreate: + return nil + case asset == nil || aws.ToString(asset.AssetId) != step.AssetID: + return planChanged("skill %s disappeared after the plan", ExperimentSkillName) + case aws.ToInt32(asset.Version) != step.Version: + return planChanged("skill %s changed after the plan", ExperimentSkillName) + } + + return nil +} + +// The values of SetupResult.SkillAction. +const ( + SkillCreated = "created" + SkillKept = "kept" + SkillUpdated = "updated" + SkillLeft = "left" + SkillOverwritten = "overwritten" + SkillSkipped = "skipped" + SkillFailed = "failed" +) + +// installSkill does the skill step. The skill is optional, so a failure does +// not stop setup. The failure goes in result.SkillError, and the caller +// reports it. +func installSkill( + ctx context.Context, + clients Clients, + step SkillStep, + result *SetupResult, + logf func(string, ...any), +) { + result.SkillAssetID = step.AssetID + action, err := doSkillStep(ctx, clients, step, result, logf) + result.SkillAction = action + if err != nil { + result.SkillAction = SkillFailed + // A plan change stops the step before the write, so setup tried + // nothing. + if step.Action == SkillSkip || errors.Is(err, ErrPlanChanged) { + result.SkillAction = SkillSkipped + } + result.SkillError = skillFailure(err) + } +} + +func doSkillStep( + ctx context.Context, + clients Clients, + step SkillStep, + result *SetupResult, + logf func(string, ...any), +) (string, error) { + switch step.Action { + case SkillKeep: + logf("Kept skill %s (%s)", ExperimentSkillName, step.AssetID) + + return SkillKept, nil + case SkillLeave: + logf("Left skill %s (%s) unchanged. Pass --replace-skill to overwrite it", ExperimentSkillName, step.AssetID) + + return SkillLeft, nil + case SkillSkip: + return "", fmt.Errorf("setup skipped skill %s: %w", ExperimentSkillName, step.Err) + } + + // The other steps can take minutes, so check the skill right before the + // write. + if err := checkSkill(ctx, clients, result.AgentSpaceID, step); err != nil { + return "", err + } + content := &agenttypes.AssetContentMemberFile{Value: agenttypes.AssetFileContent{ + Path: aws.String(skillFilePath), + Body: &agenttypes.AssetFileBodyMemberText{Value: ExperimentSkill}, + }} + if step.Action == SkillCreate { + created, err := clients.Agent.CreateAsset(ctx, &devopsagent.CreateAssetInput{ + AgentSpaceId: aws.String(result.AgentSpaceID), + AssetType: aws.String(skillAssetType), + Content: content, + Metadata: document.NewLazyDocument(map[string]any{ + "name": ExperimentSkillName, + "description": experimentSkillDescription, + "agent_types": []string{"GENERIC"}, + }), + }) + if err != nil { + return "", fmt.Errorf("unable to create skill %s: %w", ExperimentSkillName, err) + } + result.SkillAssetID = aws.ToString(created.Asset.AssetId) + logf("Created skill %s (%s)", ExperimentSkillName, result.SkillAssetID) + + return SkillCreated, nil + } + + // AWS updates only the metadata keys in the request, so the status and + // the agent types that the customer set stay. + if _, err := clients.Agent.UpdateAsset(ctx, &devopsagent.UpdateAssetInput{ + AgentSpaceId: aws.String(result.AgentSpaceID), + AssetId: aws.String(step.AssetID), + Content: content, + Metadata: document.NewLazyDocument(map[string]any{"description": experimentSkillDescription}), + }); err != nil { + return "", fmt.Errorf("unable to update skill %s (%s): %w", ExperimentSkillName, step.AssetID, err) + } + logf("Updated skill %s (%s)", ExperimentSkillName, step.AssetID) + if step.Action == SkillReplace { + return SkillOverwritten, nil + } + + return SkillUpdated, nil +} diff --git a/internal/awsdevops/skill_test.go b/internal/awsdevops/skill_test.go new file mode 100644 index 00000000..f79cbf33 --- /dev/null +++ b/internal/awsdevops/skill_test.go @@ -0,0 +1,393 @@ +package awsdevops_test + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/devopsagent/document" + agenttypes "github.com/aws/aws-sdk-go-v2/service/devopsagent/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/launchdarkly/ldcli/internal/awsdevops" +) + +func reuseOptions() awsdevops.SetupOptions { + return awsdevops.SetupOptions{AgentSpaceID: "space-2", AuthFlow: "iam", LDAccessToken: "api-token"} +} + +func metadataOf(t *testing.T, doc document.Interface) map[string]any { + t.Helper() + encoded, err := doc.MarshalSmithyDocument() + require.NoError(t, err) + var metadata map[string]any + require.NoError(t, json.Unmarshal(encoded, &metadata)) + + return metadata +} + +func TestExperimentSkillFrontMatter(t *testing.T) { + assert.Equal(t, "experiment-orchestration", awsdevops.ExperimentSkillName) + assert.Contains(t, awsdevops.ExperimentSkill, "# Automated Experimentation") +} + +// A change to SKILL.md must add its hash, so the next release updates the +// copies that this release sends. +func TestShippedSkillHashesHoldTheCurrentSkill(t *testing.T) { + assert.Contains(t, awsdevops.ShippedSkillHashes(), awsdevops.SkillHash(awsdevops.ExperimentSkill), + "add the hash of the new SKILL.md to shippedSkillHashes") +} + +func TestSetupCreatesSkillInNewSpace(t *testing.T) { + agent := &fakeAgent{} + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), awsdevops.SetupOptions{ + AgentSpaceName: "launchdarkly", + AuthFlow: "iam", + LDAccessToken: "api-token", + }) + require.NoError(t, err) + + require.Len(t, agent.createAssets, 1) + in := agent.createAssets[0] + assert.Equal(t, "space-1", aws.ToString(in.AgentSpaceId)) + assert.Equal(t, "skill", aws.ToString(in.AssetType)) + metadata := metadataOf(t, in.Metadata) + assert.Equal(t, awsdevops.ExperimentSkillName, metadata["name"]) + assert.Contains(t, metadata["description"], "Orchestrates automated experimentation lifecycle") + assert.Equal(t, []any{"GENERIC"}, metadata["agent_types"]) + assert.NotContains(t, metadata, "status") + file, ok := in.Content.(*agenttypes.AssetContentMemberFile) + require.True(t, ok) + assert.Equal(t, "SKILL.md", aws.ToString(file.Value.Path)) + assert.Equal(t, &agenttypes.AssetFileBodyMemberText{Value: awsdevops.ExperimentSkill}, file.Value.Body) + assert.Equal(t, "ki-created", result.SkillAssetID) +} + +// AWS adds sample skills to a space. Only the name of our skill counts. +func TestPlanSetupFindsSkillOnLaterPage(t *testing.T) { + agent := &fakeAgent{ + paginate: true, + assets: []agenttypes.Asset{skillAsset("ki-sample", "sample-skip-scheduled-maintenance"), skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": awsdevops.ExperimentSkill}, + } + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.SkillStep{Action: awsdevops.SkillKeep, AssetID: "ki-ours"}, plan.Skill) + assert.Equal(t, "Keep skill experiment-orchestration (ki-ours)", plan.Text().Skill) +} + +func TestPlanSetupCreatesMissingSkill(t *testing.T) { + agent := &fakeAgent{assets: []agenttypes.Asset{skillAsset("ki-sample", "sample-skip-scheduled-maintenance")}} + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.SkillCreate, plan.Skill.Action) + assert.Equal(t, "Create skill experiment-orchestration for every agent type in the space", plan.Text().Skill) +} + +func TestSetupKeepsSameSkill(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": awsdevops.ExperimentSkill}, + } + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + require.NoError(t, err) + + assert.Empty(t, agent.createAssets) + assert.Empty(t, agent.updateAssets) + assert.Equal(t, "ki-ours", result.SkillAssetID) + assert.Equal(t, awsdevops.SkillKept, result.SkillAction) + assert.Empty(t, result.SkillError) +} + +// AWS can send the file as bytes. The text is the same, so setup keeps it. +func TestPlanSetupKeepsSameSkillSentAsBytes(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillBytes: map[string][]byte{"ki-ours": []byte(awsdevops.ExperimentSkill)}, + } + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.SkillKeep, plan.Skill.Action) +} + +// An empty SKILL.md has no edits to lose, so setup updates it without the flag. +func TestPlanSetupUpdatesEmptySkill(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": ""}, + } + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.SkillUpdate, plan.Skill.Action) +} + +// A skill without SKILL.md, for example one made from a PDF, is not ours. +func TestPlanSetupLeavesSkillWithoutFile(t *testing.T) { + agent := &fakeAgent{assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}} + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.SkillLeave, plan.Skill.Action) + assert.Equal(t, + "Leave skill experiment-orchestration (ki-ours) unchanged because it has no SKILL.md. Pass --replace-skill to overwrite it", + plan.Text().Skill) +} + +// A copy from an older ldcli has no edits, so setup updates it without the flag. +func TestSetupUpdatesOlderShippedSkill(t *testing.T) { + t.Cleanup(awsdevops.ShipSkill("# Instructions of an older ldcli")) + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": "# Instructions of an older ldcli"}, + } + clients := newTestClients(agent, newFakeIAM()) + + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), true) + require.NoError(t, err) + assert.Equal(t, awsdevops.SkillUpdate, plan.Skill.Action) + assert.Contains(t, plan.Text().Skill, "Update skill experiment-orchestration (ki-ours)") + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + require.NoError(t, err) + require.Len(t, agent.updateAssets, 1) + assert.Equal(t, "ki-ours", aws.ToString(agent.updateAssets[0].AssetId)) + assert.Equal(t, awsdevops.SkillUpdated, result.SkillAction) +} + +func TestSetupLeavesEditedSkill(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": "# Edited by the customer"}, + } + clients := newTestClients(agent, newFakeIAM()) + + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), true) + require.NoError(t, err) + assert.Equal(t, awsdevops.SkillLeave, plan.Skill.Action) + assert.Contains(t, plan.Text().Skill, "--replace-skill") + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + require.NoError(t, err) + assert.Empty(t, agent.createAssets) + assert.Empty(t, agent.updateAssets) + // JSON output shows that the skill is not the ldcli version. + assert.Equal(t, awsdevops.SkillLeft, result.SkillAction) + assert.Empty(t, result.SkillError) +} + +func TestSetupOverwritesEditedSkillWithFlag(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": "# Edited by the customer"}, + } + opts := reuseOptions() + opts.ReplaceSkill = true + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), opts) + require.NoError(t, err) + + assert.Empty(t, agent.createAssets) + assert.Equal(t, awsdevops.SkillOverwritten, result.SkillAction) + require.Len(t, agent.updateAssets, 1) + in := agent.updateAssets[0] + assert.Equal(t, "ki-ours", aws.ToString(in.AssetId)) + // Only the description, so the status and agent types that the + // customer set stay. + assert.Equal(t, []string{"description"}, keysOf(metadataOf(t, in.Metadata))) + file, ok := in.Content.(*agenttypes.AssetContentMemberFile) + require.True(t, ok) + assert.Equal(t, &agenttypes.AssetFileBodyMemberText{Value: awsdevops.ExperimentSkill}, file.Value.Body) +} + +// The flag does nothing to a skill that is already the same. +func TestPlanSetupKeepsSameSkillWithFlag(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": awsdevops.ExperimentSkill}, + } + opts := reuseOptions() + opts.ReplaceSkill = true + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), opts, true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.SkillKeep, plan.Skill.Action) +} + +func TestConnectCreatesSkill(t *testing.T) { + agent := &fakeAgent{} + + result, err := awsdevops.ConnectExisting(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + require.NoError(t, err) + + require.Len(t, agent.createAssets, 1) + assert.Equal(t, "space-2", aws.ToString(agent.createAssets[0].AgentSpaceId)) + assert.Equal(t, "ki-created", result.SkillAssetID) + assert.Equal(t, awsdevops.SkillCreated, result.SkillAction) +} + +// The skill step goes after the MCP server, and a plan change in it only +// skips the skill. +func TestRunPlanSkipsSkillThatAppears(t *testing.T) { + agent := &fakeAgent{} + clients := newTestClients(agent, newFakeIAM()) + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), true) + require.NoError(t, err) + agent.assets = []agenttypes.Asset{skillAsset("ki-other", "experiment-orchestration")} + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + + require.NoError(t, err) + assert.Empty(t, agent.createAssets) + assert.Contains(t, agent.calls, "RegisterService") + assert.Contains(t, result.SkillError, "appeared after the plan") + assert.Equal(t, awsdevops.SkillSkipped, result.SkillAction) +} + +func TestRunPlanSkipsSkillThatIsGone(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": "# Edited by the customer"}, + } + clients := newTestClients(agent, newFakeIAM()) + opts := reuseOptions() + opts.ReplaceSkill = true + plan, err := awsdevops.PlanSetup(context.Background(), clients, opts, true) + require.NoError(t, err) + agent.assets = nil + + result, err := awsdevops.RunPlan(context.Background(), clients, opts, plan) + + require.NoError(t, err) + assert.Empty(t, agent.updateAssets) + assert.Contains(t, result.SkillError, "disappeared after the plan") +} + +// An edit after the plan changes the version. The overwrite stops, so it +// cannot remove an edit that the plan did not show. +func TestRunPlanSkipsSkillEditedAfterPlan(t *testing.T) { + asset := skillAsset("ki-ours", "experiment-orchestration") + asset.Version = aws.Int32(3) + agent := &fakeAgent{ + assets: []agenttypes.Asset{asset}, + skillFiles: map[string]string{"ki-ours": "# Edited by the customer"}, + } + clients := newTestClients(agent, newFakeIAM()) + opts := reuseOptions() + opts.ReplaceSkill = true + plan, err := awsdevops.PlanSetup(context.Background(), clients, opts, true) + require.NoError(t, err) + require.Equal(t, int32(3), plan.Skill.Version) + agent.assets[0].Version = aws.Int32(4) + + result, err := awsdevops.RunPlan(context.Background(), clients, opts, plan) + + require.NoError(t, err) + assert.Empty(t, agent.updateAssets) + assert.Contains(t, result.SkillError, "changed after the plan") + // The step stopped before the write. + assert.Equal(t, awsdevops.SkillSkipped, result.SkillAction) +} + +func accessDenied(action string) error { + return &agenttypes.AccessDeniedException{ + Message: aws.String("User: arn:aws:sts::123456789012:assumed-role/Dev/me is not authorized to perform: " + action), + } +} + +// Without aidevops:ListAssets, the plan skips the skill and setup does the +// other steps. The AWS error has a code, and the text keeps its message, +// which names the denied action. +func TestSetupSkipsSkillWhenAssetsCannotBeListed(t *testing.T) { + agent := &fakeAgent{listAssetsErr: accessDenied("aidevops:ListAssets")} + clients := newTestClients(agent, newFakeIAM()) + + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), true) + require.NoError(t, err) + assert.Equal(t, awsdevops.SkillSkip, plan.Skill.Action) + assert.True(t, strings.HasPrefix(plan.Text().Skill, "Skip skill experiment-orchestration: "), plan.Text().Skill) + assert.Contains(t, plan.Text().Skill, "not authorized to perform: aidevops:ListAssets") + assert.Contains(t, plan.Text().Skill, "The skill step needs") + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + require.NoError(t, err) + assert.Contains(t, agent.calls, "RegisterService") + assert.Equal(t, awsdevops.SkillSkipped, result.SkillAction) + assert.Contains(t, result.SkillError, "not authorized to perform: aidevops:ListAssets") + assert.Equal(t, 1, strings.Count(result.SkillError, awsdevops.ExperimentSkillName)) +} + +func TestSetupSkipsSkillWhenFileCannotBeRead(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + getFileErr: accessDenied("aidevops:GetAssetFile"), + } + + result, err := awsdevops.ConnectExisting(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + + require.NoError(t, err) + assert.Equal(t, awsdevops.SkillSkipped, result.SkillAction) + assert.Contains(t, result.SkillError, "not authorized to perform: aidevops:GetAssetFile") + assert.Contains(t, result.SkillError, "ki-ours") + assert.Equal(t, 1, strings.Count(result.SkillError, awsdevops.ExperimentSkillName)) +} + +// AWS sends ResourceNotFoundException also for a skill that someone deleted +// after the list. That skill does not exist, so the plan creates one. +func TestPlanSetupCreatesSkillDeletedDuringPlan(t *testing.T) { + agent := &fakeAgent{assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}} + agent.onGetFile = func() { agent.assets = nil } + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.SkillStep{Action: awsdevops.SkillCreate}, plan.Skill) +} + +func TestSetupWarnsOnSkillCreateFailure(t *testing.T) { + agent := &fakeAgent{createAssetErr: accessDenied("aidevops:CreateAsset")} + + result, err := awsdevops.ConnectExisting(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + + require.NoError(t, err) + assert.Contains(t, agent.calls, "RegisterService") + assert.Equal(t, "mcp-1", result.MCPServiceID) + assert.Equal(t, awsdevops.SkillFailed, result.SkillAction) + assert.Contains(t, result.SkillError, "unable to create skill experiment-orchestration") + assert.Contains(t, result.SkillError, "not authorized to perform: aidevops:CreateAsset") + assert.Empty(t, result.SkillAssetID) +} + +// Other failures do not get the permission text. +func TestSkillFailureNamesPermissionsOnlyWhenDenied(t *testing.T) { + agent := &fakeAgent{createAssetErr: &agenttypes.ServiceQuotaExceededException{Message: aws.String("200 skills")}} + + result, err := awsdevops.ConnectExisting(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + + require.NoError(t, err) + assert.Contains(t, result.SkillError, "200 skills") + assert.NotContains(t, result.SkillError, "The skill step needs") +} + +func keysOf(m map[string]any) []string { + keys := make([]string, 0, len(m)) + for key := range m { + keys = append(keys, key) + } + + return keys +} diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md new file mode 100644 index 00000000..bf816eb5 --- /dev/null +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -0,0 +1,171 @@ +--- +name: "experiment-orchestration" +description: "Orchestrates automated experimentation lifecycle using LaunchDarkly Guarded Rollouts, an AI coding agent for implementation, and GitHub Actions for deployment." +--- + +# Automated Experimentation + +Use this skill when you have a goal you want to move through experimentation (e.g., "increase checkout conversion by 15%", "decrease page load time by 20%"). + +**Core principle: experiment first, then guarded rollout.** Always prove a change on a small, fixed slice of traffic via an A/B experiment before ramping it up through a guarded rollout. Never start a guarded rollout blind — it exists only to scale a change the experiment has already shown to work. + +**Execution mode:** once the goal is confirmed (Step 1), run Steps 2–8 end-to-end. Async operations (code implementation, release review, deployment, experiment monitoring, rollout monitoring) should be checked periodically, not tight-polled — see the waiting note in each step. Only stop and ask the user something if a step fails unrecoverably (repeated failed release reviews, deployment failure, or an inconclusive/losing experiment result). + +**Approvals:** if the LaunchDarkly environment requires approval for a change (for example, to turn a flag on or off, start or stop an experiment, or start a guarded rollout), create an approval request instead of the direct change. Tell the person who must review it. Continue only after `list-approval-requests` shows it approved, then apply it with `apply-approval-request` and go to the next step. If a reviewer rejects it, stop and report. + +**The final report (Step 8) is mandatory, not optional.** The moment an experiment or rollout reaches a terminal outcome — winner, loser, inconclusive, or rollback — produce the full report in the same turn you announce the outcome. Don't let a casual "it worked!" substitute for the structured report. + +## Step 1: Goal Clarification + +Before doing anything, get answers to: + +1. **What metric measures success?** *(Required)* e.g. conversion rate, page load time, bounce rate. Reserve your error-rate metric as a safety guardrail — never use it as the primary success metric. +2. **What's the target improvement?** *(Required)* e.g. 15% increase, 200ms decrease. +3. **What's the current baseline?** *(Optional — infer from production metrics if not given)* +4. **What parts of the app are in scope?** *(Optional — infer from the codebase if not given)* +5. **Any constraints?** *(Optional)* e.g. no changes to the payment flow. + +Questions 1–2 are required before proceeding; infer 3–5 where possible and confirm your assumptions with the user before implementing. + +## Step 2: Hypothesis Generation + +Explore the target repository/codebase to find a plausible change: + +1. Search and read the relevant code paths. +2. Think through what UI/UX or logic change could plausibly move the chosen metric. +3. Check whether this hypothesis (or something close to it) has already been tried and failed — look at flag history or archived flags with similar naming. Avoid repeating a known failure. +4. Present the hypothesis to the user before proceeding, along with your reasoning and any inferred assumptions from Step 1. + +**Before finalizing a flag key, check for collisions:** look up any candidate flag key first. +- Already fully shipped (100% one variation, no split) → already decided, pick a different hypothesis. +- Actively running an experiment → mid-flight, don't compete with it, pick a different hypothesis. +- Doesn't exist → safe to create. + +## Step 3: Implementation + +1. Create a boolean feature flag, OFF by default in all environments, with `defaults` of `{"onVariation": 1, "offVariation": 1}` so that it serves false even when it is on, and `clientSideAvailability` of `{"usingEnvironmentId": true, "usingMobileKey": true}` so that every SDK can read it. Name it with a clear pattern like `exp--` (e.g. `exp-checkout-conversion-cta-color`), lowercase with hyphens, ~50 chars max. If the project has its own flag naming conventions (for example, a required prefix or case), follow them instead. +2. Hand off implementation to your coding agent/tool of choice, with clear instructions to gate the change behind the exact flag key from step 1. +3. This step is asynchronous — check status periodically rather than looping tightly on it. +4. Once implementation completes, move to Step 4 with the resulting branch/PR. If it fails, report the error and stop. + +## Step 4: Release Readiness + +Run your standard release/risk review on the PR before merging. + +- If it passes: merge the PR. +- If it fails: feed the review's specific feedback back into implementation and retry. Cap retries at a small fixed number (e.g. 3 attempts total) — if it still hasn't passed, stop and report the last failure to the user rather than retrying indefinitely. + +*(If your environment genuinely has no review capability available — e.g., a fully unattended automation context — you can skip straight to merge, but treat that as a deliberate, narrow exception you call out explicitly, not a default. Skipping review removes your only gate against shipping broken code.)* + +## Step 5: Deployment + +Deployment typically won't fire automatically on merge if your workflow is manually-triggered (`workflow_dispatch`-only) — you'll need to trigger it explicitly. + +1. Trigger the deploy workflow on the merge target branch. Treat "already an in-progress deployment for this ref" as expected de-duplication, not an error — don't re-trigger. +2. Poll for status, but let your polling tool's own internal long-poll do the waiting rather than looping tightly yourself. +3. Watch for a "stale" status specifically: if a deployment reports "running" for far longer than normal, cross-check the actual CI run history by commit SHA/timing before assuming it's still in progress — a background poll process may have died without updating the record. +4. **Trigger a deployment at most once per attempt.** If you're unsure whether a previous trigger succeeded, check status first — never re-trigger just because you're unsure. +5. On timeout: stop, check the CI run directly, report the situation, ask how to proceed. +6. On explicit failure: stop and report — do not proceed to the experiment. +7. On success: proceed immediately to Step 6. + +## Step 6: Experiment Phase (fixed 10%) + +Prove the change on a small, fixed slice of traffic. Do **not** start a guarded rollout here — that's Step 7, and only after this proves out. + +1. Turn the flag ON. +2. Configure a fixed 50/50 split across 10% of traffic (a flat allocation, not a staged ramp) on your chosen randomization unit (typically "user"). The remaining 90% of traffic is excluded from the experiment entirely. +3. Create an experiment with: + - Exactly one primary metric: the success metric from Step 1. + - Guardrail metric(s): always include your error-rate metric; add a performance metric (e.g. p95 page load time) too if this is a performance-focused change. + - Treatments: control (off) at 50%, treatment (on) at 50%, allocated to 10% of total traffic. +4. Start the experiment/data collection. +5. Move to Step 7 to monitor toward a decision. + +## Step 7: Monitoring & Outcome + +Check status periodically — don't tight-loop. In an interactive session, check once and report progress, then pick back up later. In an unattended/scheduled context, check once per invocation and persist your progress somewhere durable between runs. + +**Phase 1 — Prove the experiment at 10% (gate before any rollout):** + +Watch for statistical significance on the primary metric: + +- **Significant + positive lift** → experiment proven. Stop the experiment iteration with control as the winner, then move to Phase 2. Stopping serves the winner to everyone, so a treatment winner would skip the guarded rollout. +- **Significant + negative lift** → declare a loser, stop the iteration with control as the winner, turn the flag off (do not archive it while the code still evaluates it), skip Phase 2, go straight to the Step 8 report. +- **No significance after a reasonable ceiling (e.g. 4 weeks)** → report "inconclusive, need more traffic", stop the iteration with control as the winner, and stop; don't proceed to Phase 2. + +Never declare a winner off a single data point or before your stats engine confirms significance. + +**Phase 2 — Guarded rollout ramp (only after Phase 1 proves the change):** + +Start a guarded rollout with: +- The winning ("on") variation as the test, the original as control. +- Same randomization unit as the experiment. +- **Exactly 3 monitored stages, capped well below 100%** — e.g. 20% → 30% → 40%, ~60 minutes monitoring each. Don't add a stage at or above 100%; Guarded-rollout implementations reject stages above 50% audience allocation, and the rollout auto-promotes to 100% itself once the final monitored stage completes cleanly — no explicit 100% stage needed. +- The same primary + guardrail metrics as the experiment, each configured to notify and auto-rollback on regression. + +Track stage progression. If the rollout rolls back or stops at any point, treat it as a regression: declare failed, turn the flag off (do not archive it while the code still evaluates it), and go to the Step 8 report. + +Once the final stage completes cleanly and auto-promotes to 100%, declare a winner and go to the Step 8 report. + +**Retrying after a rollback:** a rollback isn't always caused by your monitored metrics genuinely regressing — it can also be triggered by an unrelated application error surfacing mid-ramp. Before blindly restarting after the user says they've fixed something: +1. Confirm the flag's current state (should be back to 100% control, nothing stuck mid-rollout). +2. Check the change history timing between "advanced to next stage" and "reverted." A rollback within seconds of advancing is inconsistent with a full metric-window regression and points to an external cause instead. +3. If the flag is cleanly reverted and the external cause is confirmed fixed, it's safe to restart the guarded rollout from scratch with the same parameters. +4. Don't silently retry without this check, and don't refuse to retry just because a prior attempt rolled back — a genuinely fixed external cause is a legitimate reason to retry. A metric-driven loser is not — don't retry that. + +**On any terminal outcome, immediately produce the Step 8 report in the same turn** — a one-line "it worked!" note is fine as a lead-in, but the structured report must follow, not wait for a follow-up request. + +## Step 8: Report + +Runs automatically the instant Step 7 reaches a terminal outcome (winner + auto-promoted to 100%; loser; inconclusive; or rollback/failure). Use this exact structure from Experiment Report to Next Steps: + +```markdown +## Experiment Report: [Goal Description] + +**Date:** [YYYY-MM-DD] +**Goal:** [metric] [direction] by [target]% +**Status:** [achieved / in progress / stalled] + +### Hypothesis +[What we tried and why] + +### Implementation +- Flag: [flag_key] +- Files modified: [list] +- Branch: [branch name] + +### Release Readiness +- [reviewed, passed after N attempt(s) / skipped, per your environment's process] + +### Experiment Phase (10% fixed split) +- Status: [proven / loser / inconclusive] +- Duration: [time] +- Metric change: [before] → [after] ([+/-]%) +- Statistical significance: [value, confidence interval] + +### Guarded Rollout Phase (if reached) +- Status: [completed / rolled_back / not started] +- Duration: [time] +- Stages reached: [N of 3 monitored stages] +- If rolled back and retried: [root cause, outcome of retry] + +### Safety Metrics +- error-rate: [baseline] → [final] ([no regression / regression detected]) +- [other guardrails]: [baseline] → [final] ([status]) + +### Next Steps +[What to do next based on the outcome] +``` + +## Safety Rules (the non-negotiables) + +- Always present the hypothesis before implementing. +- Always run a release/risk review before merging, unless your environment has a deliberate, explicitly-called-out exception. +- **Always prove a change via a fixed small-percentage experiment before starting any guarded rollout** — never ramp blind. +- Always include an error-rate (or equivalent "don't break prod") metric as a guardrail, separate from your success metric. +- Add a performance guardrail (e.g. p95 latency) for performance-focused changes. +- Every rollout metric should be configured to both notify AND auto-rollback on regression — don't rely on notification alone. +- **Cap guarded rollout stages well below 100%** (most platforms reject stages ≥50% audience allocation) and let the platform auto-promote to 100% after the final stage — don't try to add an explicit 100% stage. +- Distinguish a metric-driven rollback (don't retry) from an external-cause rollback (safe to retry once fixed) before restarting a rolled-back rollout. +- The final report is automatic and mandatory on every terminal outcome — never defer it to a follow-up ask.