diff --git a/cmd/gh-aw/main.go b/cmd/gh-aw/main.go index c54c1a1d3dc..6d7f2a264bd 100644 --- a/cmd/gh-aw/main.go +++ b/cmd/gh-aw/main.go @@ -782,7 +782,7 @@ Use "` + string(constants.CLIExtensionPrefix) + ` help all" to show help for all domainsCmd := cli.NewDomainsCommand() experimentsCmd := cli.NewExperimentsCommand() forecastCmd := cli.NewForecastCommand() - defaultsCmd := cli.NewDefaultsCommand() + envCmd := cli.NewEnvCommand() // Assign commands to groups // Setup Commands @@ -795,7 +795,7 @@ Use "` + string(constants.CLIExtensionPrefix) + ` help all" to show help for all deployCmd.GroupID = "setup" upgradeCmd.GroupID = "setup" secretsCmd.GroupID = "setup" - defaultsCmd.GroupID = "setup" + envCmd.GroupID = "setup" // Development Commands compileCmd.GroupID = "development" @@ -868,7 +868,7 @@ Use "` + string(constants.CLIExtensionPrefix) + ` help all" to show help for all rootCmd.AddCommand(domainsCmd) rootCmd.AddCommand(experimentsCmd) rootCmd.AddCommand(forecastCmd) - rootCmd.AddCommand(defaultsCmd) + rootCmd.AddCommand(envCmd) // Fix help flag descriptions for all subcommands to be consistent with the // root command ("Show help for gh aw" vs the Cobra default "help for [cmd]"). diff --git a/docs/adr/35338-env-command-default-prefix-keys-and-null-delete.md b/docs/adr/35338-env-command-default-prefix-keys-and-null-delete.md new file mode 100644 index 00000000000..356fbba87bd --- /dev/null +++ b/docs/adr/35338-env-command-default-prefix-keys-and-null-delete.md @@ -0,0 +1,98 @@ +# ADR-35338: `env` Command, `default_*` YAML Keys, Null-for-Delete, and Update Confirmation + +**Date**: 2026-05-28 +**Status**: Draft +**Deciders**: PR author (pelikhan), reviewers of PR #35338 + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +[ADR-35286](35286-compiler-managed-enterprise-env-controls.md) introduced the `gh aw defaults` command pair (`get` / `update`) backed by a flat YAML file whose keys carried a `default_` prefix (e.g. `default_max_effective_tokens`, `default_model_copilot`) that mirrored the `GH_AW_DEFAULT_*` GitHub Actions variable names. At the same time, `defaults update` performed a mutating batch operation — upserting or deleting GitHub Actions variables at repo, org, or enterprise scope — with no preview and no confirmation step, so a typo or unintended file content silently overwrote shared org-wide configuration. The original ADR's normative section did not constrain either the delete semantics or the update UX, so refining both without superseding the parent ADR is in scope. + +### Decision + +We will (1) rename the command from `defaults` to `env`, (2) keep the `default_`-prefixed YAML keys as established in ADR-35286, (3) switch the delete signal from empty string to explicit `null` — a field set to `null` (or absent) deletes the variable while a non-null string value sets it, and (4) make `gh aw env update` render a structured preview (scope, target, file, per-field action) and require interactive confirmation by default, with `--yes` / `-y` to bypass the prompt in automation. The `GH_AW_DEFAULT_*` GitHub Actions variable names themselves are unchanged. The confirmation gate is implemented as a small seam (`confirmAction func(...)`) so the path is unit-testable without driving a real TTY. + +### Alternatives Considered + +#### Alternative 1: Keep `default_*` keys as an accepted alias for backward compatibility + +Continue reading `default_*` keys alongside the trimmed keys (or canonicalize one to the other on read), so any user with a checked-in defaults file from the ADR-35286 era keeps working. This was rejected because the `gh aw defaults` command is brand new in PR #35286 (one commit prior to this PR) and has no documented release; the audience that could have adopted the legacy shape is effectively only the PR author. Keeping an alias would lock in two key spellings forever for zero real benefit and would dilute the new docs with "either of these works" caveats. A clean break now is cheaper than an alias forever. + +#### Alternative 2: Confirmation behind an opt-in `--confirm` flag instead of default-on with `--yes` bypass + +Leave the existing zero-prompt behavior and add a `--confirm` flag for users who want a preview. This was rejected because `defaults update` mutates GitHub Actions variables at potentially enterprise scope — the blast radius of an accidental run is significantly larger than typical CLI side effects, and the convention in `gh` and most modern destructive CLIs (e.g. `terraform apply`, `gh release delete`) is "confirm by default, `--yes` to skip." Defaulting to safe and letting CI opt out matches user expectations and shrinks the failure mode where a typo silently wipes shared config. + +#### Alternative 3: Diff-style preview (current → new value per field) instead of action-style preview + +Render the preview as a two-column "before / after" diff by first fetching the current value of each `GH_AW_DEFAULT_*` variable from the target scope, then showing per-field deltas. This was rejected because it would require an extra `gh api` round-trip per field on every `update` invocation (seven reads minimum), adding latency and a new failure mode (read fails, write would have succeeded), for a UX gain that is mostly cosmetic — the user can read the file they just edited. The chosen action-style preview (`set` / `delete` per field with the new value) is unambiguous about what will happen without needing the current state. + +### Consequences + +#### Positive + +- The on-disk file format is meaningfully shorter and easier to scan: seven `default_`-prefixed keys become seven trimmed keys with no information loss. +- `defaults update` now has a confirmation gate on a destructive, often org- or enterprise-scoped operation; accidental mass mutations from typos or wrong-file invocations are blocked by default. +- The console preview shows scope, target, file, and per-field action through the shared `console.RenderStruct` helper, so the surface matches other table-style CLI surfaces in this repo. +- The `--yes` / `-y` flag preserves the non-interactive path for CI without forcing users to remember a different flag name (`gh` uses `--yes` for the same purpose). +- The confirmation seam (`confirmAction` parameter on `confirmDefaultsUpdate`) allows the path to be unit-tested without a TTY, so the "skips when --yes / errors on cancel / calls action by default" behaviors are all covered by `TestConfirmDefaultsUpdate`. + +#### Negative + +- Hard breaking change in the file format: any defaults file written before this PR (using `default_*` keys) is silently treated as empty on read — every field becomes the empty string, which `defaults update` interprets as "delete the variable." A user re-running an old file with `--yes` would wipe their org defaults. The `TestDefaultsFileYAMLDoesNotReadLegacyKeys` test explicitly nails down this behavior, so it is by design, but the migration burden is real even if the population of affected users is small. +- CI workflows that call `gh aw defaults update` must add `--yes` or the command will hang waiting for stdin and eventually fail; this is documented in `docs/src/content/docs/reference/cost-management.md` but still requires a one-line change everywhere the command is wired up. +- One more decision point for users to learn: trimmed vs. legacy keys, and confirmation vs. `--yes`. The docs need to stay in sync with the binary forever. + +#### Neutral + +- The `GH_AW_DEFAULT_*` GitHub Actions variable names are unchanged; the override chain documented in ADR-35286 (Model Override Chain, Max-Effective-Tokens Override) is untouched, so no compiler or YAML-generation behavior is affected. +- The `defaultsBinding` struct gains a `fieldName` field so the preview renderer can show the file-side key (`max_turns`) rather than the GitHub variable name (`GH_AW_DEFAULT_MAX_TURNS`); the binding list remains the single source of truth for the seven managed variables. +- File permissions for the generated YAML now go through `constants.FilePermPublic` rather than an inline `0o644` literal — a small consistency cleanup that comes along for the ride. +- A new `displayName()` method on `defaultsTarget` and two new preview row types (`defaultsUpdatePreview`, `defaultsUpdateRow`) are added strictly for rendering; they have no behavior beyond formatting. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Defaults File Format + +1. The `defaults.yml` file consumed by `gh aw defaults get` and `gh aw defaults update` **MUST** use the trimmed YAML keys `max_effective_tokens`, `max_turns`, `timeout_minutes`, `detection_model`, `model_copilot`, `model_claude`, `model_codex`. +2. The `defaultsFile` struct **MUST NOT** declare `yaml:"default_*"` tags for any field; legacy `default_*` keys **MUST NOT** be read on unmarshal. +3. The `gh aw defaults get` subcommand **MUST** serialize using only the trimmed keys. +4. Each entry in `defaultsBindings` **MUST** carry both its `envName` (the `GH_AW_DEFAULT_*` GitHub Actions variable name) and its `fieldName` (the trimmed file-side key) so the update preview can label rows with the file-side key. +5. The `GH_AW_DEFAULT_*` GitHub Actions variable names themselves **MUST NOT** be changed by this ADR. + +### Update Command UX + +1. `gh aw defaults update` **MUST** render a preview of the planned mutation — scope, target display name, source file path, and a per-field action/value table — to stderr before any mutation is performed. +2. `gh aw defaults update` **MUST** require an interactive confirmation before applying mutations, unless `--yes` / `-y` is provided. +3. When the user declines confirmation, `gh aw defaults update` **MUST** return an error matching `"defaults update cancelled"` and **MUST NOT** perform any upsert or delete against the target scope. +4. The `--yes` / `-y` flag **MUST** bypass the confirmation prompt and **MUST** be the only built-in mechanism for non-interactive automation. +5. The confirmation step **MUST** be invoked through an injectable function value (the `confirmAction` parameter on `confirmDefaultsUpdate`) so it can be unit-tested without a TTY; the production wiring **MUST** use `console.ConfirmAction`. +6. An empty (or whitespace-only) value for any trimmed field in the input file **MUST** be interpreted as "delete the corresponding `GH_AW_DEFAULT_*` variable from the target scope," matching the behavior established in ADR-35286. + +### Preview Rendering + +1. The preview header **MUST** be rendered via `console.RenderStruct(defaultsUpdatePreview{...})` and include `Scope`, `Target`, `File`, and `Fields` columns. +2. The per-field table **MUST** be rendered via `console.RenderStruct` over `[]defaultsUpdateRow` with `Field`, `Action`, and `Value` columns. +3. The `Action` cell **MUST** be `"set"` for non-empty values and `"delete"` for empty values. +4. When `--yes` is provided, the preview **MUST** still be rendered before the "Skipping confirmation because --yes was provided." info message; the flag suppresses only the prompt, not the preview. + +### Documentation + +1. The reference page `docs/src/content/docs/reference/cost-management.md` **MUST** show the trimmed-key shape in its examples and **MUST** document the confirmation behavior and `--yes` override. +2. The reference page `docs/src/content/docs/reference/compiler-enterprise-environment-controls.md` **MUST** mention that the defaults file uses the trimmed keys. +3. The reference page `docs/src/content/docs/reference/environment-variables.md` **MUST** mention that `gh aw defaults` manages `GH_AW_DEFAULT_*` variables via trimmed YAML keys. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. This ADR refines but does not supersede [ADR-35286](35286-compiler-managed-enterprise-env-controls.md); the normative sections of ADR-35286 remain in force. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26549106345) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/docs/src/content/docs/reference/compiler-enterprise-environment-controls.md b/docs/src/content/docs/reference/compiler-enterprise-environment-controls.md index 98a4d9b7301..5aac5e32cb0 100644 --- a/docs/src/content/docs/reference/compiler-enterprise-environment-controls.md +++ b/docs/src/content/docs/reference/compiler-enterprise-environment-controls.md @@ -19,8 +19,10 @@ Use these variables to set organization- or repository-wide defaults without edi | `GH_AW_DEFAULT_MODEL_CLAUDE` | GitHub Actions `vars.*` at runtime | Default fallback model for Claude | `GH_AW_MODEL_AGENT_CLAUDE` / `GH_AW_MODEL_DETECTION_CLAUDE` is unset | | `GH_AW_DEFAULT_MODEL_CODEX` | GitHub Actions `vars.*` at runtime | Default fallback model for Codex | `GH_AW_MODEL_AGENT_CODEX` / `GH_AW_MODEL_DETECTION_CODEX` is unset | -Use `gh aw defaults get` and `gh aw defaults update` to manage these -variables in batch at repo, org, or enterprise scope. +Use `gh aw env get` and `gh aw env update` to manage these +variables in batch at repo, org, or enterprise scope. The defaults file uses +`default_`-prefixed keys such as `default_max_effective_tokens`, `default_timeout_minutes`, and +`default_model_copilot`. ## Precedence diff --git a/docs/src/content/docs/reference/cost-management.md b/docs/src/content/docs/reference/cost-management.md index 76c52a029ca..922faa7fef2 100644 --- a/docs/src/content/docs/reference/cost-management.md +++ b/docs/src/content/docs/reference/cost-management.md @@ -214,7 +214,7 @@ once, then let individual workflows override only when needed: 1. Export current defaults: ```bash -gh aw defaults get defaults.yml --scope org --org MY_ORG +gh aw env get defaults.yml --scope org --org MY_ORG ``` 2. Update and apply shared defaults in batch: @@ -227,9 +227,13 @@ default_model_codex: "gpt-5.4-mini" ``` ```bash -gh aw defaults update defaults.yml --scope org --org MY_ORG +gh aw env update defaults.yml --scope org --org MY_ORG ``` +`gh aw env update` shows a confirmation preview before applying changes. +Pass `--yes` to skip the prompt in automation. Set a field to `null` to delete +the corresponding variable from the target scope. + 3. If you compile workflows in CI, pass compiler-read defaults into the compiler process environment (for example via `${{ vars.* }}`): `GH_AW_DEFAULT_MAX_EFFECTIVE_TOKENS`, diff --git a/docs/src/content/docs/reference/environment-variables.md b/docs/src/content/docs/reference/environment-variables.md index af3f1bf2b0e..92259955110 100644 --- a/docs/src/content/docs/reference/environment-variables.md +++ b/docs/src/content/docs/reference/environment-variables.md @@ -175,8 +175,9 @@ At compile time, gh-aw emits runtime model expressions like: COPILOT_MODEL: ${{ vars.GH_AW_MODEL_AGENT_COPILOT || vars.GH_AW_DEFAULT_MODEL_COPILOT || '' }} ``` -Use `gh aw defaults get` / `gh aw defaults update` to batch-manage -these `GH_AW_DEFAULT_*` variables at repo, org, or enterprise scope. +Use `gh aw env get` / `gh aw env update` to batch-manage +these `GH_AW_DEFAULT_*` variables at repo, org, or enterprise scope with +`default_`-prefixed YAML keys such as `default_max_effective_tokens` and `default_model_copilot`. ### Agent runs diff --git a/pkg/cli/defaults_command_test.go b/pkg/cli/defaults_command_test.go deleted file mode 100644 index 627b24ba5b3..00000000000 --- a/pkg/cli/defaults_command_test.go +++ /dev/null @@ -1,99 +0,0 @@ -//go:build !integration - -package cli - -import ( - "testing" - - "github.com/goccy/go-yaml" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestNewDefaultsCommand(t *testing.T) { - cmd := NewDefaultsCommand() - require.NotNil(t, cmd) - assert.Equal(t, "defaults", cmd.Use) - - var hasGet, hasUpdate bool - for _, sub := range cmd.Commands() { - if sub.Name() == "get" { - hasGet = true - } - if sub.Name() == "update" { - hasUpdate = true - } - } - assert.True(t, hasGet, "defaults command should include get subcommand") - assert.True(t, hasUpdate, "defaults command should include update subcommand") -} - -func TestResolveDefaultsTarget(t *testing.T) { - orig := defaultsGetCurrentRepoSlug - defaultsGetCurrentRepoSlug = func() (string, error) { return "octo-org/example", nil } - t.Cleanup(func() { - defaultsGetCurrentRepoSlug = orig - }) - - t.Run("repo default scope uses current repo", func(t *testing.T) { - target, err := resolveDefaultsTarget("", "", "", "", false) - require.NoError(t, err) - assert.Equal(t, defaultsScopeRepo, target.scope) - assert.Equal(t, "octo-org", target.repoOwner) - assert.Equal(t, "example", target.repoName) - }) - - t.Run("update requires scope", func(t *testing.T) { - _, err := resolveDefaultsTarget("", "", "", "", true) - require.Error(t, err) - assert.Contains(t, err.Error(), "scope is required") - }) - - t.Run("org scope infers owner from repo", func(t *testing.T) { - target, err := resolveDefaultsTarget(defaultsScopeOrg, "github/gh-aw", "", "", false) - require.NoError(t, err) - assert.Equal(t, defaultsScopeOrg, target.scope) - assert.Equal(t, "github", target.org) - }) - - t.Run("ent scope requires enterprise", func(t *testing.T) { - _, err := resolveDefaultsTarget(defaultsScopeEnt, "", "", "", false) - require.Error(t, err) - assert.Contains(t, err.Error(), "--enterprise") - }) -} - -func TestDefaultsFileYAMLKeys(t *testing.T) { - file := defaultsFile{ - DefaultMaxEffectiveTokens: "10000", - DefaultMaxTurns: "42", - DefaultTimeoutMinutes: "90", - DefaultDetectionModel: "claude-sonnet-4.6", - DefaultModelCopilot: "claude-sonnet-4.7", - DefaultModelClaude: "claude-opus-4.7", - DefaultModelCodex: "gpt-5.5", - } - - data, err := yaml.Marshal(&file) - require.NoError(t, err) - - yml := string(data) - assert.Contains(t, yml, "default_max_effective_tokens:") - assert.Contains(t, yml, "default_max_turns:") - assert.Contains(t, yml, "default_timeout_minutes:") - assert.Contains(t, yml, "default_detection_model:") - assert.Contains(t, yml, "default_model_copilot:") - assert.Contains(t, yml, "default_model_claude:") - assert.Contains(t, yml, "default_model_codex:") -} - -func TestDefaultsTargetEndpoints(t *testing.T) { - repoTarget := defaultsTarget{scope: defaultsScopeRepo, repoOwner: "github", repoName: "gh-aw"} - orgTarget := defaultsTarget{scope: defaultsScopeOrg, org: "github"} - entTarget := defaultsTarget{scope: defaultsScopeEnt, enterprise: "octo-ent"} - - assert.Equal(t, "repos/github/gh-aw/actions/variables", repoTarget.variablesEndpoint()) - assert.Equal(t, "orgs/github/actions/variables", orgTarget.variablesEndpoint()) - assert.Equal(t, "enterprises/octo-ent/actions/variables", entTarget.variablesEndpoint()) - assert.Equal(t, "repos/github/gh-aw/actions/variables/GH_AW_DEFAULT_MAX_TURNS", repoTarget.variableEndpoint("GH_AW_DEFAULT_MAX_TURNS")) -} diff --git a/pkg/cli/defaults_command.go b/pkg/cli/env_command.go similarity index 61% rename from pkg/cli/defaults_command.go rename to pkg/cli/env_command.go index af0050b85e2..748de46267b 100644 --- a/pkg/cli/defaults_command.go +++ b/pkg/cli/env_command.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/repoutil" "github.com/github/gh-aw/pkg/workflow" "github.com/github/gh-aw/pkg/workflow/compilerenv" @@ -23,18 +24,19 @@ const ( ) type defaultsFile struct { - DefaultMaxEffectiveTokens string `yaml:"default_max_effective_tokens"` - DefaultMaxTurns string `yaml:"default_max_turns"` - DefaultTimeoutMinutes string `yaml:"default_timeout_minutes"` - DefaultDetectionModel string `yaml:"default_detection_model"` - DefaultModelCopilot string `yaml:"default_model_copilot"` - DefaultModelClaude string `yaml:"default_model_claude"` - DefaultModelCodex string `yaml:"default_model_codex"` + DefaultMaxEffectiveTokens *string `yaml:"default_max_effective_tokens"` + DefaultMaxTurns *string `yaml:"default_max_turns"` + DefaultTimeoutMinutes *string `yaml:"default_timeout_minutes"` + DefaultDetectionModel *string `yaml:"default_detection_model"` + DefaultModelCopilot *string `yaml:"default_model_copilot"` + DefaultModelClaude *string `yaml:"default_model_claude"` + DefaultModelCodex *string `yaml:"default_model_codex"` } type defaultsBinding struct { - envName string - get func(*defaultsFile) *string + envName string + fieldName string + get func(*defaultsFile) **string } type defaultsTarget struct { @@ -45,6 +47,26 @@ type defaultsTarget struct { enterprise string } +type defaultsUpdateChange struct { + envName string + field string + value string + delete bool +} + +type defaultsUpdatePreview struct { + Scope string `console:"header:Scope"` + Target string `console:"header:Target"` + File string `console:"header:File"` + Fields int `console:"header:Fields"` +} + +type defaultsUpdateRow struct { + Field string `console:"header:Field"` + Action string `console:"header:Action"` + Value string `console:"header:Value,omitempty"` +} + type defaultsGHError struct { command string exitCode int @@ -64,26 +86,27 @@ func (e *defaultsGHError) Unwrap() error { } var defaultsBindings = []defaultsBinding{ - {envName: compilerenv.DefaultMaxEffectiveTokens, get: func(f *defaultsFile) *string { return &f.DefaultMaxEffectiveTokens }}, - {envName: compilerenv.DefaultMaxTurns, get: func(f *defaultsFile) *string { return &f.DefaultMaxTurns }}, - {envName: compilerenv.DefaultTimeoutMinutes, get: func(f *defaultsFile) *string { return &f.DefaultTimeoutMinutes }}, - {envName: compilerenv.DefaultDetectionModel, get: func(f *defaultsFile) *string { return &f.DefaultDetectionModel }}, - {envName: compilerenv.DefaultModelCopilot, get: func(f *defaultsFile) *string { return &f.DefaultModelCopilot }}, - {envName: compilerenv.DefaultModelClaude, get: func(f *defaultsFile) *string { return &f.DefaultModelClaude }}, - {envName: compilerenv.DefaultModelCodex, get: func(f *defaultsFile) *string { return &f.DefaultModelCodex }}, + {envName: compilerenv.DefaultMaxEffectiveTokens, fieldName: "default_max_effective_tokens", get: func(f *defaultsFile) **string { return &f.DefaultMaxEffectiveTokens }}, + {envName: compilerenv.DefaultMaxTurns, fieldName: "default_max_turns", get: func(f *defaultsFile) **string { return &f.DefaultMaxTurns }}, + {envName: compilerenv.DefaultTimeoutMinutes, fieldName: "default_timeout_minutes", get: func(f *defaultsFile) **string { return &f.DefaultTimeoutMinutes }}, + {envName: compilerenv.DefaultDetectionModel, fieldName: "default_detection_model", get: func(f *defaultsFile) **string { return &f.DefaultDetectionModel }}, + {envName: compilerenv.DefaultModelCopilot, fieldName: "default_model_copilot", get: func(f *defaultsFile) **string { return &f.DefaultModelCopilot }}, + {envName: compilerenv.DefaultModelClaude, fieldName: "default_model_claude", get: func(f *defaultsFile) **string { return &f.DefaultModelClaude }}, + {envName: compilerenv.DefaultModelCodex, fieldName: "default_model_codex", get: func(f *defaultsFile) **string { return &f.DefaultModelCodex }}, } var defaultsExecGH = workflow.ExecGH var defaultsGetCurrentRepoSlug = GetCurrentRepoSlug -func NewDefaultsCommand() *cobra.Command { +func NewEnvCommand() *cobra.Command { cmd := &cobra.Command{ - Use: "defaults", + Use: "env", Short: "Manage compiler defaults as GitHub variables", Long: `Manage compiler default variables in batch for repository, organization, or enterprise scope. -The YAML file is flat and uses lowercase keys without the GH_AW_ prefix. -Empty values in update mode delete variables from the selected scope.`, +The YAML file is flat and uses default_-prefixed lowercase keys (e.g. default_max_turns). +Set a field to null (or omit it) in update mode to delete the variable from the selected scope. +Any field with a non-null string value will be set or updated.`, RunE: func(cmd *cobra.Command, args []string) error { return cmd.Help() }, @@ -123,6 +146,7 @@ func newDefaultsGetCommand() *cobra.Command { func newDefaultsUpdateCommand() *cobra.Command { var scope, repo, org, enterprise string + var yes bool cmd := &cobra.Command{ Use: "update [file]", @@ -137,7 +161,7 @@ func newDefaultsUpdateCommand() *cobra.Command { if err != nil { return err } - return defaultsUpdateFromFile(target, inputFile) + return defaultsUpdateFromFile(target, inputFile, yes) }, } @@ -145,6 +169,7 @@ func newDefaultsUpdateCommand() *cobra.Command { cmd.Flags().StringVar(&repo, "repo", "", "Target repository in owner/repo format") cmd.Flags().StringVar(&org, "org", "", "Target organization (required for --scope org unless inferable from --repo/current repo)") cmd.Flags().StringVar(&enterprise, "enterprise", "", "Target enterprise slug (required for --scope ent)") + cmd.Flags().BoolVarP(&yes, "yes", "y", false, "Skip confirmation prompt") _ = cmd.MarkFlagRequired("scope") return cmd } @@ -157,21 +182,25 @@ func defaultsGetToFile(target defaultsTarget, outputFile string) error { if err != nil { return err } - *binding.get(&file) = value + if value != "" { + v := value + *binding.get(&file) = &v + } + // nil (variable not set) serializes as null in YAML } data, err := yaml.Marshal(&file) if err != nil { return fmt.Errorf("failed to serialize defaults YAML: %w", err) } - if err := os.WriteFile(outputFile, data, 0o644); err != nil { + if err := os.WriteFile(outputFile, data, constants.FilePermPublic); err != nil { return fmt.Errorf("failed to write defaults file %q: %w", outputFile, err) } - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Saved defaults to %s", outputFile))) + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Saved defaults to "+outputFile)) return nil } -func defaultsUpdateFromFile(target defaultsTarget, inputFile string) error { +func defaultsUpdateFromFile(target defaultsTarget, inputFile string, skipConfirmation bool) error { data, err := os.ReadFile(inputFile) if err != nil { return fmt.Errorf("failed to read defaults file %q: %w", inputFile, err) @@ -182,23 +211,103 @@ func defaultsUpdateFromFile(target defaultsTarget, inputFile string) error { return fmt.Errorf("failed to parse defaults file %q: %w", inputFile, err) } - for _, binding := range defaultsBindings { - value := strings.TrimSpace(*binding.get(&file)) - if value == "" { - if err := deleteDefaultsVariable(target, binding.envName); err != nil { + changes := defaultsBuildUpdateChanges(&file) + if err := confirmDefaultsUpdate(target, inputFile, changes, skipConfirmation, console.ConfirmAction); err != nil { + return err + } + + for _, change := range changes { + if change.delete { + if err := deleteDefaultsVariable(target, change.envName); err != nil { return err } continue } - if err := upsertDefaultsVariable(target, binding.envName, value); err != nil { + if err := upsertDefaultsVariable(target, change.envName, change.value); err != nil { return err } } - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Updated defaults from %s", inputFile))) + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated defaults from "+inputFile)) + return nil +} + +func defaultsBuildUpdateChanges(file *defaultsFile) []defaultsUpdateChange { + changes := make([]defaultsUpdateChange, 0, len(defaultsBindings)) + for _, binding := range defaultsBindings { + ptr := *binding.get(file) + if ptr == nil { + changes = append(changes, defaultsUpdateChange{ + envName: binding.envName, + field: binding.fieldName, + delete: true, + }) + } else { + changes = append(changes, defaultsUpdateChange{ + envName: binding.envName, + field: binding.fieldName, + value: *ptr, + delete: false, + }) + } + } + return changes +} + +func confirmDefaultsUpdate( + target defaultsTarget, + inputFile string, + changes []defaultsUpdateChange, + skipConfirmation bool, + confirmAction func(title, affirmative, negative string) (bool, error), +) error { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Defaults update preview:")) + fmt.Fprint(os.Stderr, console.RenderStruct(defaultsUpdatePreview{ + Scope: target.scope, + Target: target.displayName(), + File: inputFile, + Fields: len(changes), + })) + fmt.Fprintln(os.Stderr) + fmt.Fprintln(os.Stderr) + fmt.Fprint(os.Stderr, console.RenderStruct(defaultsUpdateRows(changes))) + fmt.Fprintln(os.Stderr) + + if skipConfirmation { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Skipping confirmation because --yes was provided.")) + return nil + } + + confirmed, err := confirmAction( + "Do you want to update these defaults?", + "Yes, update", + "No, cancel", + ) + if err != nil { + return fmt.Errorf("failed to get confirmation: %w", err) + } + if !confirmed { + return errors.New("defaults update cancelled") + } return nil } +func defaultsUpdateRows(changes []defaultsUpdateChange) []defaultsUpdateRow { + rows := make([]defaultsUpdateRow, 0, len(changes)) + for _, change := range changes { + action := "set" + if change.delete { + action = "delete" + } + rows = append(rows, defaultsUpdateRow{ + Field: change.field, + Action: action, + Value: change.value, + }) + } + return rows +} + func resolveDefaultsTarget(scope, repo, org, enterprise string, scopeRequired bool) (defaultsTarget, error) { normalizedScope := strings.TrimSpace(scope) if normalizedScope == "" { @@ -267,6 +376,17 @@ func (t defaultsTarget) variableEndpoint(name string) string { return fmt.Sprintf("%s/%s", t.variablesEndpoint(), url.PathEscape(name)) } +func (t defaultsTarget) displayName() string { + switch t.scope { + case defaultsScopeRepo: + return t.repoOwner + "/" + t.repoName + case defaultsScopeOrg: + return t.org + default: + return t.enterprise + } +} + func runDefaultsGH(args ...string) ([]byte, error) { cmd := defaultsExecGH(args...) out, err := cmd.CombinedOutput() diff --git a/pkg/cli/env_command_test.go b/pkg/cli/env_command_test.go new file mode 100644 index 00000000000..5cbcbfe783c --- /dev/null +++ b/pkg/cli/env_command_test.go @@ -0,0 +1,187 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/goccy/go-yaml" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func strPtr(s string) *string { return &s } + +func TestNewEnvCommand(t *testing.T) { + cmd := NewEnvCommand() + require.NotNil(t, cmd) + assert.Equal(t, "env", cmd.Use) + + var updateCmd *cobra.Command + var hasGet, hasUpdate bool + for _, sub := range cmd.Commands() { + if sub.Name() == "get" { + hasGet = true + } + if sub.Name() == "update" { + hasUpdate = true + updateCmd = sub + } + } + assert.True(t, hasGet, "env command should include get subcommand") + assert.True(t, hasUpdate, "env command should include update subcommand") + require.NotNil(t, updateCmd) + assert.NotNil(t, updateCmd.Flags().Lookup("yes")) +} + +func TestResolveDefaultsTarget(t *testing.T) { + orig := defaultsGetCurrentRepoSlug + defaultsGetCurrentRepoSlug = func() (string, error) { return "octo-org/example", nil } + t.Cleanup(func() { + defaultsGetCurrentRepoSlug = orig + }) + + t.Run("repo default scope uses current repo", func(t *testing.T) { + target, err := resolveDefaultsTarget("", "", "", "", false) + require.NoError(t, err) + assert.Equal(t, defaultsScopeRepo, target.scope) + assert.Equal(t, "octo-org", target.repoOwner) + assert.Equal(t, "example", target.repoName) + }) + + t.Run("update requires scope", func(t *testing.T) { + _, err := resolveDefaultsTarget("", "", "", "", true) + require.Error(t, err) + assert.Contains(t, err.Error(), "scope is required") + }) + + t.Run("org scope infers owner from repo", func(t *testing.T) { + target, err := resolveDefaultsTarget(defaultsScopeOrg, "github/gh-aw", "", "", false) + require.NoError(t, err) + assert.Equal(t, defaultsScopeOrg, target.scope) + assert.Equal(t, "github", target.org) + }) + + t.Run("ent scope requires enterprise", func(t *testing.T) { + _, err := resolveDefaultsTarget(defaultsScopeEnt, "", "", "", false) + require.Error(t, err) + assert.Contains(t, err.Error(), "--enterprise") + }) +} + +func TestDefaultsFileYAMLKeys(t *testing.T) { + file := defaultsFile{ + DefaultMaxEffectiveTokens: strPtr("10000"), + DefaultMaxTurns: strPtr("42"), + DefaultTimeoutMinutes: strPtr("90"), + DefaultDetectionModel: strPtr("claude-sonnet-4.6"), + DefaultModelCopilot: strPtr("claude-sonnet-4.7"), + DefaultModelClaude: strPtr("claude-opus-4.7"), + DefaultModelCodex: strPtr("gpt-5.5"), + } + + data, err := yaml.Marshal(&file) + require.NoError(t, err) + + yml := string(data) + assert.Contains(t, yml, "default_max_effective_tokens:") + assert.Contains(t, yml, "default_max_turns:") + assert.Contains(t, yml, "default_timeout_minutes:") + assert.Contains(t, yml, "default_detection_model:") + assert.Contains(t, yml, "default_model_copilot:") + assert.Contains(t, yml, "default_model_claude:") + assert.Contains(t, yml, "default_model_codex:") +} + +func TestDefaultsFileYAMLNullDelete(t *testing.T) { + t.Run("null value unmarshals to nil pointer", func(t *testing.T) { + var file defaultsFile + err := yaml.Unmarshal([]byte("default_max_turns: null\n"), &file) + require.NoError(t, err) + assert.Nil(t, file.DefaultMaxTurns) + }) + + t.Run("string value unmarshals to non-nil pointer", func(t *testing.T) { + var file defaultsFile + err := yaml.Unmarshal([]byte("default_max_turns: \"42\"\ndefault_model_copilot: gpt-5-mini\n"), &file) + require.NoError(t, err) + require.NotNil(t, file.DefaultMaxTurns) + assert.Equal(t, "42", *file.DefaultMaxTurns) + require.NotNil(t, file.DefaultModelCopilot) + assert.Equal(t, "gpt-5-mini", *file.DefaultModelCopilot) + }) + + t.Run("absent key unmarshals to nil pointer", func(t *testing.T) { + var file defaultsFile + err := yaml.Unmarshal([]byte("default_model_copilot: gpt-5-mini\n"), &file) + require.NoError(t, err) + assert.Nil(t, file.DefaultMaxTurns) + }) +} + +func TestDefaultsTargetEndpoints(t *testing.T) { + repoTarget := defaultsTarget{scope: defaultsScopeRepo, repoOwner: "github", repoName: "gh-aw"} + orgTarget := defaultsTarget{scope: defaultsScopeOrg, org: "github"} + entTarget := defaultsTarget{scope: defaultsScopeEnt, enterprise: "octo-ent"} + + assert.Equal(t, "repos/github/gh-aw/actions/variables", repoTarget.variablesEndpoint()) + assert.Equal(t, "orgs/github/actions/variables", orgTarget.variablesEndpoint()) + assert.Equal(t, "enterprises/octo-ent/actions/variables", entTarget.variablesEndpoint()) + assert.Equal(t, "repos/github/gh-aw/actions/variables/GH_AW_DEFAULT_MAX_TURNS", repoTarget.variableEndpoint("GH_AW_DEFAULT_MAX_TURNS")) +} + +func TestDefaultsBuildUpdateChanges(t *testing.T) { + changes := defaultsBuildUpdateChanges(&defaultsFile{ + DefaultMaxEffectiveTokens: strPtr("10000"), + DefaultModelCodex: strPtr("gpt-5.5"), + }) + + require.Len(t, changes, len(defaultsBindings)) + assert.Equal(t, "default_max_effective_tokens", changes[0].field) + assert.Equal(t, "10000", changes[0].value) + assert.False(t, changes[0].delete) + assert.Equal(t, "default_max_turns", changes[1].field) + assert.True(t, changes[1].delete) + assert.Equal(t, "default_model_codex", changes[len(changes)-1].field) + assert.Equal(t, "gpt-5.5", changes[len(changes)-1].value) +} + +func TestConfirmDefaultsUpdate(t *testing.T) { + target := defaultsTarget{scope: defaultsScopeOrg, org: "github"} + changes := []defaultsUpdateChange{{field: "default_max_turns", value: "42"}} + + t.Run("requests confirmation by default", func(t *testing.T) { + called := false + confirmAction := func(title, affirmative, negative string) (bool, error) { + called = true + assert.Equal(t, "Do you want to update these defaults?", title) + assert.Equal(t, "Yes, update", affirmative) + assert.Equal(t, "No, cancel", negative) + return true, nil + } + + err := confirmDefaultsUpdate(target, "defaults.yml", changes, false, confirmAction) + require.NoError(t, err) + assert.True(t, called) + }) + + t.Run("skips confirmation with yes", func(t *testing.T) { + confirmAction := func(title, affirmative, negative string) (bool, error) { + t.Fatal("confirmation should be skipped") + return false, nil + } + + err := confirmDefaultsUpdate(target, "defaults.yml", changes, true, confirmAction) + require.NoError(t, err) + }) + + t.Run("returns cancellation error", func(t *testing.T) { + confirmAction := func(title, affirmative, negative string) (bool, error) { + return false, nil + } + + err := confirmDefaultsUpdate(target, "defaults.yml", changes, false, confirmAction) + require.ErrorContains(t, err, "defaults update cancelled") + }) +}