diff --git a/cmd/aws_devops_agent/confirm_test.go b/cmd/aws_devops_agent/confirm_test.go index 14abe2b4..b37f956a 100644 --- a/cmd/aws_devops_agent/confirm_test.go +++ b/cmd/aws_devops_agent/confirm_test.go @@ -129,13 +129,24 @@ func countingClients(spaces ...agenttypes.AgentSpace) (awsdevops.Clients, *write w := &writes{} return awsdevops.Clients{ - Agent: countingAgent{stubAgent: stubAgent{spaces: spaces}, writes: w}, - IAM: countingIAM{writes: w}, - STS: accountSTS{}, - Region: "eu-west-2", + Agent: countingAgent{stubAgent: stubAgent{spaces: spaces}, writes: w}, + IAM: countingIAM{writes: w}, + STS: accountSTS{}, + Sources: stubSources{skill: "---\nname: experiment-orchestration\ndescription: Run experiments.\n---\n"}, + Region: "eu-west-2", }, w } +type stubSources struct{ skill string } + +func (s stubSources) File(_ context.Context, path string) (string, error) { + if strings.HasSuffix(path, "SKILL.md") { + return s.skill, nil + } + + return "# Experiment orchestrator\n", nil +} + // noReadStdin fails the test on any read. type noReadStdin struct{ t *testing.T } diff --git a/cmd/aws_devops_agent/plan_print_test.go b/cmd/aws_devops_agent/plan_print_test.go index 1fbe0ae4..d723c7af 100644 --- a/cmd/aws_devops_agent/plan_print_test.go +++ b/cmd/aws_devops_agent/plan_print_test.go @@ -66,6 +66,7 @@ func TestSetupPlanNamesRolesAndOperatorApp(t *testing.T) { assert.Contains(t, out.String(), awsdevops.OperatorAppRoleName) assert.Contains(t, out.String(), "operator app") assert.Contains(t, out.String(), "Skill: Create skill "+awsdevops.ExperimentSkillName) + assert.Contains(t, out.String(), "Custom agent: Create custom agent "+awsdevops.ExperimentAgentName) } // A canceled plan does not create the skill. diff --git a/cmd/aws_devops_agent/setup.go b/cmd/aws_devops_agent/setup.go index 0720bb69..26e9e3e2 100644 --- a/cmd/aws_devops_agent/setup.go +++ b/cmd/aws_devops_agent/setup.go @@ -57,15 +57,24 @@ who approved access, in every agent space. If the agent space already has an MCP server, setup keeps it. 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. +tools that this ldcli version allows, setup adds them. AWS does not send back +the classification of each tool, so the rewrite gives every tool the ldcli +classification. + +Setup downloads the skill and the custom agent prompt from the main branch of +launchdarkly/ai-tooling on GitHub, so those steps need access to GitHub. If +the download fails, setup skips both steps and warns. 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 +space already has the skill and it differs from the latest version, setup +skips it. To update it to the latest version, pass --replace-skill. Setting +up the skill is optional: if its step fails, setup warns and does the other +steps. + +The experiment-orchestrator custom agent runs the skill with the LaunchDarkly +MCP tools that the space allows. It needs the MCP server and the skill. +Setting up the custom agent is optional: if its step fails, setup warns and does the other steps.`, Args: cobra.NoArgs, PreRun: trackRun(analyticsTrackerFn), @@ -86,9 +95,9 @@ does the other steps.`, cmd.Flags().String(idpClientSecretFlag, "", "OIDC client secret, required when --auth-flow=idp") cmd.Flags().String(mcpServiceIDFlag, "", "Registered MCP server to connect, instead of the server of the AWS account") 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(replaceSkillFlag, false, "Overwrite the "+awsdevops.ExperimentSkillName+" skill if it differs from the latest version") + cmd.Flags().Bool(addMCPToolsFlag, false, "Has no effect. Setup adds the missing tools without it") + _ = cmd.Flags().MarkDeprecated(addMCPToolsFlag, "setup now adds the missing MCP tools without it") cmd.Flags().Bool(yesFlag, false, "Ask no questions and accept every step") @@ -178,6 +187,9 @@ func setupWith( if result.SkillError != "" { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", result.SkillError) } + if result.AgentError != "" { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", result.AgentError) + } return printJSON(cmd, result) } @@ -188,6 +200,9 @@ func setupWith( if result.SkillError != "" { _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nWarning: %s\n", result.SkillError) } + if result.AgentError != "" { + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nWarning: %s\n", result.AgentError) + } // 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. @@ -513,7 +528,8 @@ func confirmSetup(cmd *cobra.Command, plan awsdevops.SetupPlan, ask bool) error _, _ = fmt.Fprintf(out, " Association: %s\n", text.Association) _, _ = fmt.Fprintf(out, " Operator app: %s\n", text.OperatorApp) _, _ = fmt.Fprintf(out, " MCP server: %s\n", text.MCPServer) - _, _ = fmt.Fprintf(out, " Skill: %s\n\n", text.Skill) + _, _ = fmt.Fprintf(out, " Skill: %s\n", text.Skill) + _, _ = fmt.Fprintf(out, " Custom agent: %s\n\n", text.Agent) if !ask { return nil } @@ -641,7 +657,6 @@ func setupOptions(cmd *cobra.Command) (awsdevops.SetupOptions, error) { MCPEndpoint: mustString(cmd, mcpEndpointFlag), OpenURL: browser.OpenURL, ReplaceSkill: mustBool(cmd, replaceSkillFlag), - AddMCPTools: mustBool(cmd, addMCPToolsFlag), } idcSet := opts.IdcInstanceARN != "" diff --git a/internal/awsdevops/agent.go b/internal/awsdevops/agent.go new file mode 100644 index 00000000..1171fef7 --- /dev/null +++ b/internal/awsdevops/agent.go @@ -0,0 +1,327 @@ +package awsdevops + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "slices" + + "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" +) + +const ExperimentAgentName = "experiment-orchestrator" + +const agentResetHint = "To use the latest version, delete the agent in the AWS console and run setup again" + +const ( + agentAssetType = "custom_agent" + agentFilePath = ExperimentAgentName + ".md" +) + +// AWS names a tool of an MCP server "_". +func agentTools(serverName string) []string { + return toolIDs(serverName, slices.Concat(mcpReadOnlyTools, mcpMutativeTools)) +} + +func toolIDs(serverName string, tools []string) []string { + ids := make([]string, 0, len(tools)) + for _, tool := range tools { + ids = append(ids, serverName+"_"+tool) + } + + return ids +} + +// spaceAgentTools are the ldcli tools that the association of the server +// allows, because an agent can use no others. +func spaceAgentTools(ctx context.Context, clients Clients, spaceID, serviceID, serverName string) ([]string, error) { + if spaceID == "" || serviceID == "" { + return agentTools(serverName), nil + } + tools := slices.Concat(mcpReadOnlyTools, mcpMutativeTools) + associations, err := listAssociations(ctx, clients, spaceID) + if err != nil { + return nil, err + } + for _, association := range associations { + if aws.ToString(association.ServiceId) != serviceID { + continue + } + config, ok := association.Configuration.(*agenttypes.ServiceConfigurationMemberMcpserver) + if !ok { + break + } + tools = slices.DeleteFunc(tools, func(tool string) bool { return !slices.Contains(config.Value.Tools, tool) }) + } + + return toolIDs(serverName, tools), nil +} + +type AgentAction int + +const ( + AgentCreate AgentAction = iota + AgentKeep + AgentUpdate + AgentLeave + AgentSkip +) + +type AgentStep struct { + Action AgentAction + AssetID string + // Version is the asset version at plan time. A write stops if it changed. + Version int32 + ServerName string + // Tools and Skills are what the existing agent has. + Tools []string + Skills []string + NoFile bool + // Prompt is the prompt downloaded at plan time. + Prompt string + Err error +} + +func (s AgentStep) describe() string { + label := fmt.Sprintf("%s (%s)", ExperimentAgentName, s.AssetID) + why := "its prompt differs from the latest version on the main branch of " + aiToolingRepo + if s.NoFile { + why = "it has no " + agentFilePath + } + switch s.Action { + case AgentKeep: + return "Keep custom agent " + label + case AgentUpdate: + return fmt.Sprintf("Add the tools of this ldcli version and the skill to custom agent %s", label) + case AgentLeave: + return fmt.Sprintf("Skip custom agent %s because %s. %s", label, why, agentResetHint) + case AgentSkip: + return agentFailure(fmt.Errorf("Skip custom agent %s: %w", ExperimentAgentName, s.Err)) + default: + return fmt.Sprintf("Create custom agent %s with skill %s and the MCP tools of server %s", + ExperimentAgentName, ExperimentSkillName, s.ServerName) + } +} + +func agentFailure(err error) string { + if errorCode(err) != "AccessDeniedException" { + return err.Error() + } + + return fmt.Sprintf("%v. The custom agent step needs aidevops:ListAssets, aidevops:GetAssetFile, "+ + "aidevops:CreateAsset, aidevops:UpdateAsset, and aidevops:ListAssociations", err) +} + +func planAgent( + ctx context.Context, + clients Clients, + space SpaceStep, + mcp MCPStep, + skill SkillStep, + prompt string, + fetchErr error, +) AgentStep { + step := AgentStep{ServerName: mcp.name(), Prompt: prompt} + if fetchErr != nil { + step.Action, step.Err = AgentSkip, fetchErr + return step + } + if space.Action == SpaceCreate { + return step + } + asset, err := findAsset(ctx, clients, space.ID, agentAssetType, ExperimentAgentName) + if err != nil { + step.Action, step.Err = AgentSkip, err + return step + } + if asset == nil { + return step + } + step.AssetID, step.Version = aws.ToString(asset.AssetId), aws.ToInt32(asset.Version) + step.Tools, step.Skills = agentMetadata(*asset) + text, found, err := assetText(ctx, clients, space.ID, step.AssetID, agentFilePath) + step.NoFile = !found + if err != nil { + step.Action, step.Err = AgentSkip, err + return step + } + wantTools, err := spaceAgentTools(ctx, clients, space.ID, mcp.ServiceID, step.ServerName) + if mcp.CanAddTools { + // The MCP step adds these before the agent step runs. + wantTools = union(wantTools, toolIDs(step.ServerName, mcp.MissingTools)) + } + current := found && text == prompt + switch { + case err != nil: + step.Action, step.Err = AgentSkip, err + case current && skill.AssetID != "" && slices.Contains(step.Skills, skill.AssetID) && + !slices.ContainsFunc(wantTools, func(tool string) bool { return !slices.Contains(step.Tools, tool) }): + step.Action = AgentKeep + case current: + step.Action = AgentUpdate + default: + step.Action = AgentLeave + } + + return step +} + +func agentMetadata(asset agenttypes.Asset) (tools, skills []string) { + if asset.Metadata == nil { + return nil, nil + } + encoded, err := asset.Metadata.MarshalSmithyDocument() + if err != nil { + return nil, nil + } + var metadata struct { + Tools []string `json:"tools"` + Skills []string `json:"skills"` + } + if err := json.Unmarshal(encoded, &metadata); err != nil { + return nil, nil + } + + return metadata.Tools, metadata.Skills +} + +func union(a, b []string) []string { + out := slices.Clone(a) + for _, item := range b { + if !slices.Contains(out, item) { + out = append(out, item) + } + } + + return out +} + +func checkAgent(ctx context.Context, clients Clients, spaceID string, step AgentStep) error { + asset, err := findAsset(ctx, clients, spaceID, agentAssetType, ExperimentAgentName) + switch { + case err != nil: + return err + case step.Action == AgentCreate && asset != nil: + return planChanged("custom agent %s appeared after the plan", ExperimentAgentName) + case step.Action == AgentCreate: + return nil + case asset == nil || aws.ToString(asset.AssetId) != step.AssetID: + return planChanged("custom agent %s disappeared after the plan", ExperimentAgentName) + case aws.ToInt32(asset.Version) != step.Version: + return planChanged("custom agent %s changed after the plan", ExperimentAgentName) + } + + return nil +} + +// The values of SetupResult.AgentAction. +const ( + AgentCreated = "created" + AgentKept = "kept" + AgentUpdated = "updated" + AgentLeft = "left" + AgentSkipped = "skipped" + AgentFailed = "failed" +) + +// installAgent runs after the MCP server and skill steps, because the agent +// needs both. A failure does not stop setup. +func installAgent( + ctx context.Context, + clients Clients, + step AgentStep, + result *SetupResult, + logf func(string, ...any), +) { + result.AgentAssetID = step.AssetID + action, err := doAgentStep(ctx, clients, step, result, logf) + result.AgentAction = action + if err != nil { + result.AgentAction = AgentFailed + if step.Action == AgentSkip || errors.Is(err, ErrPlanChanged) || errors.Is(err, errAgentNeeds) { + result.AgentAction = AgentSkipped + } + result.AgentError = agentFailure(err) + } +} + +var errAgentNeeds = errors.New("the custom agent needs the skill") + +func doAgentStep( + ctx context.Context, + clients Clients, + step AgentStep, + result *SetupResult, + logf func(string, ...any), +) (string, error) { + switch step.Action { + case AgentKeep: + logf("Kept custom agent %s (%s)", ExperimentAgentName, step.AssetID) + + return AgentKept, nil + case AgentLeave: + logf("Skipped custom agent %s (%s). %s", ExperimentAgentName, step.AssetID, agentResetHint) + + return AgentLeft, nil + case AgentSkip: + return "", fmt.Errorf("setup skipped custom agent %s: %w", ExperimentAgentName, step.Err) + } + if result.SkillAssetID == "" || result.SkillAction == SkillSkipped { + return "", fmt.Errorf("setup skipped custom agent %s, because skill %s is not ready: %w", + ExperimentAgentName, ExperimentSkillName, errAgentNeeds) + } + tools, err := spaceAgentTools(ctx, clients, result.AgentSpaceID, result.MCPServiceID, result.mcpServerName) + if err != nil { + return "", err + } + if len(tools) == 0 { + return "", fmt.Errorf("setup skipped custom agent %s, because the space allows none of the ldcli MCP tools: %w", + ExperimentAgentName, errAgentNeeds) + } + + if err := checkAgent(ctx, clients, result.AgentSpaceID, step); err != nil { + return "", err + } + content := &agenttypes.AssetContentMemberFile{Value: agenttypes.AssetFileContent{ + Path: aws.String(agentFilePath), + Body: &agenttypes.AssetFileBodyMemberText{Value: step.Prompt}, + }} + skills := []string{result.SkillAssetID} + if step.Action == AgentUpdate { + tools, skills = union(step.Tools, tools), union(step.Skills, skills) + } + metadata := map[string]any{"tools": tools, "skills": skills} + if step.Action == AgentCreate { + metadata["name"] = ExperimentAgentName + created, err := clients.Agent.CreateAsset(ctx, &devopsagent.CreateAssetInput{ + AgentSpaceId: aws.String(result.AgentSpaceID), + AssetType: aws.String(agentAssetType), + Content: content, + Metadata: document.NewLazyDocument(metadata), + }) + if err != nil { + return "", fmt.Errorf("unable to create custom agent %s: %w", ExperimentAgentName, err) + } + result.AgentAssetID = aws.ToString(created.Asset.AssetId) + logf("Created custom agent %s (%s)", ExperimentAgentName, result.AgentAssetID) + + return AgentCreated, nil + } + + // AWS keeps the metadata keys that the request leaves out. + if _, err := clients.Agent.UpdateAsset(ctx, &devopsagent.UpdateAssetInput{ + AgentSpaceId: aws.String(result.AgentSpaceID), + AssetId: aws.String(step.AssetID), + Content: content, + Metadata: document.NewLazyDocument(metadata), + }); err != nil { + return "", fmt.Errorf("unable to update custom agent %s (%s): %w", ExperimentAgentName, step.AssetID, err) + } + logf("Updated custom agent %s (%s)", ExperimentAgentName, step.AssetID) + + return AgentUpdated, nil +} diff --git a/internal/awsdevops/agent_test.go b/internal/awsdevops/agent_test.go new file mode 100644 index 00000000..cd5a3757 --- /dev/null +++ b/internal/awsdevops/agent_test.go @@ -0,0 +1,265 @@ +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" +) + +func TestAgentToolsStartWithTheServerName(t *testing.T) { + tools := awsdevops.AgentTools("LaunchDarkly") + + assert.Contains(t, tools, "LaunchDarkly_get-flag") + assert.Contains(t, tools, "LaunchDarkly_toggle-flag") + for _, tool := range tools { + assert.Regexp(t, `^LaunchDarkly_[a-z-]+$`, tool) + } +} + +func TestSetupCreatesAgentInNewSpace(t *testing.T) { + agent := &fakeAgent{} + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), awsdevops.SetupOptions{ + AgentSpaceName: "launchdarkly", + AuthFlow: "iam", + }) + require.NoError(t, err) + + require.Len(t, agent.createAgents, 1) + in := agent.createAgents[0] + assert.Equal(t, "space-1", aws.ToString(in.AgentSpaceId)) + metadata := metadataOf(t, in.Metadata) + assert.Equal(t, awsdevops.ExperimentAgentName, metadata["name"]) + assert.Equal(t, []any{"ki-created"}, metadata["skills"]) + assert.Contains(t, metadata["tools"], awsdevops.MCPOAuthServerName+"_get-flag") + assert.NotContains(t, metadata, "memory_stores") + file, ok := in.Content.(*agenttypes.AssetContentMemberFile) + require.True(t, ok) + assert.Equal(t, "experiment-orchestrator.md", aws.ToString(file.Value.Path)) + assert.Equal(t, &agenttypes.AssetFileBodyMemberText{Value: testAgentPrompt}, file.Value.Body) + assert.Equal(t, "ki-agent-created", result.AgentAssetID) + assert.Equal(t, awsdevops.AgentCreated, result.AgentAction) + assert.Empty(t, result.AgentError) +} + +func TestPlanSetupNamesAgentToolsAfterTheExistingServer(t *testing.T) { + server := oauthMCPService("mcp-1") + server.Name = aws.String("Prod-LD") + server.AdditionalServiceDetails.(*agenttypes.AdditionalServiceDetailsMemberMcpserver).Value.Name = aws.String("Prod-LD") + agent := &fakeAgent{services: []agenttypes.RegisteredService{server}} + clients := newTestClients(agent, newFakeIAM()) + opts := reuseOptions() + + plan, err := awsdevops.PlanSetup(context.Background(), clients, opts, true) + require.NoError(t, err) + + require.Equal(t, awsdevops.MCPReuse, plan.MCP.Action) + assert.Equal(t, "Prod-LD", plan.Agent.ServerName) + assert.Equal(t, "Create custom agent experiment-orchestrator with skill experiment-orchestration "+ + "and the MCP tools of server Prod-LD", plan.Text().Agent) + + result, err := awsdevops.RunPlan(context.Background(), clients, opts, plan) + require.NoError(t, err) + require.Len(t, agent.createAgents, 1, result.AgentError) + assert.Contains(t, metadataOf(t, agent.createAgents[0].Metadata)["tools"], "Prod-LD_get-flag") +} + +func TestSetupKeepsCurrentAgent(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{ + skillAsset("ki-ours", "experiment-orchestration"), + agentAsset("ki-agent", "experiment-orchestrator", awsdevops.AgentTools(awsdevops.MCPOAuthServerName), []string{"ki-ours"}), + }, + skillFiles: map[string]string{"ki-ours": testSkill}, + agentFiles: map[string]string{"ki-agent": testAgentPrompt}, + } + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + require.NoError(t, err) + + assert.Empty(t, agent.createAgents) + assert.Empty(t, agent.updateAgents) + assert.Equal(t, "ki-agent", result.AgentAssetID) + assert.Equal(t, awsdevops.AgentKept, result.AgentAction) +} + +func TestSetupUpdatesAgentWithOldTools(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{ + skillAsset("ki-ours", "experiment-orchestration"), + agentAsset("ki-agent", "experiment-orchestrator", []string{awsdevops.MCPOAuthServerName + "_get-flag"}, []string{"ki-ours"}), + }, + skillFiles: map[string]string{"ki-ours": testSkill}, + agentFiles: map[string]string{"ki-agent": testAgentPrompt}, + } + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + require.NoError(t, err) + + require.Len(t, agent.updateAgents, 1) + metadata := metadataOf(t, agent.updateAgents[0].Metadata) + assert.ElementsMatch(t, awsdevops.AgentTools(awsdevops.MCPOAuthServerName), metadata["tools"]) + assert.Equal(t, []any{"ki-ours"}, metadata["skills"]) + assert.NotContains(t, metadata, "memory_stores") + assert.Equal(t, awsdevops.AgentUpdated, result.AgentAction) +} + +func TestSetupSkipsDifferentAgent(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{agentAsset("ki-agent", "experiment-orchestrator", nil, nil)}, + agentFiles: map[string]string{"ki-agent": "# 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.AgentLeave, plan.Agent.Action) + assert.Contains(t, plan.Text().Agent, "delete the agent in the AWS console") + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + require.NoError(t, err) + assert.Empty(t, agent.updateAgents) + assert.Equal(t, awsdevops.AgentLeft, result.AgentAction) + assert.Empty(t, result.AgentError) +} + +func TestSetupSkipsAgentWhenSkillFails(t *testing.T) { + agent := &fakeAgent{createAssetErr: errors.New("AccessDeniedException: not allowed")} + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + require.NoError(t, err) + + assert.Empty(t, agent.createAgents) + assert.Equal(t, awsdevops.SkillFailed, result.SkillAction) + assert.Equal(t, awsdevops.AgentSkipped, result.AgentAction) + assert.Contains(t, result.AgentError, "skill experiment-orchestration is not ready") +} + +func TestSetupWarnsWhenAgentCreateFails(t *testing.T) { + agent := &fakeAgent{createAgentErr: accessDenied("aidevops:CreateAsset")} + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + require.NoError(t, err) + + assert.Equal(t, awsdevops.SkillCreated, result.SkillAction) + assert.Equal(t, awsdevops.AgentFailed, result.AgentAction) + assert.Contains(t, result.AgentError, "The custom agent step needs aidevops:ListAssets") +} + +func TestPlanSetupSkipsAgentWhenAssetsCannotBeListed(t *testing.T) { + agent := &fakeAgent{listAssetsErr: accessDenied("aidevops:ListAssets")} + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.AgentSkip, plan.Agent.Action) + assert.Contains(t, plan.Text().Agent, "The custom agent step needs aidevops:ListAssets") +} + +func TestPlanSetupKeepsAgentWithAddedTools(t *testing.T) { + tools := append(awsdevops.AgentTools(awsdevops.MCPOAuthServerName), "Datadog_query-logs") + agent := &fakeAgent{ + assets: []agenttypes.Asset{ + skillAsset("ki-ours", "experiment-orchestration"), + agentAsset("ki-agent", "experiment-orchestrator", tools, []string{"ki-other", "ki-ours"}), + }, + skillFiles: map[string]string{"ki-ours": testSkill}, + agentFiles: map[string]string{"ki-agent": testAgentPrompt}, + } + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.AgentKeep, plan.Agent.Action) +} + +func TestSetupUpdateKeepsAddedToolsAndSkills(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{ + agentAsset("ki-agent", "experiment-orchestrator", []string{"Datadog_query-logs"}, []string{"ki-other"}), + }, + agentFiles: map[string]string{"ki-agent": testAgentPrompt}, + } + + _, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + require.NoError(t, err) + + require.Len(t, agent.updateAgents, 1) + metadata := metadataOf(t, agent.updateAgents[0].Metadata) + assert.Contains(t, metadata["tools"], "Datadog_query-logs") + assert.Contains(t, metadata["tools"], awsdevops.MCPOAuthServerName+"_get-flag") + assert.Equal(t, []any{"ki-other", "ki-created"}, metadata["skills"]) +} + +func TestSetupGivesAgentOnlyTheToolsTheSpaceAllows(t *testing.T) { + agent := &fakeAgent{ + services: []agenttypes.RegisteredService{launchDarklyMCPService("mcp-1")}, + associations: []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: []string{"get-flag", "not-an-ldcli-tool"}}, + }, + }}, + } + opts := reuseOptions() + + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), opts) + require.NoError(t, err) + + require.Len(t, agent.createAgents, 1, result.AgentError) + assert.Equal(t, []any{awsdevops.MCPServerName + "_get-flag"}, metadataOf(t, agent.createAgents[0].Metadata)["tools"]) +} + +func TestPlanSetupLeavesAgentWithoutPromptFile(t *testing.T) { + agent := &fakeAgent{assets: []agenttypes.Asset{agentAsset("ki-agent", "experiment-orchestrator", nil, nil)}} + + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + require.NoError(t, err) + + assert.Equal(t, awsdevops.AgentLeave, plan.Agent.Action) + assert.Contains(t, plan.Text().Agent, "because it has no experiment-orchestrator.md") +} + +func TestPlanSetupNamesAgentToolsAfterANewServer(t *testing.T) { + plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(&fakeAgent{}, newFakeIAM()), awsdevops.SetupOptions{ + AgentSpaceName: "launchdarkly", + AuthFlow: "iam", + }, true) + require.NoError(t, err) + + assert.Contains(t, plan.Text().Agent, "the MCP tools of server "+awsdevops.MCPOAuthServerName) +} + +func TestSetupGivesAgentTheToolsThatSetupAddsToTheAllowlist(t *testing.T) { + old := awsdevops.AgentTools(awsdevops.MCPServerName)[:1] + agent := &fakeAgent{ + services: []agenttypes.RegisteredService{launchDarklyMCPService("mcp-1")}, + associations: []agenttypes.Association{mcpAssociation("list-projects")}, + assets: []agenttypes.Asset{ + skillAsset("ki-ours", "experiment-orchestration"), + agentAsset("ki-agent", "experiment-orchestrator", old, []string{"ki-ours"}), + }, + skillFiles: map[string]string{"ki-ours": testSkill}, + agentFiles: map[string]string{"ki-agent": testAgentPrompt}, + } + clients := newTestClients(agent, newFakeIAM()) + opts := reuseOptions() + + plan, err := awsdevops.PlanSetup(context.Background(), clients, opts, true) + require.NoError(t, err) + require.Equal(t, awsdevops.AgentUpdate, plan.Agent.Action) + + result, err := awsdevops.RunPlan(context.Background(), clients, opts, plan) + require.NoError(t, err) + require.Len(t, agent.updateAgents, 1, result.AgentError) + assert.ElementsMatch(t, awsdevops.AgentTools(awsdevops.MCPServerName), metadataOf(t, agent.updateAgents[0].Metadata)["tools"]) +} diff --git a/internal/awsdevops/allowlist_test.go b/internal/awsdevops/allowlist_test.go index caff3566..faaa9328 100644 --- a/internal/awsdevops/allowlist_test.go +++ b/internal/awsdevops/allowlist_test.go @@ -27,13 +27,6 @@ func mcpAssociation(tools ...string) agenttypes.Association { } } -func reuseServerOptions() awsdevops.SetupOptions { - opts := reuseOptions() - 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) { @@ -43,13 +36,13 @@ func TestSetupAddsMissingToolsToAnExistingAllowlist(t *testing.T) { } clients := newTestClients(agent, newFakeIAM()) - plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseServerOptions(), true) + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), 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) + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) require.NoError(t, err) require.Len(t, agent.updateAssociations, 1) @@ -75,7 +68,7 @@ func TestSetupKeepsACompleteAllowlist(t *testing.T) { associations: []agenttypes.Association{mcpAssociation(awsdevops.MCPTools()...)}, } - result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseServerOptions()) + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) require.NoError(t, err) assert.Empty(t, agent.updateAssociations) @@ -92,39 +85,17 @@ func TestSetupLeavesAnAllowlistWithForeignTools(t *testing.T) { } clients := newTestClients(agent, newFakeIAM()) - plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseServerOptions(), true) + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), true) require.NoError(t, err) assert.Contains(t, plan.Text().MCPServer, "which setup cannot add") - result, err := awsdevops.RunPlan(context.Background(), clients, reuseServerOptions(), plan) + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), 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{ @@ -133,11 +104,11 @@ func TestSetupSkipsAnAllowlistThatChangedAfterThePlan(t *testing.T) { } clients := newTestClients(agent, newFakeIAM()) - plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseServerOptions(), true) + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), true) require.NoError(t, err) agent.associations = []agenttypes.Association{mcpAssociation("get-flag")} - result, err := awsdevops.RunPlan(context.Background(), clients, reuseServerOptions(), plan) + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) require.NoError(t, err) assert.Empty(t, agent.updateAssociations) @@ -153,7 +124,7 @@ func TestSetupWarnsWhenTheAllowlistUpdateFails(t *testing.T) { updateAssociationErr: errors.New("AccessDeniedException: not allowed"), } - result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseServerOptions()) + result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) require.NoError(t, err) assert.Contains(t, result.MCPToolsError, "unable to add") diff --git a/internal/awsdevops/awsdevops.go b/internal/awsdevops/awsdevops.go index 848504ac..ac93d91b 100644 --- a/internal/awsdevops/awsdevops.go +++ b/internal/awsdevops/awsdevops.go @@ -85,10 +85,11 @@ type STSAPI interface { // Clients bundles the AWS clients and the resolved session details the // provisioning steps run against. type Clients struct { - Agent AgentAPI - IAM IAMAPI - STS STSAPI - Region string + Agent AgentAPI + IAM IAMAPI + STS STSAPI + Sources Sources + Region string } // NewClients builds AWS clients from the ambient credential chain: environment @@ -130,10 +131,11 @@ func NewClients(ctx context.Context, region, profile string) (Clients, error) { } return Clients{ - Agent: devopsagent.NewFromConfig(cfg), - IAM: iam.NewFromConfig(cfg), - STS: sts.NewFromConfig(cfg), - Region: cfg.Region, + Agent: devopsagent.NewFromConfig(cfg), + IAM: iam.NewFromConfig(cfg), + STS: sts.NewFromConfig(cfg), + Sources: NewGitHubSources(), + Region: cfg.Region, }, nil } diff --git a/internal/awsdevops/awsdevops_test.go b/internal/awsdevops/awsdevops_test.go index 80ef0e5c..b58cc233 100644 --- a/internal/awsdevops/awsdevops_test.go +++ b/internal/awsdevops/awsdevops_test.go @@ -62,9 +62,9 @@ type fakeAgent struct { failAssociateFor map[string]bool // 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 + paginate bool + assets []agenttypes.Asset + agentFiles map[string]string // 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. @@ -79,6 +79,17 @@ type fakeAgent struct { updateAssociations []*devopsagent.UpdateAssociationInput updateAssociationErr error updateAssets []*devopsagent.UpdateAssetInput + createAgents []*devopsagent.CreateAssetInput + updateAgents []*devopsagent.UpdateAssetInput + createAgentErr error +} + +func agentAsset(id, name string, tools, skills []string) agenttypes.Asset { + return agenttypes.Asset{ + AssetId: aws.String(id), + AssetType: aws.String("custom_agent"), + Metadata: document.NewLazyDocument(map[string]any{"name": name, "tools": tools, "skills": skills}), + } } // skillAsset is a skill asset named name. @@ -92,11 +103,20 @@ func skillAsset(id, name string) agenttypes.Asset { 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 + id := "ki-created" + if aws.ToString(in.AssetType) == "custom_agent" { + f.createAgents = append(f.createAgents, in) + if f.createAgentErr != nil { + return nil, f.createAgentErr + } + id = "ki-agent-created" + } else { + 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} + asset := agenttypes.Asset{AssetId: aws.String(id), AssetType: in.AssetType, Metadata: in.Metadata} f.assets = append(f.assets, asset) return &devopsagent.CreateAssetOutput{Asset: &asset}, nil @@ -110,6 +130,12 @@ func (f *fakeAgent) GetAssetFile(_ context.Context, in *devopsagent.GetAssetFile if f.getFileErr != nil { return nil, f.getFileErr } + if prompt, ok := f.agentFiles[aws.ToString(in.AssetId)]; ok && aws.ToString(in.Path) == "experiment-orchestrator.md" { + return &devopsagent.GetAssetFileOutput{File: &agenttypes.AssetFile{ + Path: in.Path, + Content: &agenttypes.AssetFileBodyMemberText{Value: prompt}, + }}, nil + } if aws.ToString(in.Path) != "SKILL.md" { return nil, &agenttypes.ResourceNotFoundException{} } @@ -135,13 +161,26 @@ func (f *fakeAgent) ListAssets(_ context.Context, in *devopsagent.ListAssetsInpu if f.listAssetsErr != nil { return nil, f.listAssetsErr } - items, next := page(f.assets, in.NextToken, f.paginate) + var assets []agenttypes.Asset + for _, asset := range f.assets { + if in.AssetType == nil || aws.ToString(asset.AssetType) == aws.ToString(in.AssetType) { + assets = append(assets, asset) + } + } + items, next := page(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") + for _, asset := range f.assets { + if aws.ToString(asset.AssetId) == aws.ToString(in.AssetId) && aws.ToString(asset.AssetType) == "custom_agent" { + f.updateAgents = append(f.updateAgents, in) + + return &devopsagent.UpdateAssetOutput{Asset: &agenttypes.Asset{AssetId: in.AssetId}}, nil + } + } f.updateAssets = append(f.updateAssets, in) return &devopsagent.UpdateAssetOutput{Asset: &agenttypes.Asset{AssetId: in.AssetId}}, nil @@ -304,6 +343,11 @@ func (f *fakeAgent) UpdateAssociation(_ context.Context, in *devopsagent.UpdateA if f.updateAssociationErr != nil { return nil, f.updateAssociationErr } + for i, association := range f.associations { + if aws.ToString(association.AssociationId) == aws.ToString(in.AssociationId) { + f.associations[i].Configuration = in.Configuration + } + } return &devopsagent.UpdateAssociationOutput{}, nil } @@ -318,6 +362,12 @@ func (f *fakeAgent) RegisterService(_ context.Context, in *devopsagent.RegisterS return f.registerOutput, nil } + name := aws.ToString(in.Name) + if details, ok := in.ServiceDetails.(*agenttypes.ServiceDetailsMemberMcpserver); ok && details.Value.Name != nil { + name = aws.ToString(details.Value.Name) + } + f.services = append(f.services, mcpService("mcp-1", name, awsdevops.MCPServerEndpoint)) + return &devopsagent.RegisterServiceOutput{ServiceId: aws.String("mcp-1")}, nil } @@ -428,10 +478,11 @@ func (f fakeSTS) GetCallerIdentity(_ context.Context, _ *sts.GetCallerIdentityIn func newTestClients(agent *fakeAgent, iamClient *fakeIAM) awsdevops.Clients { return awsdevops.Clients{ - Agent: agent, - IAM: iamClient, - STS: fakeSTS{account: testAccountID}, - Region: "us-east-1", + Agent: agent, + IAM: iamClient, + STS: fakeSTS{account: testAccountID}, + Sources: newFakeSources(), + Region: "us-east-1", } } @@ -461,10 +512,11 @@ func TestSetupCreatesRolesAgentSpaceAndAssociations(t *testing.T) { // The plan and the first check look only for the server of the // account, because a new space has no server. The check before the // MCP step also looks for the server of the space, which exists by - // then. The skill is last. + // then. The skill and the custom agent are last. []string{ "ListServices", "ListServices", "CreateAgentSpace", "AssociateService", "EnableOperatorApp", - "ListServices", "ListServices", "RegisterService", "AssociateService", "ListAssets", "CreateAsset", + "ListServices", "ListServices", "RegisterService", "AssociateService", + "ListAssets", "CreateAsset", "ListAssets", "CreateAsset", }, agent.calls, ) @@ -858,14 +910,14 @@ func TestMCPAssociationClassifiesTheSkillTools(t *testing.T) { assert.Contains(t, classes, tool) } for _, tool := range []string{ - "toggle-flag", "create-flag", "update-rollout", "archive-flag", "create-experiment", + "toggle-flag", "create-flag", "update-off-variation", "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) } - for _, tool := range []string{"get-flag", "list-experiments", "get-experiment-results", "query-change-history"} { + for _, tool := range []string{"get-flag", "list-experiments", "get-experiment-results", "query-change-history", "list-metric-events"} { assert.Equal(t, agenttypes.ToolClassificationReadOnly, classes[tool], tool) } } diff --git a/internal/awsdevops/export_test.go b/internal/awsdevops/export_test.go index 789550ee..a21de5b5 100644 --- a/internal/awsdevops/export_test.go +++ b/internal/awsdevops/export_test.go @@ -25,19 +25,10 @@ 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) } +const ( + SkillSourcePath = skillSourcePath + AgentSourcePath = agentSourcePath +) // MCPTools lists every tool in the allowlist, for tests. func MCPTools() []string { @@ -52,3 +43,5 @@ func SetOAuthPollInterval(d time.Duration) func() { return func() { oauthPollInterval = old } } + +func AgentTools(serverName string) []string { return agentTools(serverName) } diff --git a/internal/awsdevops/setup.go b/internal/awsdevops/setup.go index d19fbd5c..58870314 100644 --- a/internal/awsdevops/setup.go +++ b/internal/awsdevops/setup.go @@ -65,9 +65,7 @@ var oauthPollInterval = 5 * time.Second // 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. 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. +// skill creates a missing primary metric, so the lists include 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, then apply // the request after a reviewer approves it. @@ -83,10 +81,14 @@ var ( "query-change-history", "list-metrics", "get-metric", + "list-metric-events", "list-experiments", "get-experiment", "get-experiment-results", "get-experiment-metric-results", + "get-flag-health", + "get-flag-unique-context-exposures", + "search-contexts", "list-segments", "get-segment", "list-approval-requests", @@ -97,14 +99,18 @@ var ( "toggle-flag", "create-flag", "update-flag-settings", + "update-off-variation", "create-metric", "update-rollout", "archive-flag", "create-experiment", + "update-experiment", "start-experiment-iteration", + "save-and-start-experiment-iteration", "stop-experiment-iteration", "start-guarded-rollout", "stop-guarded-rollout", + "create-automated-rollout-config", "update-targeting-rules", "update-individual-targets", "create-approval-request", @@ -139,13 +145,8 @@ type SetupOptions struct { OpenURL func(url string) error // 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. + // experiment-orchestration skill that differs from the latest one. 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) @@ -175,7 +176,15 @@ type SetupResult struct { 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"` + SkillError string `json:"skillError,omitempty"` + AgentAssetID string `json:"agentAssetId,omitempty"` + // AgentAction is what the custom agent step did, for example "created". + AgentAction string `json:"agentAction,omitempty"` + // AgentError is set if the custom agent step failed or was skipped. + AgentError string `json:"agentError,omitempty"` + // mcpServerName is the name of the server in AWS, which the tool IDs of + // the custom agent start with. + mcpServerName string } // ErrPlanChanged means AWS changed after the plan. @@ -406,8 +415,6 @@ type MCPStep struct { // them. MissingTools []string CanAddTools bool - // AddTools is set by --add-mcp-tools. - AddTools bool } // shared reports whether other spaces use the server. @@ -445,6 +452,7 @@ type SetupPlan struct { AuthFlow string MCP MCPStep Skill SkillStep + Agent AgentStep } // PlanText is the plan as lines for a person to read. @@ -457,11 +465,18 @@ type PlanText struct { OperatorApp string MCPServer string Skill string + Agent 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(), Skill: p.Skill.describe()} + text := PlanText{ + Scope: "Full setup", + AgentSpace: p.Space.describe(), + MCPServer: p.MCP.describe(), + Skill: p.Skill.describe(), + Agent: p.Agent.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" @@ -553,11 +568,13 @@ func PlanSetup(ctx context.Context, clients Clients, opts SetupOptions, provisio } if association != nil { plan.MCP.MissingTools, plan.MCP.CanAddTools = missingTools(*association) - plan.MCP.AddTools = opts.AddMCPTools } } - plan.Skill = planSkill(ctx, clients, opts, plan.Space) + // Download once, so the run writes the same texts that the plan compared. + skill, prompt, fetchErr := fetchSources(ctx, clients.Sources) + plan.Skill = planSkill(ctx, clients, opts, plan.Space, skill, fetchErr) + plan.Agent = planAgent(ctx, clients, plan.Space, plan.MCP, plan.Skill, prompt, fetchErr) if !provision { // Read the app, so the run does not print a URL that does not work. @@ -793,9 +810,12 @@ func checkMCPServiceID(ctx context.Context, clients Clients, id, endpoint string return "", fmt.Errorf("ID %s names no registered MCP server", id) } -// name is the name of the existing server in AWS, or MCPServerName when AWS -// sent none. +// name is the name of the server in AWS. A new server gets +// MCPOAuthServerName. func (m MCPStep) name() string { + if m.Action == MCPRegister { + return MCPOAuthServerName + } if m.Name != "" { return m.Name } @@ -816,12 +836,9 @@ func (m MCPStep) describe() string { } switch { case len(m.MissingTools) == 0: - case m.CanAddTools && m.AddTools: + case m.CanAddTools: 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" @@ -1005,9 +1022,10 @@ 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. + // The skill and the agent go last, so a failure of the optional steps + // cannot stop the MCP server step. installSkill(ctx, clients, plan.Skill, &result, logf) + installAgent(ctx, clients, plan.Agent, &result, logf) return result, nil } @@ -1067,6 +1085,7 @@ func ConnectPlan(ctx context.Context, clients Clients, opts SetupOptions, plan S return result, err } installSkill(ctx, clients, plan.Skill, &result, logf) + installAgent(ctx, clients, plan.Agent, &result, logf) return result, nil } @@ -1298,6 +1317,7 @@ func associateMCPServer( result *SetupResult, logf func(string, ...any), ) error { + result.mcpServerName = name existing, err := findMCPAssociation(ctx, clients, result.AgentSpaceID, result.MCPServiceID) if err != nil { return err @@ -1358,10 +1378,8 @@ func missingTools(association agenttypes.Association) (missing []string, 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. +// for example after an ldcli upgrade that allows more tools. A failure does not +// stop setup, because the server works with the tools that it has. func extendAllowlist( ctx context.Context, clients Clients, @@ -1380,11 +1398,6 @@ func extendAllowlist( 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. diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index 959e1a06..2c3d12a0 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -2,13 +2,9 @@ package awsdevops import ( "context" - "crypto/sha256" - _ "embed" - "encoding/hex" "encoding/json" "errors" "fmt" - "slices" "strings" "github.com/aws/aws-sdk-go-v2/aws" @@ -17,25 +13,7 @@ import ( 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 ExperimentSkillName = "experiment-orchestration" const ( skillAssetType = "skill" @@ -60,27 +38,19 @@ func frontMatterField(text, key string) string { 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 means that the SKILL.md differs from the latest one, 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 means that setup could not download the skill or read the + // skills of the space. The skill is optional, so the run continues without + // it. SkillSkip ) @@ -93,25 +63,25 @@ type SkillStep struct { // NoFile means that the skill has no SKILL.md, for example a skill that // someone made from a PDF. NoFile bool + // Text is the SKILL.md downloaded at plan time. + Text string // 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" + why := "it differs from the latest version on the main branch of " + aiToolingRepo 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) + return fmt.Sprintf("Skip skill %s because %s. Pass --replace-skill to update it to the latest version", label, why) case SkillReplace: - return fmt.Sprintf("Overwrite skill %s, which is not the ldcli version because %s", label, why) + return fmt.Sprintf("Update skill %s to the latest version on the main branch of %s", label, aiToolingRepo) case SkillSkip: return skillFailure(fmt.Errorf("Skip skill %s: %w", ExperimentSkillName, s.Err)) default: @@ -130,18 +100,21 @@ func skillFailure(err error) string { "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} +func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space SpaceStep, want string, fetchErr error) SkillStep { + switch { + case fetchErr != nil: + return SkillStep{Action: SkillSkip, Err: fetchErr} + case space.Action == SpaceCreate: + return SkillStep{Action: SkillCreate, Text: want} } asset, err := findSkill(ctx, clients, space.ID, ExperimentSkillName) if err != nil { return SkillStep{Action: SkillSkip, Err: err} } if asset == nil { - return SkillStep{Action: SkillCreate} + return SkillStep{Action: SkillCreate, Text: want} } - step := SkillStep{AssetID: aws.ToString(asset.AssetId), Version: aws.ToInt32(asset.Version)} + step := SkillStep{AssetID: aws.ToString(asset.AssetId), Version: aws.ToInt32(asset.Version), Text: want} text, found, err := skillText(ctx, clients, space.ID, step.AssetID) if err == nil && !found { // AWS also sends ResourceNotFoundException for an asset that someone @@ -151,7 +124,7 @@ func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space Sp case err != nil: return SkillStep{Action: SkillSkip, AssetID: step.AssetID, Err: err} case asset == nil: - return SkillStep{Action: SkillCreate} + return SkillStep{Action: SkillCreate, Text: want} case aws.ToString(asset.AssetId) != step.AssetID: return SkillStep{Action: SkillSkip, Err: planChanged("skill %s changed during the plan", ExperimentSkillName)} } @@ -160,11 +133,8 @@ func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space Sp switch { case err != nil: return SkillStep{Action: SkillSkip, AssetID: step.AssetID, Err: err} - case text == ExperimentSkill: + case found && text == want: 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: @@ -177,7 +147,11 @@ func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space Sp // 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) + return findAsset(ctx, clients, spaceID, skillAssetType, name) +} + +func findAsset(ctx context.Context, clients Clients, spaceID, assetType, name string) (*agenttypes.Asset, error) { + assets, err := listAssets(ctx, clients, spaceID, assetType) if err != nil { return nil, err } @@ -214,10 +188,14 @@ func assetName(asset agenttypes.Asset) string { // 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) { + return assetText(ctx, clients, spaceID, assetID, skillFilePath) +} + +func assetText(ctx context.Context, clients Clients, spaceID, assetID, path 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), + Path: aws.String(path), }) var missing *agenttypes.ResourceNotFoundException switch { @@ -225,7 +203,7 @@ func skillText(ctx context.Context, clients Clients, spaceID, assetID string) (t 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) + return "", false, fmt.Errorf("unable to read %s of asset %s: %w", path, assetID, err) case file.File == nil: return "", false, nil } @@ -263,7 +241,6 @@ func checkSkill(ctx context.Context, clients Clients, spaceID string, step Skill const ( SkillCreated = "created" SkillKept = "kept" - SkillUpdated = "updated" SkillLeft = "left" SkillOverwritten = "overwritten" SkillSkipped = "skipped" @@ -307,7 +284,7 @@ func doSkillStep( return SkillKept, nil case SkillLeave: - logf("Left skill %s (%s) unchanged. Pass --replace-skill to overwrite it", ExperimentSkillName, step.AssetID) + logf("Skipped skill %s (%s). Pass --replace-skill to update it to the latest version", ExperimentSkillName, step.AssetID) return SkillLeft, nil case SkillSkip: @@ -321,8 +298,9 @@ func doSkillStep( } content := &agenttypes.AssetContentMemberFile{Value: agenttypes.AssetFileContent{ Path: aws.String(skillFilePath), - Body: &agenttypes.AssetFileBodyMemberText{Value: ExperimentSkill}, + Body: &agenttypes.AssetFileBodyMemberText{Value: step.Text}, }} + description := frontMatterField(step.Text, "description") if step.Action == SkillCreate { created, err := clients.Agent.CreateAsset(ctx, &devopsagent.CreateAssetInput{ AgentSpaceId: aws.String(result.AgentSpaceID), @@ -330,7 +308,7 @@ func doSkillStep( Content: content, Metadata: document.NewLazyDocument(map[string]any{ "name": ExperimentSkillName, - "description": experimentSkillDescription, + "description": description, "agent_types": []string{"GENERIC"}, }), }) @@ -349,14 +327,11 @@ func doSkillStep( AgentSpaceId: aws.String(result.AgentSpaceID), AssetId: aws.String(step.AssetID), Content: content, - Metadata: document.NewLazyDocument(map[string]any{"description": experimentSkillDescription}), + Metadata: document.NewLazyDocument(map[string]any{"description": description}), }); 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 + return SkillOverwritten, nil } diff --git a/internal/awsdevops/skill_test.go b/internal/awsdevops/skill_test.go index 0c12ed03..ab91bf9c 100644 --- a/internal/awsdevops/skill_test.go +++ b/internal/awsdevops/skill_test.go @@ -29,18 +29,6 @@ func metadataOf(t *testing.T, doc document.Interface) map[string]any { 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{} @@ -56,13 +44,13 @@ func TestSetupCreatesSkillInNewSpace(t *testing.T) { 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, testDescription, metadata["description"]) 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, &agenttypes.AssetFileBodyMemberText{Value: testSkill}, file.Value.Body) assert.Equal(t, "ki-created", result.SkillAssetID) } @@ -71,13 +59,13 @@ 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}, + skillFiles: map[string]string{"ki-ours": testSkill}, } 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, awsdevops.SkillStep{Action: awsdevops.SkillKeep, AssetID: "ki-ours", Text: testSkill}, plan.Skill) assert.Equal(t, "Keep skill experiment-orchestration (ki-ours)", plan.Text().Skill) } @@ -94,7 +82,7 @@ func TestPlanSetupCreatesMissingSkill(t *testing.T) { func TestSetupKeepsSameSkill(t *testing.T) { agent := &fakeAgent{ assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, - skillFiles: map[string]string{"ki-ours": awsdevops.ExperimentSkill}, + skillFiles: map[string]string{"ki-ours": testSkill}, } result, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) @@ -111,7 +99,7 @@ func TestSetupKeepsSameSkill(t *testing.T) { func TestPlanSetupKeepsSameSkillSentAsBytes(t *testing.T) { agent := &fakeAgent{ assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, - skillBytes: map[string][]byte{"ki-ours": []byte(awsdevops.ExperimentSkill)}, + skillBytes: map[string][]byte{"ki-ours": []byte(testSkill)}, } plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) @@ -120,8 +108,8 @@ func TestPlanSetupKeepsSameSkillSentAsBytes(t *testing.T) { 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) { +// An empty SKILL.md is not the latest one, so setup skips it without the flag. +func TestPlanSetupLeavesEmptySkill(t *testing.T) { agent := &fakeAgent{ assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, skillFiles: map[string]string{"ki-ours": ""}, @@ -130,7 +118,7 @@ func TestPlanSetupUpdatesEmptySkill(t *testing.T) { plan, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) require.NoError(t, err) - assert.Equal(t, awsdevops.SkillUpdate, plan.Skill.Action) + assert.Equal(t, awsdevops.SkillLeave, plan.Skill.Action) } // A skill without SKILL.md, for example one made from a PDF, is not ours. @@ -142,31 +130,10 @@ func TestPlanSetupLeavesSkillWithoutFile(t *testing.T) { 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", + "Skip skill experiment-orchestration (ki-ours) because it has no SKILL.md. Pass --replace-skill to update it to the latest version", 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")}, @@ -209,14 +176,14 @@ func TestSetupOverwritesEditedSkillWithFlag(t *testing.T) { 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) + assert.Equal(t, &agenttypes.AssetFileBodyMemberText{Value: testSkill}, 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}, + skillFiles: map[string]string{"ki-ours": testSkill}, } opts := reuseOptions() opts.ReplaceSkill = true @@ -354,7 +321,7 @@ func TestPlanSetupCreatesSkillDeletedDuringPlan(t *testing.T) { 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) + assert.Equal(t, awsdevops.SkillStep{Action: awsdevops.SkillCreate, Text: testSkill}, plan.Skill) } func TestSetupWarnsOnSkillCreateFailure(t *testing.T) { diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md deleted file mode 100644 index bf816eb5..00000000 --- a/internal/awsdevops/skills/experiment-orchestration/SKILL.md +++ /dev/null @@ -1,171 +0,0 @@ ---- -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. diff --git a/internal/awsdevops/source.go b/internal/awsdevops/source.go new file mode 100644 index 00000000..ede2e71e --- /dev/null +++ b/internal/awsdevops/source.go @@ -0,0 +1,74 @@ +package awsdevops + +import ( + "context" + "fmt" + "io" + "net/http" + "time" +) + +// Sources reads files from the main branch of launchdarkly/ai-tooling. +type Sources interface { + File(ctx context.Context, path string) (string, error) +} + +const ( + aiToolingRepo = "launchdarkly/ai-tooling" + skillSourcePath = "skills/aws-devops-agent/experiment-orchestration/SKILL.md" + agentSourcePath = "skills/aws-devops-agent/agents/experiment-orchestrator.md" + maxSourceBytes = 1 << 20 +) + +type GitHubSources struct { + Client *http.Client + RawURL string +} + +func NewGitHubSources() GitHubSources { + return GitHubSources{ + Client: &http.Client{Timeout: 30 * time.Second}, + RawURL: "https://raw.githubusercontent.com", + } +} + +func (s GitHubSources) File(ctx context.Context, path string) (string, error) { + text, err := s.get(ctx, fmt.Sprintf("%s/%s/main/%s", s.RawURL, aiToolingRepo, path)) + if err != nil { + return "", fmt.Errorf("unable to download %s from %s: %w", path, aiToolingRepo, err) + } + + return text, nil +} + +func (s GitHubSources) get(ctx context.Context, address string) (string, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, address, nil) + if err != nil { + return "", err + } + resp, err := s.Client.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("GitHub sent %s", resp.Status) + } + body, err := io.ReadAll(io.LimitReader(resp.Body, maxSourceBytes)) + + return string(body), err +} + +func fetchSources(ctx context.Context, sources Sources) (skill, prompt string, err error) { + if sources == nil { + return "", "", fmt.Errorf("no source for the files of %s", aiToolingRepo) + } + if skill, err = sources.File(ctx, skillSourcePath); err != nil { + return "", "", err + } + if prompt, err = sources.File(ctx, agentSourcePath); err != nil { + return "", "", err + } + + return skill, prompt, nil +} diff --git a/internal/awsdevops/source_test.go b/internal/awsdevops/source_test.go new file mode 100644 index 00000000..2b1b899a --- /dev/null +++ b/internal/awsdevops/source_test.go @@ -0,0 +1,119 @@ +package awsdevops_test + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/launchdarkly/ldcli/internal/awsdevops" +) + +const ( + testSkill = "---\n" + + "name: experiment-orchestration\n" + + "description: Run an experiment from a goal to a report.\n" + + "---\n\n" + + "# Automated Experimentation\n\n" + + "Read the flag with `get-flag`, then call `create-experiment`.\n" + testAgentPrompt = "# Experiment orchestrator\n\nRun the experiment-orchestration skill.\n" + testDescription = "Run an experiment from a goal to a report." +) + +type fakeSources struct { + files map[string]string + fileErr error +} + +func newFakeSources() *fakeSources { + return &fakeSources{files: map[string]string{ + awsdevops.SkillSourcePath: testSkill, + awsdevops.AgentSourcePath: testAgentPrompt, + }} +} + +func (f *fakeSources) File(_ context.Context, path string) (string, error) { + if f.fileErr != nil { + return "", f.fileErr + } + text, ok := f.files[path] + if !ok { + return "", errors.New("GitHub sent 404 Not Found") + } + + return text, nil +} + +func sourcesOf(clients awsdevops.Clients) *fakeSources { + return clients.Sources.(*fakeSources) +} + +func TestSetupSkipsSkillAndAgentWhenDownloadFails(t *testing.T) { + agent := &fakeAgent{} + clients := newTestClients(agent, newFakeIAM()) + sourcesOf(clients).fileErr = errors.New("unable to download SKILL.md: dial tcp: no route to host") + + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), true) + require.NoError(t, err) + assert.Equal(t, awsdevops.SkillSkip, plan.Skill.Action) + assert.Equal(t, awsdevops.AgentSkip, plan.Agent.Action) + assert.Equal(t, "Skip skill experiment-orchestration: unable to download SKILL.md: dial tcp: no route to host", + plan.Text().Skill) + assert.Contains(t, plan.Text().Agent, "Skip custom agent experiment-orchestrator: unable to download SKILL.md") + + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + require.NoError(t, err) + assert.Contains(t, agent.calls, "RegisterService") + assert.Empty(t, agent.createAssets) + assert.Empty(t, agent.createAgents) + assert.Equal(t, awsdevops.SkillSkipped, result.SkillAction) + assert.Contains(t, result.SkillError, "no route to host") + assert.Equal(t, awsdevops.AgentSkipped, result.AgentAction) + assert.Contains(t, result.AgentError, "no route to host") +} + +// A new space has nothing to compare, but the steps still need the files. +func TestSetupSkipsSkillAndAgentInNewSpaceWhenDownloadFails(t *testing.T) { + agent := &fakeAgent{} + clients := newTestClients(agent, newFakeIAM()) + sourcesOf(clients).fileErr = errors.New("GitHub sent 503 Service Unavailable") + + result, err := awsdevops.Setup(context.Background(), clients, awsdevops.SetupOptions{ + AgentSpaceName: "launchdarkly", + AuthFlow: "iam", + }) + require.NoError(t, err) + + assert.Equal(t, "space-1", result.AgentSpaceID) + assert.Empty(t, agent.createAssets) + assert.Empty(t, agent.createAgents) + assert.Equal(t, awsdevops.SkillSkipped, result.SkillAction) + assert.Equal(t, awsdevops.AgentSkipped, result.AgentAction) + assert.Contains(t, result.SkillError, "503") +} + +func TestGitHubSourcesReadFiles(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/launchdarkly/ai-tooling/main/"+awsdevops.SkillSourcePath: + _, _ = w.Write([]byte(testSkill)) + case r.URL.Path == "/launchdarkly/ai-tooling/main/"+awsdevops.AgentSourcePath: + w.WriteHeader(http.StatusTooManyRequests) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + sources := awsdevops.GitHubSources{Client: server.Client(), RawURL: server.URL} + + text, err := sources.File(context.Background(), awsdevops.SkillSourcePath) + require.NoError(t, err) + assert.Equal(t, testSkill, text) + + _, err = sources.File(context.Background(), awsdevops.AgentSourcePath) + assert.ErrorContains(t, err, "GitHub sent 429 Too Many Requests") +}