diff --git a/cmd/gh-aw/main.go b/cmd/gh-aw/main.go index 9ae1ed08df3..ed20a246648 100644 --- a/cmd/gh-aw/main.go +++ b/cmd/gh-aw/main.go @@ -553,7 +553,7 @@ func runCompileCmd(cmd *cobra.Command, args []string) error { } type commandSet struct { - addCmd, addWizardCmd, updateCmd, deployCmd, trialCmd, initCmd, statusCmd, listCmd *cobra.Command + addCmd, addWizardCmd, editCmd, updateCmd, deployCmd, trialCmd, initCmd, statusCmd, listCmd *cobra.Command mcpCmd, logsCmd, auditCmd, viewCmd, healthCmd, outcomesCmd, mcpServerCmd, prCmd, secretsCmd *cobra.Command fixCmd, upgradeCmd, completionCmd, hashCmd, projectCmd, doctorCmd, checksCmd, validateCmd, lintCmd *cobra.Command domainsCmd, experimentsCmd, forecastCmd, gradersCmd, modelsCmd, envCmd *cobra.Command @@ -708,6 +708,7 @@ func createCommandSet() commandSet { cmds := commandSet{ addCmd: cli.NewAddCommand(validateEngine), addWizardCmd: cli.NewAddWizardCommand(validateEngine), + editCmd: cli.NewEditCommand(), updateCmd: cli.NewUpdateCommand(validateEngine), deployCmd: cli.NewDeployCommand(validateEngine), trialCmd: cli.NewTrialCommand(validateEngine), @@ -845,7 +846,7 @@ func configureOtherCommandFlags() { func assignCommandGroups(cmds commandSet) { cmds.initCmd.GroupID, newCmd.GroupID, cmds.addCmd.GroupID, cmds.addWizardCmd.GroupID = "setup", "setup", "setup", "setup" - removeCmd.GroupID, cmds.updateCmd.GroupID, cmds.deployCmd.GroupID, cmds.upgradeCmd.GroupID = "setup", "setup", "setup", "setup" + removeCmd.GroupID, cmds.editCmd.GroupID, cmds.updateCmd.GroupID, cmds.deployCmd.GroupID, cmds.upgradeCmd.GroupID = "setup", "setup", "setup", "setup", "setup" cmds.secretsCmd.GroupID, cmds.envCmd.GroupID, cmds.doctorCmd.GroupID = "setup", "setup", "setup" compileCmd.GroupID, cmds.validateCmd.GroupID, cmds.lintCmd.GroupID = "development", "development", "development" cmds.mcpCmd.GroupID, cmds.fixCmd.GroupID, cmds.domainsCmd.GroupID = "development", "development", "development" @@ -859,7 +860,7 @@ func assignCommandGroups(cmds commandSet) { func addCommandsToRoot(cmds commandSet) { rootCmd.AddCommand( - compileCmd, cmds.addCmd, cmds.addWizardCmd, cmds.updateCmd, cmds.deployCmd, cmds.upgradeCmd, cmds.trialCmd, newCmd, cmds.initCmd, + compileCmd, cmds.addCmd, cmds.addWizardCmd, cmds.editCmd, cmds.updateCmd, cmds.deployCmd, cmds.upgradeCmd, cmds.trialCmd, newCmd, cmds.initCmd, runCmd, removeCmd, cmds.statusCmd, cmds.listCmd, enableCmd, disableCmd, cmds.logsCmd, cmds.auditCmd, cmds.viewCmd, cmds.healthCmd, cmds.outcomesCmd, cmds.checksCmd, cmds.mcpCmd, cmds.mcpServerCmd, cmds.prCmd, versionCmd, cmds.secretsCmd, cmds.fixCmd, cmds.validateCmd, cmds.lintCmd, cmds.completionCmd, cmds.hashCmd, cmds.projectCmd, cmds.doctorCmd, diff --git a/docs/adr/55475-schema-validated-workflow-frontmatter-edit-command.md b/docs/adr/55475-schema-validated-workflow-frontmatter-edit-command.md new file mode 100644 index 00000000000..4b2cc7cc3e2 --- /dev/null +++ b/docs/adr/55475-schema-validated-workflow-frontmatter-edit-command.md @@ -0,0 +1,45 @@ +# ADR-55475: Schema-Validated Workflow Frontmatter Edit Command + +**Date**: 2026-08-24 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +Workflow definitions in this project are Markdown files with YAML frontmatter that controls execution parameters (e.g., `max-turns`, `model`, `on.schedule`, `imports`). To change any of these parameters today, users must manually edit the raw Markdown file and then separately run `gh aw compile` to regenerate the corresponding `.lock.yml` file. Manual editing bypasses schema validation entirely, meaning invalid frontmatter values are only detected at compile time—after the file has already been written to disk. The need for an explicit, validated, and atomic path for mutating workflow frontmatter is the driving problem this PR addresses. + +### Decision + +We will add a new `gh aw edit` CLI command (`pkg/cli/edit_command.go`) that provides schema-validated, programmatic mutation of workflow frontmatter with automatic recompilation. The command accepts typed flag-based mutations (`--set`, `--unset`, `--add`, `--remove`, `--schedule`, `--add-import`, `--add-skill`) and a positional `path: value` shorthand. It validates the resulting frontmatter against the workflow schema before writing and immediately recompiles the `.lock.yml`; on compilation failure it rolls back both files to their prior state. + +### Alternatives Considered + +#### Alternative 1: Direct file editing + manual compile + +Users continue to edit YAML frontmatter by hand and run `gh aw compile` separately. This requires no new code but provides no schema validation at edit time, allows invalid frontmatter to be committed before compile, and leaves the lock file in an inconsistent state when a user forgets to recompile. It was rejected because it does not address the safety problem. + +#### Alternative 2: Extend `gh aw update` with frontmatter mutation flags + +Add mutation flags to the existing `update` command. The `update` command is semantically about syncing a workflow from a `source:` declaration. Mixing configuration mutation into the same command would create a confusing API where the same command both fetches external content and edits local state. It was rejected to preserve the clarity of the existing command model. + +### Consequences + +#### Positive +- Schema validation runs before any bytes are written to disk, preventing invalid frontmatter values from ever reaching the repository. +- Automatic recompilation keeps `.lock.yml` atomically in sync with the edited workflow file. +- Transactional rollback: if recompilation fails, both the workflow source file and the lock file are restored to their pre-edit state. +- Fuzzy schedule expressions (`daily`, `every 6h`, `daily on weekdays`, etc.) are validated with the shared schedule parser at edit time, providing a user-friendly schedule API. + +#### Negative +- Source-managed workflows (those with a `source:` key in their frontmatter) are explicitly rejected by the command; users must edit the upstream source or pin/unpin and then run `gh aw update` instead. +- YAML re-serialization via `go-yaml` may alter key ordering, indentation, comments, or whitespace in the frontmatter beyond the intended change, potentially producing noisy diffs. Edits that change nothing are detected and skip writing entirely, so no-op edits never rewrite a workflow. + +#### Neutral +- The command is labelled "Experimental" in its `Short` and `Long` help text, signalling that its interface may change before stabilization. +- The command is registered in the `setup` group alongside `add`, `update`, and `remove`, maintaining consistency with the existing command taxonomy. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/edit_command.go b/pkg/cli/edit_command.go new file mode 100644 index 00000000000..0c40024b85e --- /dev/null +++ b/pkg/cli/edit_command.go @@ -0,0 +1,434 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "reflect" + "slices" + "strings" + + "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/stringutil" + "github.com/github/gh-aw/pkg/workflow" + "github.com/goccy/go-yaml" + "github.com/spf13/cobra" +) + +// NewEditCommand creates the experimental command for changing workflow frontmatter. +func NewEditCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "edit [path: value]", + Short: "Experimental: edit workflow frontmatter and recompile", + Long: `Experimental: edit schema-validated workflow frontmatter and recompile its generated file. + +The workflow-id may be a workflow name, a Markdown filename, or a path. Changes are +validated before writing. Workflows managed by a source: declaration cannot be edited. + +Edits that change nothing leave the workflow untouched. When frontmatter does change it is +re-serialized, so YAML comments, key ordering, and quoting styles are not preserved.`, + Example: ` gh aw edit repo-assist "max-turns: 20" + gh aw edit repo-assist --schedule "every 6h" + gh aw edit repo-assist --set model=small --unset engine.model + gh aw edit repo-assist --add-import shared/common.md + gh aw edit repo-assist --add-skill shared/review`, + Args: cobra.RangeArgs(1, 2), + RunE: runEditCommand, + } + cmd.Flags().StringArray("set", nil, "Set a frontmatter path (path=value)") + cmd.Flags().StringArray("unset", nil, "Remove a frontmatter path") + cmd.Flags().StringArray("add", nil, "Append a value to a list (path=value)") + cmd.Flags().StringArray("remove", nil, "Remove a value from a list (path=value)") + cmd.Flags().StringArray("add-import", nil, "Append a workflow import path") + cmd.Flags().StringArray("remove-import", nil, "Remove a workflow import path") + cmd.Flags().StringArray("add-skill", nil, "Append a workflow skill") + cmd.Flags().StringArray("remove-skill", nil, "Remove a workflow skill") + cmd.Flags().String("schedule", "", "Set a schedule using a fuzzy schedule or cron expression; use off to remove it") + cmd.Flags().Bool("dry-run", false, "Validate changes without writing or compiling") + cmd.Flags().StringP("dir", "d", "", "Workflow directory (default: $GH_AW_WORKFLOWS_DIR or .github/workflows)") + cmd.ValidArgsFunction = CompleteWorkflowNames + RegisterDirFlagCompletion(cmd, "dir") + return cmd +} + +func runEditCommand(cmd *cobra.Command, args []string) error { + workflowPath, err := resolveWorkflowFileInDir(args[0], false, editFlagString(cmd, "dir")) + if err != nil { + return err + } + content, err := os.ReadFile(workflowPath) + if err != nil { + return fmt.Errorf("read workflow: %w", err) + } + parsed, err := parser.ExtractFrontmatterFromContent(string(content)) + if err != nil { + return err + } + if _, managed := parsed.Frontmatter["source"]; managed { + return errors.New("cannot edit a source-managed workflow; update its source or pin, then run gh aw update") + } + + changes, err := editChangesFromCommand(cmd, args) + if err != nil { + return err + } + if len(changes) == 0 { + return errors.New("provide an assignment or an edit flag") + } + edited := false + for _, change := range changes { + applied, err := applyEditChange(parsed.Frontmatter, change) + if err != nil { + return err + } + edited = edited || applied + } + if !edited { + fmt.Fprintln(cmd.OutOrStdout(), "workflow already matches the requested changes") + return nil + } + if err := parser.ValidateMainWorkflowFrontmatterWithSchemaAndLocation(parsed.Frontmatter, workflowPath); err != nil { + return fmt.Errorf("invalid edited workflow: %w", err) + } + updated, err := replaceFrontmatter(string(content), parsed.Frontmatter) + if err != nil { + return err + } + if editFlagBool(cmd, "dry-run") { + fmt.Fprint(cmd.OutOrStdout(), updated) + return nil + } + + return writeAndCompileEditedWorkflow(cmd.Context(), workflowPath, content, updated) +} + +func writeAndCompileEditedWorkflow(ctx context.Context, workflowPath string, content []byte, updated string) error { + lockPath := stringutil.MarkdownToLockFile(workflowPath) + previousLock, lockErr := os.ReadFile(lockPath) + lockExisted := lockErr == nil + if lockErr != nil && !os.IsNotExist(lockErr) { + return fmt.Errorf("read generated lock file: %w", lockErr) + } + if err := writeFileAtomically(workflowPath, []byte(updated)); err != nil { + return fmt.Errorf("write workflow: %w", err) + } + if err := compileWorkflow(ctx, workflowPath, false, true, ""); err != nil { + return errors.Join(fmt.Errorf("compile edited workflow: %w", err), restoreEditedWorkflow(workflowPath, content, lockPath, previousLock, lockExisted)) + } + return nil +} + +// restoreEditedWorkflow puts the workflow and its generated file back to their pre-edit state. +func restoreEditedWorkflow(workflowPath string, content []byte, lockPath string, previousLock []byte, lockExisted bool) error { + var errs []error + if err := writeFileAtomically(workflowPath, content); err != nil { + errs = append(errs, fmt.Errorf("restore workflow: %w", err)) + } + if lockExisted { + if err := writeFileAtomically(lockPath, previousLock); err != nil { + errs = append(errs, fmt.Errorf("restore generated lock file: %w", err)) + } + } else if err := os.Remove(lockPath); err != nil && !os.IsNotExist(err) { + errs = append(errs, fmt.Errorf("remove generated lock file: %w", err)) + } + return errors.Join(errs...) +} + +// writeFileAtomically writes content through a sibling temporary file so a failed +// write never truncates the destination. +func writeFileAtomically(path string, content []byte) error { + file, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".*") + if err != nil { + return err + } + tempPath := file.Name() + defer func() { _ = os.Remove(tempPath) }() + if _, err := file.Write(content); err != nil { + _ = file.Close() + return err + } + if err := file.Close(); err != nil { + return err + } + mode := os.FileMode(0o644) + if info, err := os.Stat(path); err == nil { + mode = info.Mode().Perm() + } + if err := os.Chmod(tempPath, mode); err != nil { + return err + } + return os.Rename(tempPath, path) +} + +type editChange struct { + kind, path string + value any +} + +func editChangesFromCommand(cmd *cobra.Command, args []string) ([]editChange, error) { + var changes []editChange + if len(args) == 2 { + change, err := parseEditAssignment(args[1], ":") + if err != nil { + return nil, err + } + changes = append(changes, change) + } + for _, name := range []string{"set", "add", "remove"} { + for _, assignment := range editFlagStrings(cmd, name) { + change, err := parseEditAssignment(assignment, "=") + if err != nil { + return nil, err + } + change.kind = name + changes = append(changes, change) + } + } + for _, path := range editFlagStrings(cmd, "unset") { + changes = append(changes, editChange{kind: "unset", path: path}) + } + for _, importPath := range editFlagStrings(cmd, "add-import") { + changes = append(changes, editChange{kind: "add", path: "imports", value: importPath}) + } + for _, importPath := range editFlagStrings(cmd, "remove-import") { + changes = append(changes, editChange{kind: "remove", path: "imports", value: importPath}) + } + for _, skill := range editFlagStrings(cmd, "add-skill") { + changes = append(changes, editChange{kind: "add", path: "skills", value: skill}) + } + for _, skill := range editFlagStrings(cmd, "remove-skill") { + changes = append(changes, editChange{kind: "remove", path: "skills", value: skill}) + } + if schedule := editFlagString(cmd, "schedule"); schedule != "" { + change, err := scheduleChange(schedule) + if err != nil { + return nil, err + } + changes = append(changes, change) + } + return changes, nil +} + +func scheduleChange(schedule string) (editChange, error) { + schedule = strings.TrimSpace(schedule) + if strings.EqualFold(schedule, "off") { + return editChange{kind: "unset", path: "on.schedule"}, nil + } + _, _, err := parser.ParseSchedule(schedule) + if err != nil { + return editChange{}, fmt.Errorf("invalid schedule: %w", err) + } + return editChange{kind: "set", path: "on.schedule", value: schedule}, nil +} + +func parseEditAssignment(assignment, separator string) (editChange, error) { + path, rawValue, ok := strings.Cut(assignment, separator) + path, rawValue = strings.TrimSpace(path), strings.TrimSpace(rawValue) + if !ok || path == "" || rawValue == "" { + return editChange{}, fmt.Errorf("invalid assignment %q; expected path%svalue", assignment, separator) + } + var value map[string]any + if err := yaml.Unmarshal([]byte("value: "+rawValue), &value); err != nil { + return editChange{}, fmt.Errorf("parse value for %q: %w", path, err) + } + return editChange{kind: "set", path: path, value: value["value"]}, nil +} + +// applyEditChange applies a change and reports whether the frontmatter was modified. +func applyEditChange(frontmatter map[string]any, change editChange) (bool, error) { + change, err := normalizeEditChange(frontmatter, change) + if err != nil { + return false, err + } + parent, key, err := editChangeParent(frontmatter, change.path, change.kind != "unset") + if err != nil { + return false, err + } + if parent == nil { + // The target does not exist, so an unset is a no-op and the original + // frontmatter representation is preserved. + return false, nil + } + return applyEditChangeToParent(parent, key, change) +} + +func normalizeEditChange(frontmatter map[string]any, change editChange) (editChange, error) { + if change.kind == "set" && change.path == "on.schedule" { + if schedule, ok := change.value.(string); ok { + var err error + change, err = scheduleChange(schedule) + if err != nil { + return editChange{}, err + } + } + } + if change.path == "imports" && (change.kind == "add" || change.kind == "remove") { + if _, objectImports := frontmatter["imports"].(map[string]any); objectImports { + change.path = "imports.aw" + } + } + return change, nil +} + +// editChangeParent walks changePath and returns the map holding its last segment. +// When create is false, a missing or non-object ancestor yields a nil parent so the +// caller can treat the change as a no-op instead of rewriting the frontmatter. +func editChangeParent(frontmatter map[string]any, changePath string, create bool) (map[string]any, string, error) { + path := strings.Split(changePath, ".") + if slices.Contains(path, "") { + return nil, "", fmt.Errorf("invalid frontmatter path %q", changePath) + } + parent := frontmatter + for _, part := range path[:len(path)-1] { + child, ok := parent[part].(map[string]any) + if !ok { + if !create { + return nil, "", nil + } + if parent[part] == nil { + child = map[string]any{} + } else if part == "on" { + expanded, err := expandTriggers(parent[part]) + if err != nil { + return nil, "", fmt.Errorf("cannot edit %q: %w", changePath, err) + } + child = expanded + } else { + return nil, "", fmt.Errorf("cannot edit %q because %q is not an object", changePath, part) + } + parent[part] = child + } + parent = child + } + return parent, path[len(path)-1], nil +} + +// expandTriggers converts the shorthand forms accepted by the on: field into the +// equivalent trigger object so a trigger can be edited without changing semantics. +func expandTriggers(triggers any) (map[string]any, error) { + switch value := triggers.(type) { + case map[string]any: + return value, nil + case string: + return expandTriggerString(value) + case []any: + expanded := make(map[string]any, len(value)) + for _, trigger := range value { + name, ok := trigger.(string) + if !ok { + return nil, errors.New("on contains a non-string trigger") + } + expanded[name] = nil + } + return expanded, nil + } + return nil, fmt.Errorf("on is not a trigger object, got %T", triggers) +} + +func expandTriggerString(triggers string) (map[string]any, error) { + triggers = strings.TrimSpace(triggers) + if _, _, err := parser.ParseSchedule(triggers); err == nil { + return map[string]any{"schedule": triggers, "workflow_dispatch": nil}, nil + } + trigger, err := workflow.ParseTriggerShorthand(triggers) + if err != nil { + return nil, fmt.Errorf("on shorthand %q is not a recognized trigger: %w", triggers, err) + } + if trigger != nil { + if len(trigger.Conditions) > 0 { + return nil, fmt.Errorf("expand the on shorthand %q into its object form before editing it", triggers) + } + return trigger.ToYAMLMap(), nil + } + if strings.ContainsAny(triggers, " \t/") { + return nil, fmt.Errorf("expand the on shorthand %q into its object form before editing it", triggers) + } + return map[string]any{triggers: nil}, nil +} + +// applyEditChangeToParent applies a change to parent and reports whether it modified it. +func applyEditChangeToParent(parent map[string]any, key string, change editChange) (bool, error) { + switch change.kind { + case "set": + existing, exists := parent[key] + if exists && reflect.DeepEqual(existing, change.value) { + return false, nil + } + parent[key] = change.value + case "unset": + if _, exists := parent[key]; !exists { + return false, nil + } + delete(parent, key) + case "add": + values, ok := parent[key].([]any) + if !ok && parent[key] != nil { + return false, fmt.Errorf("cannot add to %q because it is not a list", change.path) + } + if slices.ContainsFunc(values, func(value any) bool { return reflect.DeepEqual(value, change.value) }) { + return false, nil + } + parent[key] = append(values, change.value) + case "remove": + values, ok := parent[key].([]any) + if !ok { + return false, fmt.Errorf("cannot remove from %q because it is not a list", change.path) + } + remaining := slices.DeleteFunc(slices.Clone(values), func(value any) bool { return reflect.DeepEqual(value, change.value) }) + if len(remaining) == len(values) { + return false, nil + } + parent[key] = remaining + default: + return false, fmt.Errorf("unsupported edit operation %q", change.kind) + } + return true, nil +} + +func replaceFrontmatter(content string, frontmatter map[string]any) (string, error) { + encoded, err := yaml.MarshalWithOptions(frontmatter, yaml.Indent(2), yaml.IndentSequence(true)) + if err != nil { + return "", fmt.Errorf("encode frontmatter: %w", err) + } + firstLineEnd := strings.IndexByte(content, '\n') + if firstLineEnd < 0 || strings.TrimSpace(content[:firstLineEnd]) != "---" { + return "", errors.New("workflow must begin with YAML frontmatter") + } + bodyStart := firstLineEnd + 1 + for lineStart := bodyStart; lineStart <= len(content); { + lineEnd := strings.IndexByte(content[lineStart:], '\n') + if lineEnd < 0 { + lineEnd = len(content) + } else { + lineEnd += lineStart + } + if strings.TrimSpace(content[lineStart:lineEnd]) == "---" { + if lineEnd < len(content) { + lineEnd++ + } + return "---\n" + string(encoded) + "---\n" + content[lineEnd:], nil + } + if lineEnd == len(content) { + break + } + lineStart = lineEnd + 1 + } + return "", errors.New("frontmatter not properly closed") +} + +func editFlagString(cmd *cobra.Command, name string) string { + value, _ := cmd.Flags().GetString(name) + return value +} + +func editFlagStrings(cmd *cobra.Command, name string) []string { + value, _ := cmd.Flags().GetStringArray(name) + return value +} + +func editFlagBool(cmd *cobra.Command, name string) bool { + value, _ := cmd.Flags().GetBool(name) + return value +} diff --git a/pkg/cli/edit_command_integration_test.go b/pkg/cli/edit_command_integration_test.go new file mode 100644 index 00000000000..ec46b879935 --- /dev/null +++ b/pkg/cli/edit_command_integration_test.go @@ -0,0 +1,137 @@ +//go:build integration + +package cli + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/github/gh-aw/pkg/parser" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEditCommandIntegrationMutations(t *testing.T) { + setup := setupIntegrationTest(t) + defer setup.cleanup() + + workflowPath := filepath.Join(setup.workflowsDir, "edit.md") + require.NoError(t, os.MkdirAll(filepath.Join(setup.workflowsDir, "shared"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(setup.workflowsDir, "shared", "base.md"), []byte("# Base\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(setup.workflowsDir, "shared", "extra.md"), []byte("# Extra\n"), 0o644)) + require.NoError(t, os.WriteFile(workflowPath, []byte(editIntegrationWorkflow), 0o644)) + + requireEditSucceeds(t, setup, "edit", "edit", "max-turns: 20") + requireEditSucceeds(t, setup, "edit", "edit.md", "--schedule", "daily on weekdays") + requireEditSucceeds(t, setup, "edit", workflowPath, "--add-import", "shared/extra.md") + requireEditSucceeds(t, setup, "edit", "edit", "--remove-import", "shared/base.md") + requireEditSucceeds(t, setup, "edit", "edit", "--add-skill", "owner/repo/skills/review@0123456789012345678901234567890123456789") + requireEditSucceeds(t, setup, "edit", "edit", "--remove-skill", "owner/repo/skills/base@0123456789012345678901234567890123456789") + requireEditSucceeds(t, setup, "edit", "edit", "--set", "model=small", "--unset", "description") + requireEditSucceeds(t, setup, "edit", "edit", "--add", "labels=one", "--remove", "labels=base") + + content, err := os.ReadFile(workflowPath) + require.NoError(t, err) + frontmatter, err := parser.ExtractFrontmatterFromContent(string(content)) + require.NoError(t, err) + assert.Equal(t, uint64(20), frontmatter.Frontmatter["max-turns"]) + assert.Equal(t, "small", frontmatter.Frontmatter["model"]) + assert.NotContains(t, frontmatter.Frontmatter, "description") + assert.Equal(t, []any{"shared/extra.md"}, frontmatter.Frontmatter["imports"]) + assert.Equal(t, []any{"owner/repo/skills/review@0123456789012345678901234567890123456789"}, frontmatter.Frontmatter["skills"]) + assert.Equal(t, []any{"one"}, frontmatter.Frontmatter["labels"]) + assert.Equal(t, map[string]any{ + "schedule": "daily on weekdays", + "workflow_dispatch": nil, + }, frontmatter.Frontmatter["on"]) + + lockContent, err := os.ReadFile(filepath.Join(setup.workflowsDir, "edit.lock.yml")) + require.NoError(t, err) + assert.Contains(t, string(lockContent), "Edit Integration Workflow") +} + +func TestEditCommandIntegrationDryRunAndFailureSafety(t *testing.T) { + setup := setupIntegrationTest(t) + defer setup.cleanup() + + workflowPath := filepath.Join(setup.workflowsDir, "edit.md") + require.NoError(t, os.WriteFile(workflowPath, []byte(editIntegrationWorkflow), 0o644)) + original, err := os.ReadFile(workflowPath) + require.NoError(t, err) + + output := requireEditSucceeds(t, setup, "edit", "edit", "--set", "max-turns=20", "--dry-run") + assert.Contains(t, output, "max-turns: 20") + content, err := os.ReadFile(workflowPath) + require.NoError(t, err) + assert.Equal(t, original, content) + _, err = os.Stat(filepath.Join(setup.workflowsDir, "edit.lock.yml")) + assert.ErrorIs(t, err, os.ErrNotExist) + + requireEditSucceeds(t, setup, "edit", "edit", "--set", "max-turns=10") + beforeFailure, err := os.ReadFile(workflowPath) + require.NoError(t, err) + lockPath := filepath.Join(setup.workflowsDir, "edit.lock.yml") + beforeFailureLock, err := os.ReadFile(lockPath) + require.NoError(t, err) + + output = requireEditFails(t, setup, "edit", "edit", "--add-import", "shared/missing.md") + assert.Contains(t, output, "compile edited workflow") + content, err = os.ReadFile(workflowPath) + require.NoError(t, err) + assert.Equal(t, beforeFailure, content) + lockContent, err := os.ReadFile(lockPath) + require.NoError(t, err) + assert.Equal(t, beforeFailureLock, lockContent) + + shorthandPath := filepath.Join(setup.workflowsDir, "shorthand.md") + shorthand := "---\n# keep this comment\non: push\nengine: copilot\n---\n# Shorthand Workflow\n" + require.NoError(t, os.WriteFile(shorthandPath, []byte(shorthand), 0o644)) + output = requireEditSucceeds(t, setup, "edit", "shorthand", "--schedule", "off") + assert.Contains(t, output, "already matches") + content, err = os.ReadFile(shorthandPath) + require.NoError(t, err) + assert.Equal(t, shorthand, string(content)) + + require.NoError(t, os.WriteFile(workflowPath, []byte("---\nsource: owner/repo@v1\non: workflow_dispatch\n---\n# Managed\n"), 0o644)) + beforeManaged, err := os.ReadFile(workflowPath) + require.NoError(t, err) + output = requireEditFails(t, setup, "edit", "edit", "--set", "max-turns=20") + assert.Contains(t, output, "source-managed") + content, err = os.ReadFile(workflowPath) + require.NoError(t, err) + assert.Equal(t, beforeManaged, content) +} + +func requireEditSucceeds(t *testing.T, setup *integrationTestSetup, args ...string) string { + t.Helper() + command := exec.Command(setup.binaryPath, args...) + command.Dir = setup.tempDir + output, err := command.CombinedOutput() + require.NoError(t, err, "gh aw %v failed:\n%s", args, output) + return string(output) +} + +func requireEditFails(t *testing.T, setup *integrationTestSetup, args ...string) string { + t.Helper() + command := exec.Command(setup.binaryPath, args...) + command.Dir = setup.tempDir + output, err := command.CombinedOutput() + require.Error(t, err, "gh aw %v unexpectedly succeeded:\n%s", args, output) + return string(output) +} + +const editIntegrationWorkflow = `--- +description: Edit integration workflow +labels: [base] +on: + workflow_dispatch: +skills: + - owner/repo/skills/base@0123456789012345678901234567890123456789 +engine: claude +--- +# Edit Integration Workflow + +Test workflow. +` diff --git a/pkg/cli/edit_command_test.go b/pkg/cli/edit_command_test.go new file mode 100644 index 00000000000..a227dc80513 --- /dev/null +++ b/pkg/cli/edit_command_test.go @@ -0,0 +1,216 @@ +package cli + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEditChangesSupportAssignmentsSchedulesAndImports(t *testing.T) { + t.Parallel() + cmd := NewEditCommand() + require.NoError(t, cmd.Flags().Set("set", "max-turns=20")) + require.NoError(t, cmd.Flags().Set("schedule", "every 6h")) + require.NoError(t, cmd.Flags().Set("add-import", "shared/common.md")) + require.NoError(t, cmd.Flags().Set("add-skill", "shared/review")) + + changes, err := editChangesFromCommand(cmd, []string{"workflow", "model: small"}) + require.NoError(t, err) + frontmatter := map[string]any{"on": "workflow_dispatch"} + for _, change := range changes { + mustApplyEditChange(t, frontmatter, change) + } + + assert.Equal(t, "small", frontmatter["model"]) + assert.Equal(t, uint64(20), frontmatter["max-turns"]) + assert.Equal(t, []any{"shared/common.md"}, frontmatter["imports"]) + assert.Equal(t, []any{"shared/review"}, frontmatter["skills"]) + assert.Equal(t, map[string]any{"workflow_dispatch": nil, "schedule": "every 6h"}, frontmatter["on"]) +} + +func TestEditCommandDryRunPreservesWorkflowFile(t *testing.T) { + t.Parallel() + dir := t.TempDir() + workflowPath := dir + "/workflow.md" + original := "---\non: workflow_dispatch\n---\n# Workflow\n" + require.NoError(t, os.WriteFile(workflowPath, []byte(original), 0o644)) + + cmd := NewEditCommand() + cmd.SetArgs([]string{workflowPath, "max-turns: 20", "--dry-run"}) + var output strings.Builder + cmd.SetOut(&output) + require.NoError(t, cmd.Execute()) + + assert.Contains(t, output.String(), "max-turns: 20") + content, err := os.ReadFile(workflowPath) + require.NoError(t, err) + assert.Equal(t, original, string(content)) +} + +func TestEditChangesAddImportsToObjectForm(t *testing.T) { + t.Parallel() + frontmatter := map[string]any{"imports": map[string]any{"aw": []any{"shared/base.md"}}} + mustApplyEditChange(t, frontmatter, editChange{kind: "add", path: "imports", value: "shared/extra.md"}) + mustApplyEditChange(t, frontmatter, editChange{kind: "remove", path: "imports", value: "shared/base.md"}) + assert.Equal(t, []any{"shared/extra.md"}, frontmatter["imports"].(map[string]any)["aw"]) +} + +func TestEditChangesRemoveImportsAndSkills(t *testing.T) { + t.Parallel() + cmd := NewEditCommand() + require.NoError(t, cmd.Flags().Set("remove-import", "shared/base.md")) + require.NoError(t, cmd.Flags().Set("remove-skill", "shared/base")) + + changes, err := editChangesFromCommand(cmd, []string{"workflow"}) + require.NoError(t, err) + frontmatter := map[string]any{ + "imports": []any{"shared/base.md", "shared/extra.md"}, + "skills": []any{"shared/base", "shared/review"}, + } + for _, change := range changes { + mustApplyEditChange(t, frontmatter, change) + } + + assert.Equal(t, []any{"shared/extra.md"}, frontmatter["imports"]) + assert.Equal(t, []any{"shared/review"}, frontmatter["skills"]) +} + +func TestEditAssignmentParsesScheduleShorthands(t *testing.T) { + t.Parallel() + change, err := parseEditAssignment("on.schedule: daily on weekdays", ":") + require.NoError(t, err) + frontmatter := map[string]any{"on": "workflow_dispatch"} + mustApplyEditChange(t, frontmatter, change) + assert.Equal(t, "daily on weekdays", frontmatter["on"].(map[string]any)["schedule"]) +} + +func TestReplaceFrontmatterPreservesBodySeparators(t *testing.T) { + t.Parallel() + content := "---\non: workflow_dispatch\n---\n# Workflow\n\n---\nBody\n" + updated, err := replaceFrontmatter(content, map[string]any{"on": "push"}) + require.NoError(t, err) + assert.Contains(t, updated, "---\n# Workflow\n\n---\nBody\n") +} + +func TestEditCommandRejectsSourceManagedWorkflow(t *testing.T) { + t.Parallel() + dir := t.TempDir() + workflowPath := dir + "/workflow.md" + require.NoError(t, os.WriteFile(workflowPath, []byte("---\nsource: owner/repo@v1\non: workflow_dispatch\n---\n"), 0o644)) + + cmd := NewEditCommand() + cmd.SetArgs([]string{workflowPath, "max-turns: 20"}) + assert.ErrorContains(t, cmd.Execute(), "source-managed") +} + +// mustApplyEditChange applies a change and asserts that it modified the frontmatter. +func mustApplyEditChange(t *testing.T, frontmatter map[string]any, change editChange) { + t.Helper() + applied, err := applyEditChange(frontmatter, change) + require.NoError(t, err) + assert.True(t, applied, "expected the change to modify the frontmatter") +} + +func TestEditChangeKeepsShorthandTriggersWhenScheduleIsAbsent(t *testing.T) { + t.Parallel() + for name, triggers := range map[string]any{ + "string": "push", + "list": []any{"push", "workflow_dispatch"}, + "map": map[string]any{"push": nil}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + frontmatter := map[string]any{"on": triggers} + applied, err := applyEditChange(frontmatter, editChange{kind: "unset", path: "on.schedule"}) + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, triggers, frontmatter["on"]) + }) + } +} + +func TestEditChangeExpandsScheduleAndTriggerShorthands(t *testing.T) { + t.Parallel() + frontmatter := map[string]any{"on": "daily"} + mustApplyEditChange(t, frontmatter, editChange{kind: "set", path: "on.schedule", value: "every 6h"}) + assert.Equal(t, map[string]any{"schedule": "every 6h", "workflow_dispatch": nil}, frontmatter["on"]) + + frontmatter = map[string]any{"on": []any{"push", "workflow_dispatch"}} + mustApplyEditChange(t, frontmatter, editChange{kind: "set", path: "on.schedule", value: "every 6h"}) + assert.Equal(t, map[string]any{"push": nil, "workflow_dispatch": nil, "schedule": "every 6h"}, frontmatter["on"]) +} + +func TestEditChangeRejectsUnexpandableTriggerShorthand(t *testing.T) { + t.Parallel() + frontmatter := map[string]any{"on": "/bot"} + _, err := applyEditChange(frontmatter, editChange{kind: "set", path: "on.schedule", value: "every 6h"}) + require.ErrorContains(t, err, "object form") + assert.Equal(t, "/bot", frontmatter["on"]) +} + +func TestEditChangesAreNoOpsWhenValuesAlreadyMatch(t *testing.T) { + t.Parallel() + frontmatter := map[string]any{"max-turns": 20, "imports": []any{"shared/common.md"}} + applied, err := applyEditChange(frontmatter, editChange{kind: "set", path: "max-turns", value: 20}) + require.NoError(t, err) + assert.False(t, applied) + + applied, err = applyEditChange(frontmatter, editChange{kind: "add", path: "imports", value: "shared/common.md"}) + require.NoError(t, err) + assert.False(t, applied) + + applied, err = applyEditChange(frontmatter, editChange{kind: "remove", path: "imports", value: "shared/other.md"}) + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, []any{"shared/common.md"}, frontmatter["imports"]) +} + +func TestEditCommandLeavesWorkflowUnchangedForNoOpEdits(t *testing.T) { + t.Parallel() + dir := t.TempDir() + workflowPath := dir + "/workflow.md" + original := "---\n# keep this comment\non: push\n---\n# Workflow\n" + require.NoError(t, os.WriteFile(workflowPath, []byte(original), 0o644)) + + cmd := NewEditCommand() + cmd.SetArgs([]string{workflowPath, "--schedule", "off"}) + var output strings.Builder + cmd.SetOut(&output) + require.NoError(t, cmd.Execute()) + + assert.Contains(t, output.String(), "already matches") + content, err := os.ReadFile(workflowPath) + require.NoError(t, err) + assert.Equal(t, original, string(content)) +} + +func TestWriteFileAtomicallyReplacesContent(t *testing.T) { + t.Parallel() + dir := t.TempDir() + path := dir + "/workflow.md" + require.NoError(t, os.WriteFile(path, []byte("old"), 0o644)) + require.NoError(t, writeFileAtomically(path, []byte("new"))) + + content, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "new", string(content)) + + entries, err := os.ReadDir(dir) + require.NoError(t, err) + assert.Len(t, entries, 1) +} + +func TestWriteFileAtomicallyPreservesPermissions(t *testing.T) { + t.Parallel() + dir := t.TempDir() + path := dir + "/workflow.md" + require.NoError(t, os.WriteFile(path, []byte("old"), 0o600)) + require.NoError(t, writeFileAtomically(path, []byte("new"))) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) +}