From 481d1a3f427ee8ffe5a4d2a41fd30e317d09b2b5 Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Mon, 26 Oct 2026 13:00:36 -0700 Subject: [PATCH 01/11] feat: add the experimentation skill to the agent space in setup Setup now adds the experiment-orchestration skill to the agent space. The skill tells the agent how to move a goal through an experiment, a guarded rollout, and a report. The skill text comes from the setup guide. If the space has a skill with the same name and other instructions, setup does not change it. Pass --replace-skill to replace the instructions. The status and agent types of the skill stay the same. Co-Authored-By: Claude Opus 5.5 --- cmd/aws_devops_agent/confirm_test.go | 12 + cmd/aws_devops_agent/plan_print_test.go | 11 + cmd/aws_devops_agent/select_test.go | 16 ++ cmd/aws_devops_agent/setup.go | 20 +- internal/awsdevops/awsdevops.go | 4 + internal/awsdevops/awsdevops_test.go | 69 +++++- internal/awsdevops/lookup.go | 22 ++ internal/awsdevops/setup.go | 26 +- internal/awsdevops/skill.go | 232 ++++++++++++++++++ internal/awsdevops/skill_test.go | 226 +++++++++++++++++ .../skills/experiment-orchestration/SKILL.md | 167 +++++++++++++ 11 files changed, 797 insertions(+), 8 deletions(-) create mode 100644 internal/awsdevops/skill.go create mode 100644 internal/awsdevops/skill_test.go create mode 100644 internal/awsdevops/skills/experiment-orchestration/SKILL.md diff --git a/cmd/aws_devops_agent/confirm_test.go b/cmd/aws_devops_agent/confirm_test.go index 8cd36ccf6..7bad8b3d2 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") @@ -77,6 +83,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 47afff3d8..0ef842869 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 { diff --git a/cmd/aws_devops_agent/select_test.go b/cmd/aws_devops_agent/select_test.go index 54458388d..40b879468 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 } diff --git a/cmd/aws_devops_agent/setup.go b/cmd/aws_devops_agent/setup.go index 2dc40a2a2..c3187b5e1 100644 --- a/cmd/aws_devops_agent/setup.go +++ b/cmd/aws_devops_agent/setup.go @@ -27,6 +27,7 @@ const ( replaceMCPTokenFlag = "replace-mcp-token" mcpServiceIDFlag = "mcp-service-id" mcpEndpointFlag = "mcp-endpoint" + replaceSkillFlag = "replace-skill" yesFlag = "yes" ) @@ -34,12 +35,13 @@ 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 --auth-flow, setup runs the +MCP server and adds the skill. On a space that setup made, or with --auth-flow, setup runs the full setup. 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 @@ -53,7 +55,12 @@ 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. + +The experimentation skill tells the agent how to run an experiment from a +goal to a report. If the space has a skill with the same name and other +instructions, setup does not change it. To replace the instructions, pass +--replace-skill.`, Args: cobra.NoArgs, PreRun: trackRun(analyticsTrackerFn), RunE: runSetup, @@ -73,6 +80,7 @@ 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, "Replace the instructions of the "+awsdevops.ExperimentSkillName+" skill if they differ from the ldcli version") cmd.Flags().Bool(yesFlag, false, "Ask no questions and accept every step") @@ -495,7 +503,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 } @@ -681,6 +690,7 @@ func setupOptions(cmd *cobra.Command) (awsdevops.SetupOptions, error) { ReplaceMCPToken: mustBool(cmd, replaceMCPTokenFlag), MCPServiceID: mustString(cmd, mcpServiceIDFlag), MCPEndpoint: mustString(cmd, mcpEndpointFlag), + ReplaceSkill: mustBool(cmd, replaceSkillFlag), } idcSet := opts.IdcInstanceARN != "" diff --git a/internal/awsdevops/awsdevops.go b/internal/awsdevops/awsdevops.go index 72155518a..2afca795d 100644 --- a/internal/awsdevops/awsdevops.go +++ b/internal/awsdevops/awsdevops.go @@ -51,16 +51,20 @@ 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) + UpdateAsset(context.Context, *devopsagent.UpdateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.UpdateAssetOutput, 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 6a0de2f5a..a14d15966 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" @@ -64,6 +65,65 @@ 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 + listAssetsErr error + createAssetErr error + createAssets []*devopsagent.CreateAssetInput + 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") + text, ok := f.skillFiles[aws.ToString(in.AssetId)] + if !ok || aws.ToString(in.Path) != "SKILL.md" { + 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) { @@ -365,6 +425,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]) @@ -373,8 +434,12 @@ 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. The + // skill goes before the MCP step. + []string{ + "CreateAgentSpace", "AssociateService", "EnableOperatorApp", + "ListAssets", "CreateAsset", "ListServices", "RegisterService", "AssociateService", + }, agent.calls, ) } diff --git a/internal/awsdevops/lookup.go b/internal/awsdevops/lookup.go index 444d41e46..2e2c86a47 100644 --- a/internal/awsdevops/lookup.go +++ b/internal/awsdevops/lookup.go @@ -340,3 +340,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 eb68019cd..0d433bb71 100644 --- a/internal/awsdevops/setup.go +++ b/internal/awsdevops/setup.go @@ -115,6 +115,10 @@ type SetupOptions struct { // registration, so a self-hosted one is never confused with ours. MCPEndpoint string + // ReplaceSkill replaces the instructions of a skill with the name of the + // experimentation skill when they differ. + ReplaceSkill bool + // Logf, when set, reports progress as each step completes. Logf func(format string, args ...any) } @@ -132,6 +136,7 @@ type SetupResult struct { OperatorAppEnabled bool `json:"operatorAppEnabled"` MCPServiceID string `json:"mcpServiceId,omitempty"` MCPAssociationID string `json:"mcpAssociationId,omitempty"` + SkillAssetID string `json:"skillAssetId,omitempty"` } // ErrPlanChanged means AWS changed after the plan. @@ -396,6 +401,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. @@ -405,11 +411,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{AgentSpace: p.Space.describe(), MCPServer: p.MCP.describe()} + text := PlanText{AgentSpace: p.Space.describe(), MCPServer: p.MCP.describe(), Skill: p.Skill.describe()} if p.Space.Action == SpaceConnect { text.Association = "No change" text.Roles = []string{"No change"} @@ -492,6 +499,11 @@ func PlanSetup(ctx context.Context, clients Clients, opts SetupOptions, provisio return plan, err } + plan.Skill, err = planSkill(ctx, clients, opts, plan.Space) + if err != nil { + return plan, err + } + 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) @@ -884,6 +896,11 @@ func RunPlan(ctx context.Context, clients Clients, opts SetupOptions, plan Setup if err := checkSentToken(ctx, opts, plan.MCP); err != nil { return result, err } + if plan.Space.Action != SpaceCreate { + if err := checkSkill(ctx, clients, plan.Space.ID, plan.Skill); err != nil { + return result, err + } + } spaceRole, err := checkRole(ctx, clients.IAM, plan, AgentSpaceRoleName) if err != nil { return result, err @@ -969,6 +986,10 @@ func RunPlan(ctx context.Context, clients Clients, opts SetupOptions, plan Setup result.OperatorAppURL = OperatorAppURL(result.AgentSpaceID) } + if err := installSkill(ctx, clients, plan.Skill, &result, logf); err != nil { + return result, err + } + // The role and association steps can take minutes. A server that appeared // or went in that time makes AWS reject the step with an error that does // not say that the plan changed, so check the server again. A new space @@ -1054,6 +1075,9 @@ func ConnectPlan(ctx context.Context, clients Clients, opts SetupOptions, plan S if err := checkSentToken(ctx, opts, plan.MCP); err != nil { return result, err } + if err := installSkill(ctx, clients, plan.Skill, &result, logf); err != nil { + return result, err + } if err := registerMCPServer(ctx, clients, opts, plan.MCP, &result, logf); err != nil { return result, err } diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go new file mode 100644 index 000000000..6ed760744 --- /dev/null +++ b/internal/awsdevops/skill.go @@ -0,0 +1,232 @@ +package awsdevops + +import ( + "context" + _ "embed" + "encoding/json" + "errors" + "fmt" + "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") +) + +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 "" +} + +// SkillAction is the planned skill action. +type SkillAction int + +const ( + SkillCreate SkillAction = iota + SkillKeep + // SkillLeave means that a skill with the name has other instructions. Skills + // have no tags, so setup cannot tell an edit by the customer from an older + // ldcli version, and it does not replace them without --replace-skill. + SkillLeave + SkillReplace +) + +// SkillStep is the planned experimentation skill step. +type SkillStep struct { + Action SkillAction + AssetID string +} + +func (s SkillStep) describe() string { + label := fmt.Sprintf("%s (%s)", ExperimentSkillName, s.AssetID) + switch s.Action { + case SkillKeep: + return "Keep skill " + label + case SkillLeave: + return fmt.Sprintf("Leave skill %s unchanged because its instructions differ. "+ + "Pass --replace-skill to replace them", label) + case SkillReplace: + return "Replace the instructions of skill " + label + default: + return "Create skill " + ExperimentSkillName + } +} + +func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space SpaceStep) (SkillStep, error) { + if space.Action == SpaceCreate { + return SkillStep{Action: SkillCreate}, nil + } + id, err := findSkill(ctx, clients, space.ID, ExperimentSkillName) + if err != nil || id == "" { + return SkillStep{Action: SkillCreate}, err + } + same, err := skillMatches(ctx, clients, space.ID, id) + switch { + case err != nil: + return SkillStep{}, err + case same: + return SkillStep{Action: SkillKeep, AssetID: id}, nil + case opts.ReplaceSkill: + return SkillStep{Action: SkillReplace, AssetID: id}, nil + default: + return SkillStep{Action: SkillLeave, AssetID: id}, nil + } +} + +// findSkill returns the asset ID of the skill with this name in the space, or +// "" if there is none. AWS keeps skill names unique in a space. +func findSkill(ctx context.Context, clients Clients, spaceID, name string) (string, error) { + assets, err := listAssets(ctx, clients, spaceID, skillAssetType) + if err != nil { + return "", err + } + for _, asset := range assets { + if assetName(asset) == name { + return aws.ToString(asset.AssetId), nil + } + } + + return "", 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 +} + +// skillMatches reports whether the SKILL.md of the skill is ours. +func skillMatches(ctx context.Context, clients Clients, spaceID, assetID string) (bool, 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: + return false, fmt.Errorf("unable to read skill %s (%s): %w", ExperimentSkillName, assetID, err) + } + text, ok := file.File.Content.(*agenttypes.AssetFileBodyMemberText) + + return ok && text.Value == ExperimentSkill, nil +} + +// checkSkill compares the space with the skill step. +func checkSkill(ctx context.Context, clients Clients, spaceID string, step SkillStep) error { + id, err := findSkill(ctx, clients, spaceID, ExperimentSkillName) + switch { + case err != nil: + return err + case step.Action == SkillCreate && id != "": + return planChanged("skill %s appeared after the plan", ExperimentSkillName) + case step.Action != SkillCreate && id != step.AssetID: + return planChanged("skill %s disappeared after the plan", ExperimentSkillName) + } + + return nil +} + +func installSkill( + ctx context.Context, + clients Clients, + step SkillStep, + result *SetupResult, + logf func(string, ...any), +) error { + // The other steps can take minutes, so check the skill again. + 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}, + }} + switch step.Action { + case SkillKeep: + result.SkillAssetID = step.AssetID + logf("Kept skill %s (%s)", ExperimentSkillName, step.AssetID) + case SkillLeave: + result.SkillAssetID = step.AssetID + logf("Skill %s (%s) has other instructions, so ldcli did not change it", ExperimentSkillName, step.AssetID) + logf("Pass --replace-skill to replace them") + case SkillReplace: + // 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 replace the instructions of skill %s (%s): %w", ExperimentSkillName, step.AssetID, err) + } + result.SkillAssetID = step.AssetID + logf("Replaced the instructions of skill %s (%s)", ExperimentSkillName, step.AssetID) + default: + 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 nil +} diff --git a/internal/awsdevops/skill_test.go b/internal/awsdevops/skill_test.go new file mode 100644 index 000000000..b024b080e --- /dev/null +++ b/internal/awsdevops/skill_test.go @@ -0,0 +1,226 @@ +package awsdevops_test + +import ( + "context" + "encoding/json" + "errors" + "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") +} + +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", 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) +} + +func TestSetupLeavesChangedSkill(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") + + _, err = awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + require.NoError(t, err) + assert.Empty(t, agent.createAssets) + assert.Empty(t, agent.updateAssets) +} + +func TestSetupReplacesChangedSkillWithFlag(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": "# Older instructions"}, + } + opts := reuseOptions() + opts.ReplaceSkill = true + + _, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), opts) + require.NoError(t, err) + + assert.Empty(t, agent.createAssets) + 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) +} + +func TestRunPlanStopsWhenSkillAppears(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")} + + _, err = awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + + require.ErrorIs(t, err, awsdevops.ErrPlanChanged) + assert.Empty(t, agent.createAssets) + assert.NotContains(t, agent.calls, "EnableOperatorApp") +} + +func TestRunPlanStopsWhenSkillIsGone(t *testing.T) { + agent := &fakeAgent{ + assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, + skillFiles: map[string]string{"ki-ours": "# Older instructions"}, + } + 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 + + _, err = awsdevops.RunPlan(context.Background(), clients, opts, plan) + + require.ErrorIs(t, err, awsdevops.ErrPlanChanged) + assert.Empty(t, agent.updateAssets) +} + +func TestPlanSetupFailsWhenAssetsCannotBeListed(t *testing.T) { + agent := &fakeAgent{listAssetsErr: errors.New("AccessDeniedException: no aidevops:ListAssets")} + + _, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + + require.ErrorContains(t, err, "aidevops:ListAssets") + assert.ErrorContains(t, err, "space-2") +} + +func TestSetupReportsSkillCreateFailure(t *testing.T) { + agent := &fakeAgent{createAssetErr: errors.New("ServiceQuotaExceededException: 200 skills")} + + _, err := awsdevops.ConnectExisting(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + + require.ErrorContains(t, err, "unable to create skill experiment-orchestration") + assert.NotContains(t, agent.calls, "RegisterService") +} + +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 000000000..ba54057c4 --- /dev/null +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -0,0 +1,167 @@ +--- +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). + +**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. Name it with a clear pattern like `exp--` (e.g. `exp-checkout-conversion-cta-color`), lowercase with hyphens, ~50 chars max. +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 and move to Phase 2. +- **Significant + negative lift** → declare a loser, archive the flag, skip Phase 2, go straight to the Step 8 report. +- **No significance after a reasonable ceiling (e.g. 30 minutes)** → report "inconclusive, need more traffic" 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, clean up the flag (deprecate/archive 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: + +## 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. From 80289612f66531b6edca8003ec64825e6d58a889 Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Mon, 26 Oct 2026 13:18:13 -0700 Subject: [PATCH 02/11] fix: make the skill step optional and update copies from older versions Setup now does the skill step after the MCP server step. If the skill step fails, setup shows a warning and puts the error in skillError. The other steps still run. If setup cannot read the skills of the space, the plan skips the skill. Setup keeps the SHA-256 of each SKILL.md that it sent. If a space has one of these texts, setup updates it without --replace-skill. Other text is a customer edit. The plan keeps the version of the skill asset. If the version changes before the write, setup does not change the skill. The plan line and the help text say that every agent type in the space can load the skill. Co-Authored-By: Claude Opus 5.5 --- cmd/aws_devops_agent/plan_print_test.go | 21 +++ cmd/aws_devops_agent/setup.go | 28 ++-- internal/awsdevops/awsdevops_test.go | 7 +- internal/awsdevops/export_test.go | 14 ++ internal/awsdevops/setup.go | 29 ++-- internal/awsdevops/skill.go | 185 ++++++++++++++++-------- internal/awsdevops/skill_test.go | 110 +++++++++++--- 7 files changed, 284 insertions(+), 110 deletions(-) diff --git a/cmd/aws_devops_agent/plan_print_test.go b/cmd/aws_devops_agent/plan_print_test.go index 0ef842869..4129d451f 100644 --- a/cmd/aws_devops_agent/plan_print_test.go +++ b/cmd/aws_devops_agent/plan_print_test.go @@ -469,3 +469,24 @@ func TestSetupSaysWhenTheSecondPlanReusesAServer(t *testing.T) { assert.Contains(t, stderrOf(cmd), "MCP server Prod LD (mcp-7) was registered after the first plan") 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, errors.New("AccessDeniedException: no 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, errOut.String(), "Warning: unable to create skill "+awsdevops.ExperimentSkillName) + assert.Contains(t, stdoutOf(cmd), "The "+awsdevops.ExperimentSkillName+" skill is not set up") + assert.Contains(t, stdoutOf(cmd), "Open the DevOps Agent at:") +} diff --git a/cmd/aws_devops_agent/setup.go b/cmd/aws_devops_agent/setup.go index c3187b5e1..a3f2e7e52 100644 --- a/cmd/aws_devops_agent/setup.go +++ b/cmd/aws_devops_agent/setup.go @@ -41,11 +41,11 @@ 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 and adds the skill. On a space that setup made, or with --auth-flow, setup runs the -full setup. 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 +--auth-flow, setup runs the full setup. 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 @@ -57,10 +57,12 @@ 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. -The experimentation skill tells the agent how to run an experiment from a -goal to a report. If the space has a skill with the same name and other -instructions, setup does not change it. To replace the instructions, pass ---replace-skill.`, +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, @@ -80,7 +82,7 @@ instructions, setup does not change it. To replace the instructions, pass 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, "Replace the instructions of the "+awsdevops.ExperimentSkillName+" skill if they differ from the ldcli version") + cmd.Flags().Bool(replaceSkillFlag, false, "Overwrite the "+awsdevops.ExperimentSkillName+" skill if someone edited it") cmd.Flags().Bool(yesFlag, false, "Ask no questions and accept every step") @@ -181,6 +183,12 @@ func setupWith( awsdevops.AccessTokenURL(opts.LDBaseURI)) } + if result.SkillError != "" { + _, _ = fmt.Fprintf(cmd.OutOrStdout(), + "\nThe %s skill is not set up: %s. Run setup again to add it\n", + awsdevops.ExperimentSkillName, 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 { diff --git a/internal/awsdevops/awsdevops_test.go b/internal/awsdevops/awsdevops_test.go index a14d15966..04e576a6f 100644 --- a/internal/awsdevops/awsdevops_test.go +++ b/internal/awsdevops/awsdevops_test.go @@ -434,11 +434,10 @@ 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. The - // skill goes before the MCP step. + // before the MCP step looks, because the space exists by then, + // then the skill last. []string{ - "CreateAgentSpace", "AssociateService", "EnableOperatorApp", - "ListAssets", "CreateAsset", "ListServices", "RegisterService", "AssociateService", + "CreateAgentSpace", "AssociateService", "EnableOperatorApp", "ListServices", "RegisterService", "AssociateService", "ListAssets", "CreateAsset", }, agent.calls, ) diff --git a/internal/awsdevops/export_test.go b/internal/awsdevops/export_test.go index 38904942a..9ee486db2 100644 --- a/internal/awsdevops/export_test.go +++ b/internal/awsdevops/export_test.go @@ -21,3 +21,17 @@ 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) } diff --git a/internal/awsdevops/setup.go b/internal/awsdevops/setup.go index 0d433bb71..270429039 100644 --- a/internal/awsdevops/setup.go +++ b/internal/awsdevops/setup.go @@ -115,8 +115,9 @@ type SetupOptions struct { // registration, so a self-hosted one is never confused with ours. MCPEndpoint string - // ReplaceSkill replaces the instructions of a skill with the name of the - // experimentation skill when they differ. + // 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 // Logf, when set, reports progress as each step completes. @@ -137,6 +138,9 @@ type SetupResult struct { MCPServiceID string `json:"mcpServiceId,omitempty"` MCPAssociationID string `json:"mcpAssociationId,omitempty"` SkillAssetID string `json:"skillAssetId,omitempty"` + // SkillError is set if the skill step failed. The skill is optional, so + // the rest of the setup ran. + SkillError string `json:"skillError,omitempty"` } // ErrPlanChanged means AWS changed after the plan. @@ -499,10 +503,7 @@ func PlanSetup(ctx context.Context, clients Clients, opts SetupOptions, provisio return plan, err } - plan.Skill, err = planSkill(ctx, clients, opts, plan.Space) - if err != nil { - return plan, err - } + 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. @@ -896,11 +897,6 @@ func RunPlan(ctx context.Context, clients Clients, opts SetupOptions, plan Setup if err := checkSentToken(ctx, opts, plan.MCP); err != nil { return result, err } - if plan.Space.Action != SpaceCreate { - if err := checkSkill(ctx, clients, plan.Space.ID, plan.Skill); err != nil { - return result, err - } - } spaceRole, err := checkRole(ctx, clients.IAM, plan, AgentSpaceRoleName) if err != nil { return result, err @@ -986,10 +982,6 @@ func RunPlan(ctx context.Context, clients Clients, opts SetupOptions, plan Setup result.OperatorAppURL = OperatorAppURL(result.AgentSpaceID) } - if err := installSkill(ctx, clients, plan.Skill, &result, logf); err != nil { - return result, err - } - // The role and association steps can take minutes. A server that appeared // or went in that time makes AWS reject the step with an error that does // not say that the plan changed, so check the server again. A new space @@ -1000,6 +992,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 } @@ -1075,12 +1070,10 @@ func ConnectPlan(ctx context.Context, clients Clients, opts SetupOptions, plan S if err := checkSentToken(ctx, opts, plan.MCP); err != nil { return result, err } - if err := installSkill(ctx, clients, plan.Skill, &result, logf); err != nil { - return result, err - } 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 } diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index 6ed760744..785a1f754 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -2,10 +2,13 @@ package awsdevops import ( "context" + "crypto/sha256" _ "embed" + "encoding/hex" "encoding/json" "errors" "fmt" + "slices" "strings" "github.com/aws/aws-sdk-go-v2/aws" @@ -25,6 +28,14 @@ var ( 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{ + "a5a063f2f3739ca73c10602e05f18d36f5d0e29c372be025e7e6c71d9846c4eb", +} + const ( skillAssetType = "skill" skillFilePath = "SKILL.md" @@ -48,23 +59,37 @@ 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 - // SkillLeave means that a skill with the name has other instructions. Skills - // have no tags, so setup cannot tell an edit by the customer from an older - // ldcli version, and it does not replace them without --replace-skill. + // SkillUpdate replaces a SKILL.md that an older ldcli sent. + SkillUpdate + // SkillLeave means that someone edited the SKILL.md, so setup does not + // replace 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 + // Reason says why the step is skipped. + Reason string } func (s SkillStep) describe() string { @@ -72,51 +97,63 @@ func (s SkillStep) describe() string { 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 its instructions differ. "+ - "Pass --replace-skill to replace them", label) + return fmt.Sprintf("Leave skill %s unchanged because someone edited it. "+ + "Pass --replace-skill to overwrite it", label) case SkillReplace: - return "Replace the instructions of skill " + label + return fmt.Sprintf("Overwrite the edited instructions of skill %s", label) + case SkillSkip: + return fmt.Sprintf("Skip skill %s: %s", ExperimentSkillName, s.Reason) default: - return "Create skill " + ExperimentSkillName + return fmt.Sprintf("Create skill %s for every agent type in the space", ExperimentSkillName) } } -func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space SpaceStep) (SkillStep, error) { +func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space SpaceStep) SkillStep { if space.Action == SpaceCreate { - return SkillStep{Action: SkillCreate}, nil + return SkillStep{Action: SkillCreate} } - id, err := findSkill(ctx, clients, space.ID, ExperimentSkillName) - if err != nil || id == "" { - return SkillStep{Action: SkillCreate}, err + asset, err := findSkill(ctx, clients, space.ID, ExperimentSkillName) + if err != nil { + return SkillStep{Action: SkillSkip, Reason: errorCode(err)} + } + if asset == nil { + return SkillStep{Action: SkillCreate} } - same, err := skillMatches(ctx, clients, space.ID, id) + step := SkillStep{AssetID: aws.ToString(asset.AssetId), Version: aws.ToInt32(asset.Version)} + text, err := skillText(ctx, clients, space.ID, step.AssetID) switch { case err != nil: - return SkillStep{}, err - case same: - return SkillStep{Action: SkillKeep, AssetID: id}, nil + return SkillStep{Action: SkillSkip, AssetID: step.AssetID, Reason: errorCode(err)} + case text == ExperimentSkill: + step.Action = SkillKeep + case slices.Contains(shippedSkillHashes, skillHash(text)): + step.Action = SkillUpdate case opts.ReplaceSkill: - return SkillStep{Action: SkillReplace, AssetID: id}, nil + step.Action = SkillReplace default: - return SkillStep{Action: SkillLeave, AssetID: id}, nil + step.Action = SkillLeave } + + return step } -// findSkill returns the asset ID of the skill with this name in the space, or -// "" if there is none. AWS keeps skill names unique in a space. -func findSkill(ctx context.Context, clients Clients, spaceID, name string) (string, error) { +// 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 "", err + return nil, err } for _, asset := range assets { if assetName(asset) == name { - return aws.ToString(asset.AssetId), nil + return &asset, nil } } - return "", nil + return nil, nil } // assetName reads the name from the metadata of an asset. It decodes the JSON @@ -140,8 +177,8 @@ func assetName(asset agenttypes.Asset) string { return metadata.Name } -// skillMatches reports whether the SKILL.md of the skill is ours. -func skillMatches(ctx context.Context, clients Clients, spaceID, assetID string) (bool, error) { +// skillText returns the SKILL.md of the skill, or "" if it has none. +func skillText(ctx context.Context, clients Clients, spaceID, assetID string) (string, error) { file, err := clients.Agent.GetAssetFile(ctx, &devopsagent.GetAssetFileInput{ AgentSpaceId: aws.String(spaceID), AssetId: aws.String(assetID), @@ -150,38 +187,79 @@ func skillMatches(ctx context.Context, clients Clients, spaceID, assetID string) var missing *agenttypes.ResourceNotFoundException switch { case errors.As(err, &missing): - return false, nil + return "", nil case err != nil: - return false, fmt.Errorf("unable to read skill %s (%s): %w", ExperimentSkillName, assetID, err) + return "", fmt.Errorf("unable to read skill %s (%s): %w", ExperimentSkillName, assetID, err) + case file.File == nil: + return "", nil + } + text, _ := file.File.Content.(*agenttypes.AssetFileBodyMemberText) + if text == nil { + return "", nil } - text, ok := file.File.Content.(*agenttypes.AssetFileBodyMemberText) - return ok && text.Value == ExperimentSkill, nil + return text.Value, nil } -// checkSkill compares the space with the skill step. +// 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 { - id, err := findSkill(ctx, clients, spaceID, ExperimentSkillName) + asset, err := findSkill(ctx, clients, spaceID, ExperimentSkillName) switch { case err != nil: return err - case step.Action == SkillCreate && id != "": + case step.Action == SkillCreate && asset != nil: return planChanged("skill %s appeared after the plan", ExperimentSkillName) - case step.Action != SkillCreate && id != step.AssetID: + 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 } +// installSkill does the skill step. The skill is optional, so a failure does +// not stop setup. It 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 + if err := doSkillStep(ctx, clients, step, result, logf); err != nil { + result.SkillError = err.Error() + logf("Warning: %v", err) + } +} + +func doSkillStep( + ctx context.Context, + clients Clients, + step SkillStep, + result *SetupResult, + logf func(string, ...any), ) error { - // The other steps can take minutes, so check the skill again. + switch step.Action { + case SkillKeep: + logf("Kept skill %s (%s)", ExperimentSkillName, step.AssetID) + + return nil + case SkillLeave: + logf("Skill %s (%s) has edits, so ldcli did not change it", ExperimentSkillName, step.AssetID) + logf("Pass --replace-skill to overwrite it") + + return nil + case SkillSkip: + return fmt.Errorf("skipped skill %s: %s", ExperimentSkillName, step.Reason) + } + + // 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 } @@ -189,28 +267,7 @@ func installSkill( Path: aws.String(skillFilePath), Body: &agenttypes.AssetFileBodyMemberText{Value: ExperimentSkill}, }} - switch step.Action { - case SkillKeep: - result.SkillAssetID = step.AssetID - logf("Kept skill %s (%s)", ExperimentSkillName, step.AssetID) - case SkillLeave: - result.SkillAssetID = step.AssetID - logf("Skill %s (%s) has other instructions, so ldcli did not change it", ExperimentSkillName, step.AssetID) - logf("Pass --replace-skill to replace them") - case SkillReplace: - // 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 replace the instructions of skill %s (%s): %w", ExperimentSkillName, step.AssetID, err) - } - result.SkillAssetID = step.AssetID - logf("Replaced the instructions of skill %s (%s)", ExperimentSkillName, step.AssetID) - default: + if step.Action == SkillCreate { created, err := clients.Agent.CreateAsset(ctx, &devopsagent.CreateAssetInput{ AgentSpaceId: aws.String(result.AgentSpaceID), AssetType: aws.String(skillAssetType), @@ -226,7 +283,21 @@ func installSkill( } result.SkillAssetID = aws.ToString(created.Asset.AssetId) logf("Created skill %s (%s)", ExperimentSkillName, result.SkillAssetID) + + return 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) return nil } diff --git a/internal/awsdevops/skill_test.go b/internal/awsdevops/skill_test.go index b024b080e..4fa207e28 100644 --- a/internal/awsdevops/skill_test.go +++ b/internal/awsdevops/skill_test.go @@ -34,6 +34,13 @@ func TestExperimentSkillFrontMatter(t *testing.T) { 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{} @@ -82,7 +89,7 @@ func TestPlanSetupCreatesMissingSkill(t *testing.T) { require.NoError(t, err) assert.Equal(t, awsdevops.SkillCreate, plan.Skill.Action) - assert.Equal(t, "Create skill experiment-orchestration", plan.Text().Skill) + assert.Equal(t, "Create skill experiment-orchestration for every agent type in the space", plan.Text().Skill) } func TestSetupKeepsSameSkill(t *testing.T) { @@ -97,9 +104,30 @@ func TestSetupKeepsSameSkill(t *testing.T) { assert.Empty(t, agent.createAssets) assert.Empty(t, agent.updateAssets) assert.Equal(t, "ki-ours", result.SkillAssetID) + assert.Empty(t, result.SkillError) } -func TestSetupLeavesChangedSkill(t *testing.T) { +// 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)") + + _, 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)) +} + +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"}, @@ -111,16 +139,17 @@ func TestSetupLeavesChangedSkill(t *testing.T) { assert.Equal(t, awsdevops.SkillLeave, plan.Skill.Action) assert.Contains(t, plan.Text().Skill, "--replace-skill") - _, err = awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) require.NoError(t, err) assert.Empty(t, agent.createAssets) assert.Empty(t, agent.updateAssets) + assert.Empty(t, result.SkillError) } -func TestSetupReplacesChangedSkillWithFlag(t *testing.T) { +func TestSetupOverwritesEditedSkillWithFlag(t *testing.T) { agent := &fakeAgent{ assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, - skillFiles: map[string]string{"ki-ours": "# Older instructions"}, + skillFiles: map[string]string{"ki-ours": "# Edited by the customer"}, } opts := reuseOptions() opts.ReplaceSkill = true @@ -166,24 +195,27 @@ func TestConnectCreatesSkill(t *testing.T) { assert.Equal(t, "ki-created", result.SkillAssetID) } -func TestRunPlanStopsWhenSkillAppears(t *testing.T) { +// 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")} - _, err = awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) - require.ErrorIs(t, err, awsdevops.ErrPlanChanged) + require.NoError(t, err) assert.Empty(t, agent.createAssets) - assert.NotContains(t, agent.calls, "EnableOperatorApp") + assert.Contains(t, agent.calls, "RegisterService") + assert.Contains(t, result.SkillError, "appeared after the plan") } -func TestRunPlanStopsWhenSkillIsGone(t *testing.T) { +func TestRunPlanSkipsSkillThatIsGone(t *testing.T) { agent := &fakeAgent{ assets: []agenttypes.Asset{skillAsset("ki-ours", "experiment-orchestration")}, - skillFiles: map[string]string{"ki-ours": "# Older instructions"}, + skillFiles: map[string]string{"ki-ours": "# Edited by the customer"}, } clients := newTestClients(agent, newFakeIAM()) opts := reuseOptions() @@ -192,28 +224,64 @@ func TestRunPlanStopsWhenSkillIsGone(t *testing.T) { require.NoError(t, err) agent.assets = nil - _, err = awsdevops.RunPlan(context.Background(), clients, opts, plan) + result, err := awsdevops.RunPlan(context.Background(), clients, opts, plan) - require.ErrorIs(t, err, awsdevops.ErrPlanChanged) + require.NoError(t, err) assert.Empty(t, agent.updateAssets) + assert.Contains(t, result.SkillError, "disappeared after the plan") } -func TestPlanSetupFailsWhenAssetsCannotBeListed(t *testing.T) { +// 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") +} + +// Without aidevops:ListAssets, the plan skips the skill and setup does the +// other steps. +func TestSetupSkipsSkillWhenAssetsCannotBeListed(t *testing.T) { agent := &fakeAgent{listAssetsErr: errors.New("AccessDeniedException: no aidevops:ListAssets")} + clients := newTestClients(agent, newFakeIAM()) - _, err := awsdevops.PlanSetup(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions(), true) + plan, err := awsdevops.PlanSetup(context.Background(), clients, reuseOptions(), true) + require.NoError(t, err) + assert.Equal(t, awsdevops.SkillSkip, plan.Skill.Action) + assert.Contains(t, plan.Text().Skill, "aidevops:ListAssets") - require.ErrorContains(t, err, "aidevops:ListAssets") - assert.ErrorContains(t, err, "space-2") + result, err := awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + require.NoError(t, err) + assert.Contains(t, agent.calls, "RegisterService") + assert.Contains(t, result.SkillError, "aidevops:ListAssets") } -func TestSetupReportsSkillCreateFailure(t *testing.T) { +func TestSetupWarnsOnSkillCreateFailure(t *testing.T) { agent := &fakeAgent{createAssetErr: errors.New("ServiceQuotaExceededException: 200 skills")} - _, err := awsdevops.ConnectExisting(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) + result, err := awsdevops.ConnectExisting(context.Background(), newTestClients(agent, newFakeIAM()), reuseOptions()) - require.ErrorContains(t, err, "unable to create skill experiment-orchestration") - assert.NotContains(t, agent.calls, "RegisterService") + require.NoError(t, err) + assert.Contains(t, agent.calls, "RegisterService") + assert.Equal(t, "mcp-1", result.MCPServiceID) + assert.Contains(t, result.SkillError, "unable to create skill experiment-orchestration") + assert.Empty(t, result.SkillAssetID) } func keysOf(m map[string]any) []string { From 254e9a5e3d7c7314d9cfb810ad088edd8e935db1 Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Mon, 26 Oct 2026 13:29:21 -0700 Subject: [PATCH 03/11] fix: keep the AWS message in skill warnings and report the skill action The skill warning now keeps the full AWS error, so it names the denied action. If AWS denies access, the warning also names the permissions that the skill step needs. The warning shows one time, at the end of the run. An empty SKILL.md now gets an update without --replace-skill. A skill with no SKILL.md gets its own plan line. A SKILL.md that AWS sends as bytes is compared as text. The setup result now has skillAction, so a JSON run shows if setup kept, updated, or left the skill. Co-Authored-By: Claude Opus 5.5 --- cmd/aws_devops_agent/plan_print_test.go | 8 +- cmd/aws_devops_agent/setup.go | 4 +- internal/awsdevops/awsdevops_test.go | 15 ++- internal/awsdevops/setup.go | 6 +- internal/awsdevops/skill.go | 116 ++++++++++++++++-------- internal/awsdevops/skill_test.go | 85 +++++++++++++++-- 6 files changed, 179 insertions(+), 55 deletions(-) diff --git a/cmd/aws_devops_agent/plan_print_test.go b/cmd/aws_devops_agent/plan_print_test.go index 4129d451f..c3df5644b 100644 --- a/cmd/aws_devops_agent/plan_print_test.go +++ b/cmd/aws_devops_agent/plan_print_test.go @@ -473,7 +473,7 @@ func TestSetupSaysWhenTheSecondPlanReusesAServer(t *testing.T) { type failingSkillAgent struct{ countingAgent } func (failingSkillAgent) CreateAsset(context.Context, *devopsagent.CreateAssetInput, ...func(*devopsagent.Options)) (*devopsagent.CreateAssetOutput, error) { - return nil, errors.New("AccessDeniedException: no aidevops:CreateAsset") + 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. @@ -486,7 +486,9 @@ func TestSetupWarnsWhenSkillFails(t *testing.T) { require.NoError(t, setupWith(cmd, clients, newSpaceOptions(), true, false)) assert.Contains(t, w.calls, "RegisterService") - assert.Contains(t, errOut.String(), "Warning: unable to create skill "+awsdevops.ExperimentSkillName) - assert.Contains(t, stdoutOf(cmd), "The "+awsdevops.ExperimentSkillName+" skill is not set up") + 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")) } diff --git a/cmd/aws_devops_agent/setup.go b/cmd/aws_devops_agent/setup.go index a3f2e7e52..8be8cdbed 100644 --- a/cmd/aws_devops_agent/setup.go +++ b/cmd/aws_devops_agent/setup.go @@ -184,9 +184,7 @@ func setupWith( } if result.SkillError != "" { - _, _ = fmt.Fprintf(cmd.OutOrStdout(), - "\nThe %s skill is not set up: %s. Run setup again to add it\n", - awsdevops.ExperimentSkillName, 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 diff --git a/internal/awsdevops/awsdevops_test.go b/internal/awsdevops/awsdevops_test.go index 04e576a6f..fd0258125 100644 --- a/internal/awsdevops/awsdevops_test.go +++ b/internal/awsdevops/awsdevops_test.go @@ -68,7 +68,9 @@ type fakeAgent struct { // assets holds the skill assets of every space. assets []agenttypes.Asset // skillFiles holds the SKILL.md text by asset ID. - skillFiles map[string]string + skillFiles map[string]string + // skillBytes holds a SKILL.md that AWS sends as bytes, by asset ID. + skillBytes map[string][]byte listAssetsErr error createAssetErr error createAssets []*devopsagent.CreateAssetInput @@ -98,8 +100,17 @@ func (f *fakeAgent) CreateAsset(_ context.Context, in *devopsagent.CreateAssetIn func (f *fakeAgent) GetAssetFile(_ context.Context, in *devopsagent.GetAssetFileInput, _ ...func(*devopsagent.Options)) (*devopsagent.GetAssetFileOutput, error) { f.calls = append(f.calls, "GetAssetFile") + 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 || aws.ToString(in.Path) != "SKILL.md" { + if !ok { return nil, &agenttypes.ResourceNotFoundException{} } diff --git a/internal/awsdevops/setup.go b/internal/awsdevops/setup.go index 270429039..9998107b5 100644 --- a/internal/awsdevops/setup.go +++ b/internal/awsdevops/setup.go @@ -138,8 +138,10 @@ type SetupResult struct { MCPServiceID string `json:"mcpServiceId,omitempty"` MCPAssociationID string `json:"mcpAssociationId,omitempty"` SkillAssetID string `json:"skillAssetId,omitempty"` - // SkillError is set if the skill step failed. The skill is optional, so - // the rest of the setup ran. + // 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"` } diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index 785a1f754..715fab97f 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -71,10 +71,11 @@ type SkillAction int const ( SkillCreate SkillAction = iota SkillKeep - // SkillUpdate replaces a SKILL.md that an older ldcli sent. + // SkillUpdate replaces a SKILL.md that an older ldcli sent, or one that + // is empty. SkillUpdate - // SkillLeave means that someone edited the SKILL.md, so setup does not - // replace it without --replace-skill. + // 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 @@ -88,48 +89,67 @@ type SkillStep struct { AssetID string // Version is the asset version at plan time. A write stops if it changed. Version int32 - // Reason says why the step is skipped. - Reason string + // 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 someone edited it. "+ - "Pass --replace-skill to overwrite it", label) + return fmt.Sprintf("Leave skill %s unchanged because %s. Pass --replace-skill to overwrite it", label, why) case SkillReplace: - return fmt.Sprintf("Overwrite the edited instructions of skill %s", label) + return fmt.Sprintf("Overwrite skill %s, which is not the ldcli version because %s", label, why) case SkillSkip: - return fmt.Sprintf("Skip skill %s: %s", ExperimentSkillName, s.Reason) + 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, Reason: errorCode(err)} + 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, err := skillText(ctx, clients, space.ID, step.AssetID) + text, found, err := skillText(ctx, clients, space.ID, step.AssetID) + step.NoFile = !found switch { case err != nil: - return SkillStep{Action: SkillSkip, AssetID: step.AssetID, Reason: errorCode(err)} + return SkillStep{Action: SkillSkip, AssetID: step.AssetID, Err: err} case text == ExperimentSkill: step.Action = SkillKeep - case slices.Contains(shippedSkillHashes, skillHash(text)): + case found && (text == "" || slices.Contains(shippedSkillHashes, skillHash(text))): + // An empty file has nothing to lose. step.Action = SkillUpdate case opts.ReplaceSkill: step.Action = SkillReplace @@ -177,8 +197,9 @@ func assetName(asset agenttypes.Asset) string { return metadata.Name } -// skillText returns the SKILL.md of the skill, or "" if it has none. -func skillText(ctx context.Context, clients Clients, spaceID, assetID string) (string, error) { +// 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), @@ -187,18 +208,20 @@ func skillText(ctx context.Context, clients Clients, spaceID, assetID string) (s var missing *agenttypes.ResourceNotFoundException switch { case errors.As(err, &missing): - return "", nil + return "", false, nil case err != nil: - return "", fmt.Errorf("unable to read skill %s (%s): %w", ExperimentSkillName, assetID, err) + return "", false, fmt.Errorf("unable to read skill %s (%s): %w", ExperimentSkillName, assetID, err) case file.File == nil: - return "", nil + return "", false, nil } - text, _ := file.File.Content.(*agenttypes.AssetFileBodyMemberText) - if text == nil { - return "", 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 text.Value, nil + return "", false, nil } // checkSkill compares the space with a step that writes. A change to the @@ -221,8 +244,20 @@ func checkSkill(ctx context.Context, clients Clients, spaceID string, step Skill 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. It goes in result.SkillError, and the caller reports it. +// not stop setup. The failure goes in result.SkillError, and the caller +// reports it. func installSkill( ctx context.Context, clients Clients, @@ -231,9 +266,14 @@ func installSkill( logf func(string, ...any), ) { result.SkillAssetID = step.AssetID - if err := doSkillStep(ctx, clients, step, result, logf); err != nil { - result.SkillError = err.Error() - logf("Warning: %v", err) + action, err := doSkillStep(ctx, clients, step, result, logf) + result.SkillAction = action + if err != nil { + result.SkillAction = SkillFailed + if step.Action == SkillSkip { + result.SkillAction = SkillSkipped + } + result.SkillError = skillFailure(err) } } @@ -243,25 +283,24 @@ func doSkillStep( step SkillStep, result *SetupResult, logf func(string, ...any), -) error { +) (string, error) { switch step.Action { case SkillKeep: logf("Kept skill %s (%s)", ExperimentSkillName, step.AssetID) - return nil + return SkillKept, nil case SkillLeave: - logf("Skill %s (%s) has edits, so ldcli did not change it", ExperimentSkillName, step.AssetID) - logf("Pass --replace-skill to overwrite it") + logf("Left skill %s (%s) unchanged. Pass --replace-skill to overwrite it", ExperimentSkillName, step.AssetID) - return nil + return SkillLeft, nil case SkillSkip: - return fmt.Errorf("skipped skill %s: %s", ExperimentSkillName, step.Reason) + 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 + return "", err } content := &agenttypes.AssetContentMemberFile{Value: agenttypes.AssetFileContent{ Path: aws.String(skillFilePath), @@ -279,12 +318,12 @@ func doSkillStep( }), }) if err != nil { - return fmt.Errorf("unable to create skill %s: %w", ExperimentSkillName, err) + 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 nil + return SkillCreated, nil } // AWS updates only the metadata keys in the request, so the status and @@ -295,9 +334,12 @@ func doSkillStep( 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) + 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 nil + return SkillUpdated, nil } diff --git a/internal/awsdevops/skill_test.go b/internal/awsdevops/skill_test.go index 4fa207e28..37437b186 100644 --- a/internal/awsdevops/skill_test.go +++ b/internal/awsdevops/skill_test.go @@ -3,7 +3,7 @@ package awsdevops_test import ( "context" "encoding/json" - "errors" + "strings" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -104,9 +104,49 @@ func TestSetupKeepsSameSkill(t *testing.T) { 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")) @@ -121,10 +161,11 @@ func TestSetupUpdatesOlderShippedSkill(t *testing.T) { assert.Equal(t, awsdevops.SkillUpdate, plan.Skill.Action) assert.Contains(t, plan.Text().Skill, "Update skill experiment-orchestration (ki-ours)") - _, err = awsdevops.RunPlan(context.Background(), clients, reuseOptions(), plan) + 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) { @@ -143,6 +184,8 @@ func TestSetupLeavesEditedSkill(t *testing.T) { 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) } @@ -154,10 +197,11 @@ func TestSetupOverwritesEditedSkillWithFlag(t *testing.T) { opts := reuseOptions() opts.ReplaceSkill = true - _, err := awsdevops.Setup(context.Background(), newTestClients(agent, newFakeIAM()), opts) + 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)) @@ -193,6 +237,7 @@ func TestConnectCreatesSkill(t *testing.T) { 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 @@ -253,37 +298,61 @@ func TestRunPlanSkipsSkillEditedAfterPlan(t *testing.T) { require.NoError(t, err) assert.Empty(t, agent.updateAssets) assert.Contains(t, result.SkillError, "changed after the plan") + assert.Equal(t, awsdevops.SkillFailed, 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. +// 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: errors.New("AccessDeniedException: no aidevops:ListAssets")} + 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.Contains(t, plan.Text().Skill, "aidevops:ListAssets") + 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.Contains(t, result.SkillError, "aidevops:ListAssets") + 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 TestSetupWarnsOnSkillCreateFailure(t *testing.T) { - agent := &fakeAgent{createAssetErr: errors.New("ServiceQuotaExceededException: 200 skills")} + 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 { From 6dfcc5688e2eb8656625962edf3af9e2fa0e976d Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Mon, 26 Oct 2026 13:41:19 -0700 Subject: [PATCH 04/11] fix: report a plan change as a skipped skill and warn on stderr in JSON runs If the skill changes between the plan and the write, skillAction is now skipped, because setup tried no write. The warning for a denied GetAssetFile now names the skill one time. The plan line for a skipped skill starts with "Skip skill". A JSON run now also writes the skill warning to stderr. If someone deletes the skill after the list, the plan now creates a new skill. Before, the plan said that the skill had no SKILL.md. Co-Authored-By: Claude Opus 5.5 --- cmd/aws_devops_agent/plan_print_test.go | 14 +++++++++++ cmd/aws_devops_agent/setup.go | 5 ++++ internal/awsdevops/awsdevops_test.go | 13 ++++++++-- internal/awsdevops/skill.go | 22 ++++++++++++++--- internal/awsdevops/skill_test.go | 32 ++++++++++++++++++++++++- 5 files changed, 80 insertions(+), 6 deletions(-) diff --git a/cmd/aws_devops_agent/plan_print_test.go b/cmd/aws_devops_agent/plan_print_test.go index c3df5644b..b03b87140 100644 --- a/cmd/aws_devops_agent/plan_print_test.go +++ b/cmd/aws_devops_agent/plan_print_test.go @@ -492,3 +492,17 @@ func TestSetupWarnsWhenSkillFails(t *testing.T) { // 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) +} diff --git a/cmd/aws_devops_agent/setup.go b/cmd/aws_devops_agent/setup.go index 8be8cdbed..82bc832e7 100644 --- a/cmd/aws_devops_agent/setup.go +++ b/cmd/aws_devops_agent/setup.go @@ -173,6 +173,11 @@ func setupWith( } if !plaintext { + // stdout holds only the JSON, so a CI log shows the warning on stderr. + if result.SkillError != "" { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", result.SkillError) + } + return printJSON(cmd, result) } diff --git a/internal/awsdevops/awsdevops_test.go b/internal/awsdevops/awsdevops_test.go index fd0258125..ebe4195e6 100644 --- a/internal/awsdevops/awsdevops_test.go +++ b/internal/awsdevops/awsdevops_test.go @@ -70,8 +70,11 @@ type fakeAgent struct { // 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 + skillBytes map[string][]byte + listAssetsErr error + getFileErr error + // onGetFile runs at the start of each GetAssetFile call. + onGetFile func() createAssetErr error createAssets []*devopsagent.CreateAssetInput updateAssets []*devopsagent.UpdateAssetInput @@ -100,6 +103,12 @@ func (f *fakeAgent) CreateAsset(_ context.Context, in *devopsagent.CreateAssetIn 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{} } diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index 715fab97f..c797d4291 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -112,7 +112,7 @@ func (s SkillStep) describe() string { 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)) + 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) } @@ -142,6 +142,19 @@ func planSkill(ctx context.Context, clients Clients, opts SetupOptions, space Sp } 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: @@ -210,7 +223,8 @@ func skillText(ctx context.Context, clients Clients, spaceID, assetID string) (t case errors.As(err, &missing): return "", false, nil case err != nil: - return "", false, fmt.Errorf("unable to read skill %s (%s): %w", ExperimentSkillName, assetID, err) + // 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 } @@ -270,7 +284,9 @@ func installSkill( result.SkillAction = action if err != nil { result.SkillAction = SkillFailed - if step.Action == SkillSkip { + // 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) diff --git a/internal/awsdevops/skill_test.go b/internal/awsdevops/skill_test.go index 37437b186..f79cbf33d 100644 --- a/internal/awsdevops/skill_test.go +++ b/internal/awsdevops/skill_test.go @@ -255,6 +255,7 @@ func TestRunPlanSkipsSkillThatAppears(t *testing.T) { 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) { @@ -298,7 +299,8 @@ func TestRunPlanSkipsSkillEditedAfterPlan(t *testing.T) { require.NoError(t, err) assert.Empty(t, agent.updateAssets) assert.Contains(t, result.SkillError, "changed after the plan") - assert.Equal(t, awsdevops.SkillFailed, result.SkillAction) + // The step stopped before the write. + assert.Equal(t, awsdevops.SkillSkipped, result.SkillAction) } func accessDenied(action string) error { @@ -317,6 +319,7 @@ func TestSetupSkipsSkillWhenAssetsCannotBeListed(t *testing.T) { 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") @@ -328,6 +331,33 @@ func TestSetupSkipsSkillWhenAssetsCannotBeListed(t *testing.T) { 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")} From 44041f78c7c57cc10e0bcdf37aaa72a06273ad42 Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Mon, 26 Oct 2026 14:56:45 -0700 Subject: [PATCH 05/11] fix: let the experiment skill resume across runs and wait 2 weeks for a result A scheduled run of the agent has no memory of the run that started the experiment. The skill now keeps an experiment record, tags the flags that it owns, and resumes in-flight work before it starts a new goal. The experiment now runs at least 2 weeks before a decision, not 30 minutes. A result that is not a win stops the iteration with control as the winner and turns the flag off. Guarded rollout stages stay at or below 50%, as LaunchDarkly requires. If no coding agent is available, the skill waits for a person to confirm the deployment. Co-Authored-By: Claude Opus 5.5 --- internal/awsdevops/skill.go | 2 +- .../skills/experiment-orchestration/SKILL.md | 76 ++++++++++++++----- 2 files changed, 59 insertions(+), 19 deletions(-) diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index c797d4291..1fa6c72fd 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -33,7 +33,7 @@ var ( // updates it without --replace-skill. When you change SKILL.md, add the hash // of the new text. Never remove a hash. var shippedSkillHashes = []string{ - "a5a063f2f3739ca73c10602e05f18d36f5d0e29c372be025e7e6c71d9846c4eb", + "ec5723b6afbaa0f791e71153fb83594b83912f77901030dcf92ba4d55471789c", } const ( diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md index ba54057c4..f7ef92631 100644 --- a/internal/awsdevops/skills/experiment-orchestration/SKILL.md +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -9,10 +9,35 @@ Use this skill when you have a goal you want to move through experimentation (e. **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). +**Execution mode:** once the goal is confirmed (Step 1), run Steps 2–8 end-to-end, one step per invocation while you are resuming. 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), or if a person must implement and deploy the change (Step 3). + +**Resuming:** an experiment runs for weeks, so most invocations pick up work that an earlier one started. Always follow "Resuming in-flight work" below first. Start at Step 1 only for a goal that no open record already covers. **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. +## Experiment Record + +Keep one record per flag in the durable store your environment provides, and update it whenever the step changes. A later invocation has no memory of this one, so the record must hold everything it needs: + +- Flag key and project and environment keys +- Goal: metric, direction, target, and baseline +- Hypothesis +- Current step (for example, "Step 3: waiting for deployment", "Step 7: Phase 1 running", "Step 8: report produced") +- Implementation details for the report: files, branch, and release readiness result ("implemented by a person" when no coding agent did it) +- Deployment confirmation: who confirmed that the change is deployed, and when +- Experiment key, start date, and stop date +- Guardrail metric baselines at the experiment start +- Guarded rollout start date and end date + +Every flag this skill creates carries the tag `experiment-orchestration`, and its description holds the goal and the hypothesis. This marks the flags this skill owns, and it keeps the goal recoverable if the store is lost. + +## Resuming in-flight work + +1. Read every experiment record that is not at "Step 8: report produced". If you have no store, list the flags that are tagged `experiment-orchestration` and are not archived, and rebuild the records from their descriptions and their experiments. Never act on a flag without this tag. +2. For each one, read the current state from LaunchDarkly (flag, experiment, guarded rollout) and compare it with the record. +3. Advance each one by at most one step, following the step in this skill that the record names. A terminal outcome also produces its Step 8 report in the same invocation. Never start a new goal or accept a new hypothesis while resuming; those need a person. +4. Update the record. If nothing is in flight, say so and stop. + ## Step 1: Goal Clarification Before doing anything, get answers to: @@ -41,10 +66,18 @@ Explore the target repository/codebase to find a plausible change: ## Step 3: Implementation -1. Create a boolean feature flag, OFF by default in all environments. Name it with a clear pattern like `exp--` (e.g. `exp-checkout-conversion-cta-color`), lowercase with hyphens, ~50 chars max. -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. +1. Create a boolean feature flag, OFF by default in all environments, with the tag `experiment-orchestration` and a description that holds the goal and the hypothesis. Name it with a clear pattern like `exp--` (e.g. `exp-checkout-conversion-cta-color`), lowercase with hyphens, ~50 chars max. If the change is in a browser or mobile app, make the flag available to client-side SDKs (`clientSideAvailability.usingEnvironmentId`, or `usingMobileKey` for mobile); otherwise the app cannot read the flag and the experiment collects no data. +2. Create the experiment record (see "Experiment Record"). +3. 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. +4. This step is asynchronous — check status periodically rather than looping tightly on it. +5. Once implementation completes, move to Step 4 with the resulting branch/PR. If it fails, report the error and stop. + +**If no coding agent is available:** report the flag key and the change to make, set the record to "Step 3: waiting for deployment", and stop. A person implements, reviews, and deploys the change. Continue at Step 6 (skipping Steps 4–5) only when one of these is true: + +- A person says in this invocation that the change is deployed. Write their confirmation to the record. +- The record already holds a person's deployment confirmation. + +Until then, a resumed invocation reports "waiting for deployment" for this flag and changes nothing. Flag evaluations from the app show that the code evaluates the flag, but not that the treatment works, so they never replace a person's confirmation. ## Step 4: Release Readiness @@ -72,12 +105,14 @@ Deployment typically won't fire automatically on merge if your workflow is manua 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. +2. Create an experiment on the flag's default rule (`"fallthrough"`) with: + - Audience allocation of 10% of the rule's traffic (a flat allocation, not a staged ramp) on your chosen randomization unit (typically "user"). The other 90% receive the control variation and are not analyzed. + - Treatments: control (off) at 50%, treatment (on) at 50%. Treatment allocations must sum to 100%; the 10% is the audience allocation, not the treatments. + - Exactly one primary metric: the success metric from Step 1. If it does not exist, create the metric and the event that feeds it first. - 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. + - Sequential testing, if your methodology offers it, so that checking results on every invocation does not inflate false positives. +3. Start the experiment/data collection. +4. Record the experiment key, the start date, and the guardrail baselines, and set the record to "Step 7: Phase 1 running". 5. Move to Step 7 to monitor toward a decision. ## Step 7: Monitoring & Outcome @@ -86,23 +121,26 @@ Check status periodically — don't tight-loop. In an interactive session, check **Phase 1 — Prove the experiment at 10% (gate before any rollout):** -Watch for statistical significance on the primary metric: +Run the experiment for at least 2 full weeks, so that weekday and weekend behavior are both in the data. Check results once per invocation (daily is enough), and decide on the primary metric: -- **Significant + positive lift** → experiment proven. Stop the experiment iteration and move to Phase 2. -- **Significant + negative lift** → declare a loser, archive the flag, skip Phase 2, go straight to the Step 8 report. -- **No significance after a reasonable ceiling (e.g. 30 minutes)** → report "inconclusive, need more traffic" and stop; don't proceed to Phase 2. +- **Guardrail regression at any time** → stop early with control as the winner (see "Ending without a win" below). +- **Before 2 weeks** → keep running, even if the primary metric looks significant. Report progress only. +- **After 2 weeks, significant + positive lift** → experiment proven. Stop the iteration with the treatment as the winner and move to Phase 2. +- **After 2 weeks, significant + negative lift** → declare a loser and end without a win. +- **No significance after 4 weeks** → declare "inconclusive, need more traffic" and end without a win. -Never declare a winner off a single data point or before your stats engine confirms significance. +**Ending without a win:** stop the iteration with control as the winner and a reason that says why, because stopping an iteration requires a winning treatment. Stopping does not change targeting, so then turn the flag off so that everyone gets control. Record the stop date, skip Phase 2, and go to the Step 8 report. Don't archive the flag: the code still evaluates it. Recommend removing the flag from the code, and then archiving it, in the report's Next Steps. + 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. +- **Exactly 3 monitored stages, none above 50%** — e.g. 10% → 25% → 50%, ~24 hours monitoring each. LaunchDarkly rejects any step above 50%, because each step needs an equal split between the original and new variations, 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, clean up the flag (deprecate/archive it), and go to the Step 8 report. +LaunchDarkly advances the stages and rolls back on its own, so each invocation only reads the rollout status and updates the record. If the rollout rolls back or stops at any point, treat it as a regression: declare failed, confirm that everyone gets control, and go to the Step 8 report. Recommend flag cleanup in Next Steps rather than archiving the flag yourself. Once the final stage completes cleanly and auto-promotes to 100%, declare a winner and go to the Step 8 report. @@ -162,6 +200,8 @@ Runs automatically the instant Step 7 reaches a terminal outcome (winner + auto- - 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. +- **Run an experiment at least 2 full weeks** before deciding, unless a guardrail regresses. +- **Keep every guarded rollout stage at or below 50%** (LaunchDarkly rejects larger steps) and let the platform auto-promote to 100% after the final stage — don't try to add an explicit 100% stage. +- Update the experiment record on every step change, so that the next invocation can resume. - 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. From 9a09cc6bdd52bb20eb8098ad538932bd884d650c Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Mon, 26 Oct 2026 16:15:34 -0700 Subject: [PATCH 06/11] fix: shape the experiment skill for a chat run with approved writes In AWS DevOps Agent, every LaunchDarkly write needs a person to approve it in chat, and no coding agent or CI/CD is connected. The skill now runs as two chats: one that starts an experiment and one that checks on experiments. A person implements and deploys the change, and the agent stops at each wait. - The flag serves false when it is on, so turning it on before the experiment starts does not give everyone the treatment. - The experiment uses 10% of traffic through 5% and 5% treatments and notInExperimentVariationId, with sequential testing. - The agent shows the draft experiment and waits for a confirmation before it starts it, as the create-experiment tool asks. - The record in the flag description holds the project and environment. - The release review, merge, and deploy steps are gone. The allowlist adds update-flag-settings, for the record, and create-metric. Co-Authored-By: Claude Opus 5.5 --- internal/awsdevops/setup.go | 7 +- internal/awsdevops/skill.go | 2 +- internal/awsdevops/skill_test.go | 4 +- .../skills/experiment-orchestration/SKILL.md | 268 ++++++++---------- 4 files changed, 135 insertions(+), 146 deletions(-) diff --git a/internal/awsdevops/setup.go b/internal/awsdevops/setup.go index 9998107b5..95aafcf51 100644 --- a/internal/awsdevops/setup.go +++ b/internal/awsdevops/setup.go @@ -48,7 +48,10 @@ 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. var ( @@ -76,6 +79,8 @@ var ( mcpMutativeTools = []string{ "toggle-flag", "create-flag", + "update-flag-settings", + "create-metric", "update-rollout", "archive-flag", "create-experiment", diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index 1fa6c72fd..0028c62df 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -33,7 +33,7 @@ var ( // updates it without --replace-skill. When you change SKILL.md, add the hash // of the new text. Never remove a hash. var shippedSkillHashes = []string{ - "ec5723b6afbaa0f791e71153fb83594b83912f77901030dcf92ba4d55471789c", + "6972a3b18f2f8a52f9be2edc9157551f1bf5a7547026926b8eef9a348200d70e", } const ( diff --git a/internal/awsdevops/skill_test.go b/internal/awsdevops/skill_test.go index f79cbf33d..d44dd8440 100644 --- a/internal/awsdevops/skill_test.go +++ b/internal/awsdevops/skill_test.go @@ -31,7 +31,7 @@ func metadataOf(t *testing.T, doc document.Interface) map[string]any { func TestExperimentSkillFrontMatter(t *testing.T) { assert.Equal(t, "experiment-orchestration", awsdevops.ExperimentSkillName) - assert.Contains(t, awsdevops.ExperimentSkill, "# Automated Experimentation") + assert.Contains(t, awsdevops.ExperimentSkill, "# Experimentation") } // A change to SKILL.md must add its hash, so the next release updates the @@ -57,7 +57,7 @@ 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.Contains(t, metadata["description"], "Start a LaunchDarkly experiment for a product goal") assert.Equal(t, []any{"GENERIC"}, metadata["agent_types"]) assert.NotContains(t, metadata, "status") file, ok := in.Content.(*agenttypes.AssetContentMemberFile) diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md index f7ef92631..16ee2a62d 100644 --- a/internal/awsdevops/skills/experiment-orchestration/SKILL.md +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -1,207 +1,191 @@ --- name: "experiment-orchestration" -description: "Orchestrates automated experimentation lifecycle using LaunchDarkly Guarded Rollouts, an AI coding agent for implementation, and GitHub Actions for deployment." +description: "Start a LaunchDarkly experiment for a product goal, or check on running ones. Use when someone wants to move a goal (for example, increase checkout conversion by 15%) through a flag, an A/B experiment, and then a guarded rollout, or asks how their experiments are doing. Not for a standalone guarded rollout or flag change." --- -# Automated Experimentation +# 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%"). +Use this skill in chat when a person wants to move a goal through experimentation (for example, "increase checkout conversion by 15%"), or asks how their experiments are doing. -**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. +**Core principle: experiment first, then guarded rollout.** Always prove a change on a small, fixed slice of traffic with an A/B experiment before you ramp it up with a guarded rollout. A guarded rollout only scales a change that the experiment has already shown to work. -**Execution mode:** once the goal is confirmed (Step 1), run Steps 2–8 end-to-end, one step per invocation while you are resuming. 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), or if a person must implement and deploy the change (Step 3). +An experiment runs for weeks, so the work happens in separate chats: -**Resuming:** an experiment runs for weeks, so most invocations pick up work that an earlier one started. Always follow "Resuming in-flight work" below first. Start at Step 1 only for a goal that no open record already covers. +1. **Start an experiment** (Steps 1–4): goal, hypothesis, flag, a person ships the change, then the experiment starts. +2. **Check on experiments** (Steps 5–6): read where each experiment stands, decide when a decision is due, 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. +Decide which one the person asked for. If they ask about experiments that already exist, go to "Check on experiments". Never start a goal that the person did not ask for. -## Experiment Record +## How you work -Keep one record per flag in the durable store your environment provides, and update it whenever the step changes. A later invocation has no memory of this one, so the record must hold everything it needs: +- **A person implements and deploys the code.** You cannot read or change the code, and you cannot deploy. You create and change things in LaunchDarkly only. +- **Every write needs the person's approval.** Before each write (create a flag, change a flag, start or stop an experiment, start a guarded rollout), say in one sentence what it changes. If the person rejects it, stop that step and do not try another way. +- **A blocked write is a setup problem.** If a write fails with "This action is not available in this agent space", the agent space does not allow LaunchDarkly writes: directed actions are off, or the tool is not allowed. Tell the person, and do not retry. +- **Change only your flags.** Change only flags that have the tag `experiment-orchestration`. Read other flags for collision checks and history, but never change them. +- **Stop at each wait.** When the next step needs a person (shipping code, or two weeks of data), say what happens next and end. Do not wait or poll in the chat. -- Flag key and project and environment keys -- Goal: metric, direction, target, and baseline -- Hypothesis -- Current step (for example, "Step 3: waiting for deployment", "Step 7: Phase 1 running", "Step 8: report produced") -- Implementation details for the report: files, branch, and release readiness result ("implemented by a person" when no coding agent did it) -- Deployment confirmation: who confirmed that the change is deployed, and when -- Experiment key, start date, and stop date -- Guardrail metric baselines at the experiment start -- Guarded rollout start date and end date - -Every flag this skill creates carries the tag `experiment-orchestration`, and its description holds the goal and the hypothesis. This marks the flags this skill owns, and it keeps the goal recoverable if the store is lost. +## Experiment record -## Resuming in-flight work +The next chat may be with another person, and you have no memory of this one. Keep the record in the description of the flag, and update it whenever a field changes: -1. Read every experiment record that is not at "Step 8: report produced". If you have no store, list the flags that are tagged `experiment-orchestration` and are not archived, and rebuild the records from their descriptions and their experiments. Never act on a flag without this tag. -2. For each one, read the current state from LaunchDarkly (flag, experiment, guarded rollout) and compare it with the record. -3. Advance each one by at most one step, following the step in this skill that the record names. A terminal outcome also produces its Step 8 report in the same invocation. Never start a new goal or accept a new hypothesis while resuming; those need a person. -4. Update the record. If nothing is in flight, say so and stop. +- Project and environment keys +- Goal: metric, direction, target, and baseline +- Hypothesis, and the change that the person implements +- Current step, for example "waiting for deployment", "experiment running", "guarded rollout running", or "report produced" +- Deployment: who confirmed that the change is deployed, and when -## Step 1: Goal Clarification +Read everything else (the experiment key, dates, results, and rollout stages) from LaunchDarkly. -Before doing anything, get answers to: +# Start an experiment -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. +## Step 1: Goal -Questions 1–2 are required before proceeding; infer 3–5 where possible and confirm your assumptions with the user before implementing. +Get answers to: -## Step 2: Hypothesis Generation +1. **What metric measures success?** *(Required)* For example, conversion rate, page load time, or bounce rate. Keep the error-rate metric as a safety guardrail; never use it as the primary metric. +2. **What is the target improvement?** *(Required)* For example, a 15% increase or a 200ms decrease. +3. **What is the baseline?** *(Optional: read it from the metric if the person does not know.)* +4. **What part of the product is in scope?** *(Required, because you cannot read the code.)* +5. **Which LaunchDarkly project and environment?** *(Required)* The environment is the one that the app's production traffic evaluates in. Confirm both; do not guess. +6. **Any constraints?** *(Optional)* For example, no changes to the payment flow. -Explore the target repository/codebase to find a plausible change: +## Step 2: Hypothesis -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. +You cannot read the code, so build the hypothesis with the person: -**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. +1. Ask what the product surface does today and what change could move the metric. +2. Use LaunchDarkly for context: existing flags and metrics, archived flags with similar keys, and the change history. Avoid a hypothesis that was already tried and failed. +3. Propose one hypothesis in this shape: "Changing **X** will improve **metric** for **audience**, because **reason**." Confirm it with the person before you continue. -## Step 3: Implementation +**Before you pick a flag key, check for collisions:** -1. Create a boolean feature flag, OFF by default in all environments, with the tag `experiment-orchestration` and a description that holds the goal and the hypothesis. Name it with a clear pattern like `exp--` (e.g. `exp-checkout-conversion-cta-color`), lowercase with hyphens, ~50 chars max. If the change is in a browser or mobile app, make the flag available to client-side SDKs (`clientSideAvailability.usingEnvironmentId`, or `usingMobileKey` for mobile); otherwise the app cannot read the flag and the experiment collects no data. -2. Create the experiment record (see "Experiment Record"). -3. 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. -4. This step is asynchronous — check status periodically rather than looping tightly on it. -5. Once implementation completes, move to Step 4 with the resulting branch/PR. If it fails, report the error and stop. +- The key exists: pick a different key. If that flag tests the same idea, check its result first. + - Already shipped (100% one variation, no split): the idea is already decided, so pick a different hypothesis. + - An experiment is running on it: do not compete with it, so pick a different hypothesis. +- Does not exist: safe to create. -**If no coding agent is available:** report the flag key and the change to make, set the record to "Step 3: waiting for deployment", and stop. A person implements, reviews, and deploys the change. Continue at Step 6 (skipping Steps 4–5) only when one of these is true: +## Step 3: Flag and handoff -- A person says in this invocation that the change is deployed. Write their confirmation to the record. -- The record already holds a person's deployment confirmation. +1. Create a boolean flag, OFF in all environments, with the tag `experiment-orchestration` and `defaults` of `{"onVariation": 1, "offVariation": 1}`, so that it serves false whether it is on or off. A flag that is on and matches no rule serves its on variation, so without these defaults, turning it on would give every user the treatment before the experiment starts. Name it `exp--` (for example `exp-checkout-conversion-cta-color`), lowercase with hyphens, about 50 characters at most. If the change is in a browser or mobile app, make the flag available to client-side SDKs (`clientSideAvailability.usingEnvironmentId`, or `usingMobileKey` for mobile). Otherwise the app cannot read the flag, and the experiment collects no data. +2. Write the experiment record to the flag description, with the step "waiting for deployment". +3. Give the person the handoff: + - The flag key, and that the change must be behind it + - The change, in plain words + - What the code must do when the flag is off or cannot be evaluated: keep the current behavior + - The event that the primary metric needs, if the app does not send it yet +4. Stop. The person implements, reviews, and deploys the change, then comes back. -Until then, a resumed invocation reports "waiting for deployment" for this flag and changes nothing. Flag evaluations from the app show that the code evaluates the flag, but not that the treatment works, so they never replace a person's confirmation. +## Step 4: Start the experiment -## Step 4: Release Readiness +Start only after a person says that the change is deployed. -Run your standard release/risk review on the PR before merging. +**Design the experiment:** -- 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. +1. Make sure that the metrics exist. You can create a metric only for an event that the app already sends. If the event is not sent yet, ask the person to add it and come back. +2. Create the experiment as a draft, on the flag's default rule (`"fallthrough"`), in the environment from the record: + - Only 10% of the rule's traffic takes part: treatments of control (false) at 5% and treatment (true) at 5%, with `notInExperimentVariationId` set to the false variation, so that the other 90% get control and are not analyzed. If the tool rejects allocations that do not sum to 100%, do not fall back to 50/50 of all traffic: tell the person, and ask them to set a 10% audience on the design page before they confirm. + - Your randomization unit (usually "user"). + - Exactly one primary metric: the success metric from Step 1. + - Guardrail metrics: always the error-rate metric, and a performance metric (for example p95 page load time) for a performance change. + - `analysisConfig.sequentialTestingEnabled` set to true, so that checking results at each check-in does not inflate false positives. +3. Show the person the design and its `designUrl`, check with them that 10% of traffic takes part, and wait for their confirmation. Do not start the experiment in the same reply. -*(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.)* +**After the person confirms the design:** -## Step 5: Deployment +4. Turn the flag on. Its defaults serve false, so no user gets the treatment yet. Starting an experiment needs the flag on. +5. Start the experiment. +6. Write the record once: the step "experiment running", and who confirmed the deployment, and when. Tell the person to check back in 2 weeks, or sooner for a guardrail check, and end. -Deployment typically won't fire automatically on merge if your workflow is manually-triggered (`workflow_dispatch`-only) — you'll need to trigger it explicitly. +# Check on experiments -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 5: Read and decide -## Step 6: Experiment Phase (fixed 10%) +For each project in the person's account (or the one they name), list the flags with the tag `experiment-orchestration` that are not archived. For each one, read the record and the current state in the environment from the record, then report where it stands. Act only where a decision is due. -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. +If the person asks about an experiment on a flag without the tag, read it and report on it, but never change it. -1. Turn the flag ON. -2. Create an experiment on the flag's default rule (`"fallthrough"`) with: - - Audience allocation of 10% of the rule's traffic (a flat allocation, not a staged ramp) on your chosen randomization unit (typically "user"). The other 90% receive the control variation and are not analyzed. - - Treatments: control (off) at 50%, treatment (on) at 50%. Treatment allocations must sum to 100%; the 10% is the audience allocation, not the treatments. - - Exactly one primary metric: the success metric from Step 1. If it does not exist, create the metric and the event that feeds it first. - - 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. - - Sequential testing, if your methodology offers it, so that checking results on every invocation does not inflate false positives. -3. Start the experiment/data collection. -4. Record the experiment key, the start date, and the guardrail baselines, and set the record to "Step 7: Phase 1 running". -5. Move to Step 7 to monitor toward a decision. +**Waiting for deployment:** report it. If the person says now that the change is deployed, go to Step 4. -## Step 7: Monitoring & Outcome +**Experiment running:** -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. +- **A guardrail regressed, at any time:** end without a win (below). +- **Less than 2 full weeks of data:** keep it running, even if the primary metric looks significant. Report progress only. +- **2 weeks or more, significant positive lift:** the experiment is proven. Stop the iteration with the treatment as the winner, then start the guarded rollout (below). +- **2 weeks or more, significant negative lift:** a loser. End without a win. +- **2 to 4 weeks, not significant:** keep it running. Report progress only. +- **No significance after 4 weeks:** inconclusive, need more traffic. End without a win. -**Phase 1 — Prove the experiment at 10% (gate before any rollout):** +Never declare a winner off a single data point, or before the stats engine confirms significance. -Run the experiment for at least 2 full weeks, so that weekday and weekend behavior are both in the data. Check results once per invocation (daily is enough), and decide on the primary metric: +**Ending without a win:** stop the iteration with control as the winner and a reason that says why, because stopping an iteration requires a winning treatment. Stopping does not change targeting, so then turn the flag off so that everyone gets control. Do not archive the flag, because the code still evaluates it. Go to Step 6. -- **Guardrail regression at any time** → stop early with control as the winner (see "Ending without a win" below). -- **Before 2 weeks** → keep running, even if the primary metric looks significant. Report progress only. -- **After 2 weeks, significant + positive lift** → experiment proven. Stop the iteration with the treatment as the winner and move to Phase 2. -- **After 2 weeks, significant + negative lift** → declare a loser and end without a win. -- **No significance after 4 weeks** → declare "inconclusive, need more traffic" and end without a win. +**Starting the guarded rollout:** -**Ending without a win:** stop the iteration with control as the winner and a reason that says why, because stopping an iteration requires a winning treatment. Stopping does not change targeting, so then turn the flag off so that everyone gets control. Record the stop date, skip Phase 2, and go to the Step 8 report. Don't archive the flag: the code still evaluates it. Recommend removing the flag from the code, and then archiving it, in the report's Next Steps. - Never declare a winner off a single data point or before your stats engine confirms significance. +- The winning ("on") variation as the test, the original as control, and the same randomization unit as the experiment. +- Exactly 3 monitored stages, none above 50% (for example 10% → 25% → 50%), with about 24 hours of monitoring each. LaunchDarkly rejects any step above 50%, and it promotes the rollout to 100% itself after the last stage, so do not add a 100% stage. +- The same primary and guardrail metrics as the experiment, each set to notify and roll back on regression. -**Phase 2 — Guarded rollout ramp (only after Phase 1 proves the change):** +Set the record to "guarded rollout running". LaunchDarkly advances the stages and rolls back on its own, so later check-ins only read its status. -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, none above 50%** — e.g. 10% → 25% → 50%, ~24 hours monitoring each. LaunchDarkly rejects any step above 50%, because each step needs an equal split between the original and new variations, 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. +**Guarded rollout running:** -LaunchDarkly advances the stages and rolls back on its own, so each invocation only reads the rollout status and updates the record. If the rollout rolls back or stops at any point, treat it as a regression: declare failed, confirm that everyone gets control, and go to the Step 8 report. Recommend flag cleanup in Next Steps rather than archiving the flag yourself. +- **Still in a stage:** report the stage. +- **Rolled back or stopped:** the rollout failed. Confirm that everyone gets control, and go to Step 6. +- **Reached 100%:** a winner. Go to Step 6. -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 can come from an unrelated application error during the ramp, not from the change. Retry only when the person asks, and only after these checks: -**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. +1. The flag is back to 100% control, with nothing stuck mid-rollout. +2. The change history shows the timing between "advanced to next stage" and "reverted". A rollback within seconds of advancing does not fit a regression over a full metric window, so it points to an external cause. +3. The person confirms that the external cause is fixed. -**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. +Then restart the guarded rollout from the start with the same settings. Never retry a rollback that the metrics caused. -## Step 8: Report +## Step 6: 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: +When an experiment reaches an outcome (a winner at 100%, a loser, inconclusive, or a failed rollout), produce this report in the same reply, and set the record to "report produced": -## Experiment Report: [Goal Description] +## Experiment Report: [Goal] **Date:** [YYYY-MM-DD] **Goal:** [metric] [direction] by [target]% -**Status:** [achieved / in progress / stalled] +**Status:** [achieved / not achieved / inconclusive] ### 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] +### Change +- Flag: [flag key] +- Change: [as described in the handoff] +- Deployed: [who confirmed, and when] -### Experiment Phase (10% fixed split) -- Status: [proven / loser / inconclusive] +### Experiment (10% of traffic) +- Status: [proven / loser / inconclusive / stopped for a guardrail] - Duration: [time] - Metric change: [before] → [after] ([+/-]%) - Statistical significance: [value, confidence interval] -### Guarded Rollout Phase (if reached) -- Status: [completed / rolled_back / not started] +### Guarded rollout (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. -- **Run an experiment at least 2 full weeks** before deciding, unless a guardrail regresses. -- **Keep every guarded rollout stage at or below 50%** (LaunchDarkly rejects larger steps) and let the platform auto-promote to 100% after the final stage — don't try to add an explicit 100% stage. -- Update the experiment record on every step change, so that the next invocation can resume. -- 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. +- Stages reached: [N of 3] +- If rolled back and retried: [cause, and the result of the retry] + +### Safety metrics +- Error rate: [baseline] → [final] ([no regression / regression]) +- [Other guardrails]: [baseline] → [final] ([status]) + +### Next steps +[What to do next. For an experiment that did not win, recommend removing the flag from the code, and then archiving it.] + +## Safety rules + +- Present the hypothesis, and get the person's agreement, before you create anything. +- Never treat a change as deployed without a person's confirmation. +- Always prove a change with a fixed 10% experiment before any guarded rollout. +- Run an experiment for at least 2 full weeks before you decide, unless a guardrail regresses. +- Always include an error-rate guardrail, separate from the success metric. Add a performance guardrail for a performance change. +- Set every rollout metric to both notify and roll back on regression. +- Keep every guarded rollout stage at or below 50%, and let LaunchDarkly promote to 100%. +- Tell a rollback that the metrics caused (do not retry) from one with an external cause (safe to retry once it is fixed). +- Change only flags with the tag `experiment-orchestration`, and keep the record in the flag description current. From 800dc4d656e27bbf2a74dcdfc4771060d7b17bde Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Mon, 26 Oct 2026 16:29:11 -0700 Subject: [PATCH 07/11] fix: add missing MCP tools to an existing allowlist on request, and keep the win out of the 100% path Stopping an experiment iteration serves the chosen winner to everyone that the rule matches. The skill now stops a won experiment with control as the winner, records the win first, makes sure that the default rule serves control, and then ramps the treatment with the guarded rollout. A space that reuses an MCP server kept its old allowlist, so the tools that the skill now uses were missing. Setup now names the missing tools in the plan and warns. With --add-mcp-tools it adds them. It rewrites the allowlist only if every tool on it is an ldcli tool, because AWS replaces the whole configuration and does not send back each tool's classification, and an unclassified tool runs without approval. The run skips the rewrite if the allowlist changed after the plan. Co-Authored-By: Claude Opus 5.5 --- cmd/aws_devops_agent/select_test.go | 4 + cmd/aws_devops_agent/setup.go | 15 ++ internal/awsdevops/allowlist_test.go | 162 ++++++++++++++++++ internal/awsdevops/awsdevops.go | 1 + internal/awsdevops/awsdevops_test.go | 15 +- internal/awsdevops/export_test.go | 8 +- internal/awsdevops/setup.go | 142 ++++++++++++++- internal/awsdevops/skill.go | 2 +- .../skills/experiment-orchestration/SKILL.md | 10 +- 9 files changed, 347 insertions(+), 12 deletions(-) create mode 100644 internal/awsdevops/allowlist_test.go diff --git a/cmd/aws_devops_agent/select_test.go b/cmd/aws_devops_agent/select_test.go index 40b879468..87484e9e3 100644 --- a/cmd/aws_devops_agent/select_test.go +++ b/cmd/aws_devops_agent/select_test.go @@ -109,6 +109,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 82bc832e7..4c7bb5454 100644 --- a/cmd/aws_devops_agent/setup.go +++ b/cmd/aws_devops_agent/setup.go @@ -28,6 +28,7 @@ const ( mcpServiceIDFlag = "mcp-service-id" mcpEndpointFlag = "mcp-endpoint" replaceSkillFlag = "replace-skill" + addMCPToolsFlag = "add-mcp-tools" yesFlag = "yes" ) @@ -57,6 +58,11 @@ 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. +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 @@ -83,6 +89,8 @@ does the other steps.`, 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 +182,9 @@ 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) } @@ -188,6 +199,9 @@ 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) } @@ -702,6 +716,7 @@ func setupOptions(cmd *cobra.Command) (awsdevops.SetupOptions, error) { 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 000000000..1b34d5278 --- /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.AllowedTools()...)}, + } + + 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 2afca795d..83061513f 100644 --- a/internal/awsdevops/awsdevops.go +++ b/internal/awsdevops/awsdevops.go @@ -65,6 +65,7 @@ type AgentAPI interface { ListServices(context.Context, *devopsagent.ListServicesInput, ...func(*devopsagent.Options)) (*devopsagent.ListServicesOutput, error) RegisterService(context.Context, *devopsagent.RegisterServiceInput, ...func(*devopsagent.Options)) (*devopsagent.RegisterServiceOutput, 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 ebe4195e6..b59a018ec 100644 --- a/internal/awsdevops/awsdevops_test.go +++ b/internal/awsdevops/awsdevops_test.go @@ -77,7 +77,10 @@ type fakeAgent struct { onGetFile func() createAssetErr error createAssets []*devopsagent.CreateAssetInput - updateAssets []*devopsagent.UpdateAssetInput + // updateAssociations holds the allowlist updates. + updateAssociations []*devopsagent.UpdateAssociationInput + updateAssociationErr error + updateAssets []*devopsagent.UpdateAssetInput } // skillAsset is a skill asset named name. @@ -302,6 +305,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) diff --git a/internal/awsdevops/export_test.go b/internal/awsdevops/export_test.go index 9ee486db2..1526e3aac 100644 --- a/internal/awsdevops/export_test.go +++ b/internal/awsdevops/export_test.go @@ -1,6 +1,9 @@ package awsdevops -import "context" +import ( + "context" + "slices" +) // Setup plans a full setup and runs it, for tests. func Setup(ctx context.Context, clients Clients, opts SetupOptions) (SetupResult, error) { @@ -35,3 +38,6 @@ func ShippedSkillHashes() []string { return shippedSkillHashes } // SkillHash is the hash that setup compares. func SkillHash(text string) string { return skillHash(text) } + +// AllowedTools are the tools that setup allows on the MCP server. +func AllowedTools() []string { return slices.Concat(mcpReadOnlyTools, mcpMutativeTools) } diff --git a/internal/awsdevops/setup.go b/internal/awsdevops/setup.go index 95aafcf51..c21ec95cf 100644 --- a/internal/awsdevops/setup.go +++ b/internal/awsdevops/setup.go @@ -124,6 +124,10 @@ type SetupOptions struct { // 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) @@ -142,7 +146,13 @@ type SetupResult struct { OperatorAppEnabled bool `json:"operatorAppEnabled"` MCPServiceID string `json:"mcpServiceId,omitempty"` MCPAssociationID string `json:"mcpAssociationId,omitempty"` - SkillAssetID string `json:"skillAssetId,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 @@ -376,6 +386,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. @@ -510,6 +527,17 @@ 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 { @@ -803,6 +831,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: @@ -1213,7 +1253,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, result, logf) + return associateMCPServer(ctx, clients, opts, step, result, logf) } // A run after a failure finds an unconnected server by this name. name := MCPServerNameFor(result.AgentSpaceID) @@ -1302,7 +1342,7 @@ func registerMCPServer( } } - return associateMCPServer(ctx, clients, result, logf) + return associateMCPServer(ctx, clients, opts, step, result, logf) } // leaveSharedServer disconnects a shared server from this space only, after @@ -1453,16 +1493,19 @@ func disassociateEverywhere( func associateMCPServer( ctx context.Context, clients Clients, + opts SetupOptions, + step MCPStep, 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", MCPServerName) + extendAllowlist(ctx, clients, opts, step, *existing, result, logf) return nil } @@ -1477,6 +1520,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 index 0028c62df..dd223e1cf 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -33,7 +33,7 @@ var ( // updates it without --replace-skill. When you change SKILL.md, add the hash // of the new text. Never remove a hash. var shippedSkillHashes = []string{ - "6972a3b18f2f8a52f9be2edc9157551f1bf5a7547026926b8eef9a348200d70e", + "53b05092fd7f9ecca7027c60699c88162f1f4410430ea27afa0dc18205dd894c", } const ( diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md index 16ee2a62d..761ebddee 100644 --- a/internal/awsdevops/skills/experiment-orchestration/SKILL.md +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -31,7 +31,7 @@ The next chat may be with another person, and you have no memory of this one. Ke - Project and environment keys - Goal: metric, direction, target, and baseline - Hypothesis, and the change that the person implements -- Current step, for example "waiting for deployment", "experiment running", "guarded rollout running", or "report produced" +- Current step, for example "waiting for deployment", "experiment running", "won, guarded rollout pending", "guarded rollout running", or "report produced" - Deployment: who confirmed that the change is deployed, and when Read everything else (the experiment key, dates, results, and rollout stages) from LaunchDarkly. @@ -110,23 +110,27 @@ If the person asks about an experiment on a flag without the tag, read it and re - **A guardrail regressed, at any time:** end without a win (below). - **Less than 2 full weeks of data:** keep it running, even if the primary metric looks significant. Report progress only. -- **2 weeks or more, significant positive lift:** the experiment is proven. Stop the iteration with the treatment as the winner, then start the guarded rollout (below). +- **2 weeks or more, significant positive lift:** the experiment is proven. First set the record to "won, guarded rollout pending", so that a later check-in can tell the win apart from a loss. Then stop the iteration with control as the winner, and a reason that says that the treatment won, with its lift. Stopping serves the winner to everyone that the default rule matches, so a treatment winner would reach 100% of users at once and skip the guarded rollout. Then start the guarded rollout (below), which ramps the treatment. - **2 weeks or more, significant negative lift:** a loser. End without a win. - **2 to 4 weeks, not significant:** keep it running. Report progress only. - **No significance after 4 weeks:** inconclusive, need more traffic. End without a win. Never declare a winner off a single data point, or before the stats engine confirms significance. -**Ending without a win:** stop the iteration with control as the winner and a reason that says why, because stopping an iteration requires a winning treatment. Stopping does not change targeting, so then turn the flag off so that everyone gets control. Do not archive the flag, because the code still evaluates it. Go to Step 6. +**Ending without a win:** stop the iteration with control as the winner and a reason that says why, because stopping an iteration requires a winning treatment. Stopping serves control to everyone that the default rule matches. Then turn the flag off, so that its state shows that it is not in use. Do not archive the flag, because the code still evaluates it. Go to Step 6. **Starting the guarded rollout:** +Before you start it, read the flag's default rule. It must serve control (false) to everyone. If it still has the experiment's split, set it to serve control first. + - The winning ("on") variation as the test, the original as control, and the same randomization unit as the experiment. - Exactly 3 monitored stages, none above 50% (for example 10% → 25% → 50%), with about 24 hours of monitoring each. LaunchDarkly rejects any step above 50%, and it promotes the rollout to 100% itself after the last stage, so do not add a 100% stage. - The same primary and guardrail metrics as the experiment, each set to notify and roll back on regression. Set the record to "guarded rollout running". LaunchDarkly advances the stages and rolls back on its own, so later check-ins only read its status. +**Won, guarded rollout pending:** the iteration is stopped and the treatment won. Start the guarded rollout (above). + **Guarded rollout running:** - **Still in a stage:** report the stage. From 3b9332040586af7801afc451649c0e11198d3125 Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Tue, 27 Oct 2026 09:52:37 -0700 Subject: [PATCH 08/11] fix: address review feedback on the experiment skill - The report template is in a fenced markdown block, so its headings stay out of the skill's outline. - The report compares control with treatment, not before with after. - "Design waiting for confirmation" is a record state, so a later chat resumes the draft experiment instead of making another one. - The skill says why the 2-week minimum holds with sequential testing on: sequential testing lets a guardrail regression end the experiment early, and the minimum covers weekly cycles and novelty effects. - The report recommends a flag cleanup for a winner too. Co-Authored-By: Claude Opus 5.5 --- internal/awsdevops/skill.go | 2 +- .../skills/experiment-orchestration/SKILL.md | 21 ++++++++++++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index dd223e1cf..1d4ca7ad1 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -33,7 +33,7 @@ var ( // updates it without --replace-skill. When you change SKILL.md, add the hash // of the new text. Never remove a hash. var shippedSkillHashes = []string{ - "53b05092fd7f9ecca7027c60699c88162f1f4410430ea27afa0dc18205dd894c", + "57844bc3c5aca292ef88b5f949cd791206074b5cfc850ea52ebdd40020ead695", } const ( diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md index 761ebddee..98390cf77 100644 --- a/internal/awsdevops/skills/experiment-orchestration/SKILL.md +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -31,7 +31,7 @@ The next chat may be with another person, and you have no memory of this one. Ke - Project and environment keys - Goal: metric, direction, target, and baseline - Hypothesis, and the change that the person implements -- Current step, for example "waiting for deployment", "experiment running", "won, guarded rollout pending", "guarded rollout running", or "report produced" +- Current step, for example "waiting for deployment", "design waiting for confirmation", "experiment running", "won, guarded rollout pending", "guarded rollout running", or "report produced" - Deployment: who confirmed that the change is deployed, and when Read everything else (the experiment key, dates, results, and rollout stages) from LaunchDarkly. @@ -88,13 +88,14 @@ Start only after a person says that the change is deployed. - Exactly one primary metric: the success metric from Step 1. - Guardrail metrics: always the error-rate metric, and a performance metric (for example p95 page load time) for a performance change. - `analysisConfig.sequentialTestingEnabled` set to true, so that checking results at each check-in does not inflate false positives. -3. Show the person the design and its `designUrl`, check with them that 10% of traffic takes part, and wait for their confirmation. Do not start the experiment in the same reply. +3. Write the record: the step "design waiting for confirmation", the experiment key, and who confirmed the deployment, and when. A later chat then resumes the draft instead of making another one. +4. Show the person the design and its `designUrl`, check with them that 10% of traffic takes part, and wait for their confirmation. Do not start the experiment in the same reply. **After the person confirms the design:** -4. Turn the flag on. Its defaults serve false, so no user gets the treatment yet. Starting an experiment needs the flag on. -5. Start the experiment. -6. Write the record once: the step "experiment running", and who confirmed the deployment, and when. Tell the person to check back in 2 weeks, or sooner for a guardrail check, and end. +5. Turn the flag on. Its defaults serve false, so no user gets the treatment yet. Starting an experiment needs the flag on. +6. Start the experiment. +7. Set the record to "experiment running". Tell the person to check back in 2 weeks, or sooner for a guardrail check, and end. # Check on experiments @@ -106,6 +107,8 @@ If the person asks about an experiment on a flag without the tag, read it and re **Waiting for deployment:** report it. If the person says now that the change is deployed, go to Step 4. +**Design waiting for confirmation:** the draft experiment from the record exists but has not started. Show its `designUrl` again and ask the person to confirm it, then continue at item 5 of Step 4. Do not create another draft. + **Experiment running:** - **A guardrail regressed, at any time:** end without a win (below). @@ -117,6 +120,8 @@ If the person asks about an experiment on a flag without the tag, read it and re Never declare a winner off a single data point, or before the stats engine confirms significance. +Sequential testing keeps each check-in valid, which is what lets a guardrail regression end the experiment early. The 2-week minimum for a win is about the product, not the statistics: behavior differs between weekdays and weekends, and a new experience can get a short novelty bump. A win needs at least two full weekly cycles. + **Ending without a win:** stop the iteration with control as the winner and a reason that says why, because stopping an iteration requires a winning treatment. Stopping serves control to everyone that the default rule matches. Then turn the flag off, so that its state shows that it is not in use. Do not archive the flag, because the code still evaluates it. Go to Step 6. **Starting the guarded rollout:** @@ -149,6 +154,7 @@ Then restart the guarded rollout from the start with the same settings. Never re When an experiment reaches an outcome (a winner at 100%, a loser, inconclusive, or a failed rollout), produce this report in the same reply, and set the record to "report produced": +```markdown ## Experiment Report: [Goal] **Date:** [YYYY-MM-DD] @@ -166,7 +172,7 @@ When an experiment reaches an outcome (a winner at 100%, a loser, inconclusive, ### Experiment (10% of traffic) - Status: [proven / loser / inconclusive / stopped for a guardrail] - Duration: [time] -- Metric change: [before] → [after] ([+/-]%) +- Metric change: [control] → [treatment] ([+/-]%) - Statistical significance: [value, confidence interval] ### Guarded rollout (if reached) @@ -180,7 +186,8 @@ When an experiment reaches an outcome (a winner at 100%, a loser, inconclusive, - [Other guardrails]: [baseline] → [final] ([status]) ### Next steps -[What to do next. For an experiment that did not win, recommend removing the flag from the code, and then archiving it.] +[What to do next. For a winner at 100%, recommend a cleanup change that removes the flag from the code and keeps the winning path, and then archiving the flag. For an experiment that did not win, recommend removing the flag from the code and keeping the current behavior, and then archiving the flag.] +``` ## Safety rules From 58b1055afe2ee667f3f19d6338b6d6fa16631774 Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Wed, 28 Oct 2026 10:44:38 -0700 Subject: [PATCH 09/11] fix: restore the original experiment skill with minimal fixes Use the skill from the setup guide again. The skill runs in one long chat, so the two-chat design and the flag description record are gone. The skill keeps the original text, with these fixes: - The flag serves false when it is on, and every SDK can read it. - The flag key follows the project naming conventions. - Each stop of an iteration uses control as the winner, so a win does not skip the guarded rollout. - An experiment runs at least 2 weeks, and is inconclusive after 4 weeks. - A loss or a rollback turns the flag off instead of archiving it. - The report template is in a code fence. - In an environment that needs approval, the agent creates an approval request and applies it after a reviewer approves it. The MCP allowlist adds apply-approval-request as a mutative tool. Co-Authored-By: Claude Opus 5.5 --- internal/awsdevops/awsdevops_test.go | 1 + internal/awsdevops/setup.go | 4 +- internal/awsdevops/skill.go | 1 + internal/awsdevops/skill_test.go | 4 +- .../skills/experiment-orchestration/SKILL.md | 250 ++++++++---------- 5 files changed, 117 insertions(+), 143 deletions(-) diff --git a/internal/awsdevops/awsdevops_test.go b/internal/awsdevops/awsdevops_test.go index b59a018ec..5c4ff50f0 100644 --- a/internal/awsdevops/awsdevops_test.go +++ b/internal/awsdevops/awsdevops_test.go @@ -994,6 +994,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/setup.go b/internal/awsdevops/setup.go index c21ec95cf..ca5f5eed5 100644 --- a/internal/awsdevops/setup.go +++ b/internal/awsdevops/setup.go @@ -53,7 +53,8 @@ const ( // 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", @@ -91,6 +92,7 @@ var ( "update-targeting-rules", "update-individual-targets", "create-approval-request", + "apply-approval-request", } ) diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index 1d4ca7ad1..49dbe7223 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -34,6 +34,7 @@ var ( // of the new text. Never remove a hash. var shippedSkillHashes = []string{ "57844bc3c5aca292ef88b5f949cd791206074b5cfc850ea52ebdd40020ead695", + "d67f47da9fa05d3991a07f8f22fa001e1e7285a5574974e1ac669caca9654bf4", } const ( diff --git a/internal/awsdevops/skill_test.go b/internal/awsdevops/skill_test.go index d44dd8440..f79cbf33d 100644 --- a/internal/awsdevops/skill_test.go +++ b/internal/awsdevops/skill_test.go @@ -31,7 +31,7 @@ func metadataOf(t *testing.T, doc document.Interface) map[string]any { func TestExperimentSkillFrontMatter(t *testing.T) { assert.Equal(t, "experiment-orchestration", awsdevops.ExperimentSkillName) - assert.Contains(t, awsdevops.ExperimentSkill, "# Experimentation") + assert.Contains(t, awsdevops.ExperimentSkill, "# Automated Experimentation") } // A change to SKILL.md must add its hash, so the next release updates the @@ -57,7 +57,7 @@ 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"], "Start a LaunchDarkly experiment for a product goal") + 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) diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md index 98390cf77..1eb5f5e54 100644 --- a/internal/awsdevops/skills/experiment-orchestration/SKILL.md +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -1,202 +1,172 @@ --- name: "experiment-orchestration" -description: "Start a LaunchDarkly experiment for a product goal, or check on running ones. Use when someone wants to move a goal (for example, increase checkout conversion by 15%) through a flag, an A/B experiment, and then a guarded rollout, or asks how their experiments are doing. Not for a standalone guarded rollout or flag change." +description: "Orchestrates automated experimentation lifecycle using LaunchDarkly Guarded Rollouts, an AI coding agent for implementation, and GitHub Actions for deployment." --- -# Experimentation +# Automated Experimentation -Use this skill in chat when a person wants to move a goal through experimentation (for example, "increase checkout conversion by 15%"), or asks how their experiments are doing. +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 with an A/B experiment before you ramp it up with a guarded rollout. A guarded rollout only scales a change that the experiment has already shown to work. +**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. -An experiment runs for weeks, so the work happens in separate chats: +**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). -1. **Start an experiment** (Steps 1–4): goal, hypothesis, flag, a person ships the change, then the experiment starts. -2. **Check on experiments** (Steps 5–6): read where each experiment stands, decide when a decision is due, and report. +**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. -Decide which one the person asked for. If they ask about experiments that already exist, go to "Check on experiments". Never start a goal that the person did not ask for. +**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. -## How you work +## Step 1: Goal Clarification -- **A person implements and deploys the code.** You cannot read or change the code, and you cannot deploy. You create and change things in LaunchDarkly only. -- **Every write needs the person's approval.** Before each write (create a flag, change a flag, start or stop an experiment, start a guarded rollout), say in one sentence what it changes. If the person rejects it, stop that step and do not try another way. -- **A blocked write is a setup problem.** If a write fails with "This action is not available in this agent space", the agent space does not allow LaunchDarkly writes: directed actions are off, or the tool is not allowed. Tell the person, and do not retry. -- **Change only your flags.** Change only flags that have the tag `experiment-orchestration`. Read other flags for collision checks and history, but never change them. -- **Stop at each wait.** When the next step needs a person (shipping code, or two weeks of data), say what happens next and end. Do not wait or poll in the chat. +Before doing anything, get answers to: -## Experiment record +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. -The next chat may be with another person, and you have no memory of this one. Keep the record in the description of the flag, and update it whenever a field changes: +Questions 1–2 are required before proceeding; infer 3–5 where possible and confirm your assumptions with the user before implementing. -- Project and environment keys -- Goal: metric, direction, target, and baseline -- Hypothesis, and the change that the person implements -- Current step, for example "waiting for deployment", "design waiting for confirmation", "experiment running", "won, guarded rollout pending", "guarded rollout running", or "report produced" -- Deployment: who confirmed that the change is deployed, and when +## Step 2: Hypothesis Generation -Read everything else (the experiment key, dates, results, and rollout stages) from LaunchDarkly. +Explore the target repository/codebase to find a plausible change: -# Start an experiment +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. -## Step 1: Goal +**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. -Get answers to: +## Step 3: Implementation -1. **What metric measures success?** *(Required)* For example, conversion rate, page load time, or bounce rate. Keep the error-rate metric as a safety guardrail; never use it as the primary metric. -2. **What is the target improvement?** *(Required)* For example, a 15% increase or a 200ms decrease. -3. **What is the baseline?** *(Optional: read it from the metric if the person does not know.)* -4. **What part of the product is in scope?** *(Required, because you cannot read the code.)* -5. **Which LaunchDarkly project and environment?** *(Required)* The environment is the one that the app's production traffic evaluates in. Confirm both; do not guess. -6. **Any constraints?** *(Optional)* For example, no changes to the payment flow. +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 2: Hypothesis +## Step 4: Release Readiness -You cannot read the code, so build the hypothesis with the person: +Run your standard release/risk review on the PR before merging. -1. Ask what the product surface does today and what change could move the metric. -2. Use LaunchDarkly for context: existing flags and metrics, archived flags with similar keys, and the change history. Avoid a hypothesis that was already tried and failed. -3. Propose one hypothesis in this shape: "Changing **X** will improve **metric** for **audience**, because **reason**." Confirm it with the person before you continue. +- 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. -**Before you pick a flag key, check for collisions:** +*(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.)* -- The key exists: pick a different key. If that flag tests the same idea, check its result first. - - Already shipped (100% one variation, no split): the idea is already decided, so pick a different hypothesis. - - An experiment is running on it: do not compete with it, so pick a different hypothesis. -- Does not exist: safe to create. +## Step 5: Deployment -## Step 3: Flag and handoff +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. Create a boolean flag, OFF in all environments, with the tag `experiment-orchestration` and `defaults` of `{"onVariation": 1, "offVariation": 1}`, so that it serves false whether it is on or off. A flag that is on and matches no rule serves its on variation, so without these defaults, turning it on would give every user the treatment before the experiment starts. Name it `exp--` (for example `exp-checkout-conversion-cta-color`), lowercase with hyphens, about 50 characters at most. If the change is in a browser or mobile app, make the flag available to client-side SDKs (`clientSideAvailability.usingEnvironmentId`, or `usingMobileKey` for mobile). Otherwise the app cannot read the flag, and the experiment collects no data. -2. Write the experiment record to the flag description, with the step "waiting for deployment". -3. Give the person the handoff: - - The flag key, and that the change must be behind it - - The change, in plain words - - What the code must do when the flag is off or cannot be evaluated: keep the current behavior - - The event that the primary metric needs, if the app does not send it yet -4. Stop. The person implements, reviews, and deploys the change, then comes back. +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 4: Start the experiment +## Step 6: Experiment Phase (fixed 10%) -Start only after a person says that the change is deployed. +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. -**Design the experiment:** - -1. Make sure that the metrics exist. You can create a metric only for an event that the app already sends. If the event is not sent yet, ask the person to add it and come back. -2. Create the experiment as a draft, on the flag's default rule (`"fallthrough"`), in the environment from the record: - - Only 10% of the rule's traffic takes part: treatments of control (false) at 5% and treatment (true) at 5%, with `notInExperimentVariationId` set to the false variation, so that the other 90% get control and are not analyzed. If the tool rejects allocations that do not sum to 100%, do not fall back to 50/50 of all traffic: tell the person, and ask them to set a 10% audience on the design page before they confirm. - - Your randomization unit (usually "user"). +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 metrics: always the error-rate metric, and a performance metric (for example p95 page load time) for a performance change. - - `analysisConfig.sequentialTestingEnabled` set to true, so that checking results at each check-in does not inflate false positives. -3. Write the record: the step "design waiting for confirmation", the experiment key, and who confirmed the deployment, and when. A later chat then resumes the draft instead of making another one. -4. Show the person the design and its `designUrl`, check with them that 10% of traffic takes part, and wait for their confirmation. Do not start the experiment in the same reply. - -**After the person confirms the design:** - -5. Turn the flag on. Its defaults serve false, so no user gets the treatment yet. Starting an experiment needs the flag on. -6. Start the experiment. -7. Set the record to "experiment running". Tell the person to check back in 2 weeks, or sooner for a guardrail check, and end. - -# Check on experiments - -## Step 5: Read and decide - -For each project in the person's account (or the one they name), list the flags with the tag `experiment-orchestration` that are not archived. For each one, read the record and the current state in the environment from the record, then report where it stands. Act only where a decision is due. - -If the person asks about an experiment on a flag without the tag, read it and report on it, but never change it. - -**Waiting for deployment:** report it. If the person says now that the change is deployed, go to Step 4. + - 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. -**Design waiting for confirmation:** the draft experiment from the record exists but has not started. Show its `designUrl` again and ask the person to confirm it, then continue at item 5 of Step 4. Do not create another draft. +## Step 7: Monitoring & Outcome -**Experiment running:** +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. -- **A guardrail regressed, at any time:** end without a win (below). -- **Less than 2 full weeks of data:** keep it running, even if the primary metric looks significant. Report progress only. -- **2 weeks or more, significant positive lift:** the experiment is proven. First set the record to "won, guarded rollout pending", so that a later check-in can tell the win apart from a loss. Then stop the iteration with control as the winner, and a reason that says that the treatment won, with its lift. Stopping serves the winner to everyone that the default rule matches, so a treatment winner would reach 100% of users at once and skip the guarded rollout. Then start the guarded rollout (below), which ramps the treatment. -- **2 weeks or more, significant negative lift:** a loser. End without a win. -- **2 to 4 weeks, not significant:** keep it running. Report progress only. -- **No significance after 4 weeks:** inconclusive, need more traffic. End without a win. +**Phase 1 — Prove the experiment at 10% (gate before any rollout):** -Never declare a winner off a single data point, or before the stats engine confirms significance. +Watch for statistical significance on the primary metric: -Sequential testing keeps each check-in valid, which is what lets a guardrail regression end the experiment early. The 2-week minimum for a win is about the product, not the statistics: behavior differs between weekdays and weekends, and a new experience can get a short novelty bump. A win needs at least two full weekly cycles. +- **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. -**Ending without a win:** stop the iteration with control as the winner and a reason that says why, because stopping an iteration requires a winning treatment. Stopping serves control to everyone that the default rule matches. Then turn the flag off, so that its state shows that it is not in use. Do not archive the flag, because the code still evaluates it. Go to Step 6. +Never declare a winner off a single data point or before your stats engine confirms significance. Do not decide on a lift before the experiment has run 2 full weeks. -**Starting the guarded rollout:** +**Phase 2 — Guarded rollout ramp (only after Phase 1 proves the change):** -Before you start it, read the flag's default rule. It must serve control (false) to everyone. If it still has the experiment's split, set it to serve control first. +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. -- The winning ("on") variation as the test, the original as control, and the same randomization unit as the experiment. -- Exactly 3 monitored stages, none above 50% (for example 10% → 25% → 50%), with about 24 hours of monitoring each. LaunchDarkly rejects any step above 50%, and it promotes the rollout to 100% itself after the last stage, so do not add a 100% stage. -- The same primary and guardrail metrics as the experiment, each set to notify and roll back 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. -Set the record to "guarded rollout running". LaunchDarkly advances the stages and rolls back on its own, so later check-ins only read its status. +Once the final stage completes cleanly and auto-promotes to 100%, declare a winner and go to the Step 8 report. -**Won, guarded rollout pending:** the iteration is stopped and the treatment won. Start the guarded rollout (above). +**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. -**Guarded rollout running:** +**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. -- **Still in a stage:** report the stage. -- **Rolled back or stopped:** the rollout failed. Confirm that everyone gets control, and go to Step 6. -- **Reached 100%:** a winner. Go to Step 6. +## Step 8: Report -**Retrying after a rollback:** a rollback can come from an unrelated application error during the ramp, not from the change. Retry only when the person asks, and only after these checks: - -1. The flag is back to 100% control, with nothing stuck mid-rollout. -2. The change history shows the timing between "advanced to next stage" and "reverted". A rollback within seconds of advancing does not fit a regression over a full metric window, so it points to an external cause. -3. The person confirms that the external cause is fixed. - -Then restart the guarded rollout from the start with the same settings. Never retry a rollback that the metrics caused. - -## Step 6: Report - -When an experiment reaches an outcome (a winner at 100%, a loser, inconclusive, or a failed rollout), produce this report in the same reply, and set the record to "report produced": +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] +## Experiment Report: [Goal Description] **Date:** [YYYY-MM-DD] **Goal:** [metric] [direction] by [target]% -**Status:** [achieved / not achieved / inconclusive] +**Status:** [achieved / in progress / stalled] ### Hypothesis [What we tried and why] -### Change -- Flag: [flag key] -- Change: [as described in the handoff] -- Deployed: [who confirmed, and when] +### Implementation +- Flag: [flag_key] +- Files modified: [list] +- Branch: [branch name] -### Experiment (10% of traffic) -- Status: [proven / loser / inconclusive / stopped for a guardrail] +### 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: [control] → [treatment] ([+/-]%) +- Metric change: [before] → [after] ([+/-]%) - Statistical significance: [value, confidence interval] -### Guarded rollout (if reached) -- Status: [completed / rolled back / not started] +### Guarded Rollout Phase (if reached) +- Status: [completed / rolled_back / not started] - Duration: [time] -- Stages reached: [N of 3] -- If rolled back and retried: [cause, and the result of the retry] +- 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]) -- [Other guardrails]: [baseline] → [final] ([status]) +### Safety Metrics +- error-rate: [baseline] → [final] ([no regression / regression detected]) +- [other guardrails]: [baseline] → [final] ([status]) -### Next steps -[What to do next. For a winner at 100%, recommend a cleanup change that removes the flag from the code and keeps the winning path, and then archiving the flag. For an experiment that did not win, recommend removing the flag from the code and keeping the current behavior, and then archiving the flag.] +### Next Steps +[What to do next based on the outcome] ``` -## Safety rules - -- Present the hypothesis, and get the person's agreement, before you create anything. -- Never treat a change as deployed without a person's confirmation. -- Always prove a change with a fixed 10% experiment before any guarded rollout. -- Run an experiment for at least 2 full weeks before you decide, unless a guardrail regresses. -- Always include an error-rate guardrail, separate from the success metric. Add a performance guardrail for a performance change. -- Set every rollout metric to both notify and roll back on regression. -- Keep every guarded rollout stage at or below 50%, and let LaunchDarkly promote to 100%. -- Tell a rollback that the metrics caused (do not retry) from one with an external cause (safe to retry once it is fixed). -- Change only flags with the tag `experiment-orchestration`, and keep the record in the flag description current. +## 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. + From b31e3c3062b6f9fd651cc8f12e65c6f690e7bacb Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Wed, 28 Oct 2026 10:59:08 -0700 Subject: [PATCH 10/11] fix: remove the 2-week minimum from the experiment skill Co-Authored-By: Claude Opus 5.5 --- internal/awsdevops/skill.go | 2 +- internal/awsdevops/skills/experiment-orchestration/SKILL.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index 49dbe7223..34a90f61d 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -34,7 +34,7 @@ var ( // of the new text. Never remove a hash. var shippedSkillHashes = []string{ "57844bc3c5aca292ef88b5f949cd791206074b5cfc850ea52ebdd40020ead695", - "d67f47da9fa05d3991a07f8f22fa001e1e7285a5574974e1ac669caca9654bf4", + "9dd6c8cf7c3432faa5c444c0529998676ad6bdb94aeb4558c9581973c04535f2", } const ( diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md index 1eb5f5e54..d45546fe3 100644 --- a/internal/awsdevops/skills/experiment-orchestration/SKILL.md +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -94,7 +94,7 @@ Watch for statistical significance on the primary metric: - **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. Do not decide on a lift before the experiment has run 2 full weeks. +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):** From f9893d2eecbd5c2bca648ef767d5f156d9e5d276 Mon Sep 17 00:00:00 2001 From: Christina Chu Date: Wed, 28 Oct 2026 11:13:19 -0700 Subject: [PATCH 11/11] fix: remove the extra blank line at the end of the experiment skill Co-Authored-By: Claude Opus 5.5 --- internal/awsdevops/skill.go | 2 +- internal/awsdevops/skills/experiment-orchestration/SKILL.md | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/awsdevops/skill.go b/internal/awsdevops/skill.go index 34a90f61d..959e1a067 100644 --- a/internal/awsdevops/skill.go +++ b/internal/awsdevops/skill.go @@ -34,7 +34,7 @@ var ( // of the new text. Never remove a hash. var shippedSkillHashes = []string{ "57844bc3c5aca292ef88b5f949cd791206074b5cfc850ea52ebdd40020ead695", - "9dd6c8cf7c3432faa5c444c0529998676ad6bdb94aeb4558c9581973c04535f2", + "c4c4e12aeff900402dc17ff370bc1e78eadcb34abac5b256319c7cd793bd73f3", } const ( diff --git a/internal/awsdevops/skills/experiment-orchestration/SKILL.md b/internal/awsdevops/skills/experiment-orchestration/SKILL.md index d45546fe3..bf816eb50 100644 --- a/internal/awsdevops/skills/experiment-orchestration/SKILL.md +++ b/internal/awsdevops/skills/experiment-orchestration/SKILL.md @@ -169,4 +169,3 @@ Runs automatically the instant Step 7 reaches a terminal outcome (winner + auto- - **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. -