From fce5cd2de68f01572b6feb542c60b0c7b91664e2 Mon Sep 17 00:00:00 2001 From: Michael Finson Date: Mon, 3 Aug 2026 20:14:19 +0300 Subject: [PATCH 1/3] fix: preserve operation dry-run semantics --- command_catalog.go | 17 ++++++++++++++++- command_catalog_test.go | 28 +++++++++++++++++++++++++++- destructive_contract.go | 3 +++ destructive_contract_test.go | 26 ++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 2 deletions(-) diff --git a/command_catalog.go b/command_catalog.go index 08ec48e..ab081b3 100644 --- a/command_catalog.go +++ b/command_catalog.go @@ -122,7 +122,7 @@ func buildCommandCatalog(api cli.API) commandCatalog { Example: parameter.Example, }) } - flags = append(flags, agentContractCatalogFlags()...) + flags = appendUniqueCatalogFlags(flags, agentContractCatalogFlags()) sort.Slice(flags, func(i, j int) bool { return flags[i].Name < flags[j].Name }) entries = append(entries, commandCatalogEntry{ Path: []string{operation.Name}, @@ -159,6 +159,21 @@ func buildCommandCatalog(api cli.API) commandCatalog { } } +func appendUniqueCatalogFlags(existing []commandCatalogFlag, additional []commandCatalogFlag) []commandCatalogFlag { + seen := make(map[string]bool, len(existing)+len(additional)) + for _, flag := range existing { + seen[flag.Name] = true + } + for _, flag := range additional { + if seen[flag.Name] { + continue + } + seen[flag.Name] = true + existing = append(existing, flag) + } + return existing +} + func catalogArgumentsForOperation(operation cli.Operation) []commandCatalogArgument { arguments := make([]commandCatalogArgument, 0, len(operation.PathParams)+1) for _, parameter := range operation.PathParams { diff --git a/command_catalog_test.go b/command_catalog_test.go index b7f3dda..2976c4f 100644 --- a/command_catalog_test.go +++ b/command_catalog_test.go @@ -28,10 +28,17 @@ func TestBuildCommandCatalog(t *testing.T) { }, {Name: "create-budget", Short: "Create a budget", Method: "POST", BodyMediaType: "application/json"}, {Name: "delete-datahub-events-by-filter", Method: "POST", BodyMediaType: "application/json"}, + { + Name: "cancel-invite", + Method: "POST", + QueryParams: []*cli.Param{ + {Name: "dryRun", Type: "boolean", Description: "Use the API simulation"}, + }, + }, {Name: "list-budgets", Short: "List budgets", Method: "GET"}, }} catalog := buildCommandCatalog(api) - if catalog.Version != catalogSchemaVersion || len(catalog.Commands) != 5 { + if catalog.Version != catalogSchemaVersion || len(catalog.Commands) != 6 { t.Fatalf("unexpected catalog: %+v", catalog) } var deleteEntry commandCatalogEntry @@ -65,6 +72,25 @@ func TestBuildCommandCatalog(t *testing.T) { if len(createEntry.Arguments) != 1 || createEntry.Arguments[0].Location != "body" || createEntry.Arguments[0].MediaType != "application/json" { t.Fatalf("create arguments = %+v", createEntry.Arguments) } + + var cancelInviteEntry commandCatalogEntry + for _, entry := range catalog.Commands { + if strings.Join(entry.Path, " ") == "cancel-invite" { + cancelInviteEntry = entry + } + } + dryRunFlags := 0 + for _, flag := range cancelInviteEntry.Flags { + if flag.Name == "--dry-run" { + dryRunFlags++ + if flag.Description != "Use the API simulation" { + t.Fatalf("dry-run description = %q", flag.Description) + } + } + } + if dryRunFlags != 1 { + t.Fatalf("dry-run flags = %d", dryRunFlags) + } } func TestCatalogAndRuntimeDestructiveClassificationMatch(t *testing.T) { diff --git a/destructive_contract.go b/destructive_contract.go index 883415c..14e3e6b 100644 --- a/destructive_contract.go +++ b/destructive_contract.go @@ -140,6 +140,9 @@ func isDestructiveCommand(command *cobra.Command) bool { func enforceDestructiveConfirmation(command *cobra.Command, args []string) error { if viper.GetBool("agent-dry-run") { + if command.LocalNonPersistentFlags().Lookup("dry-run") != nil { + return nil + } result := dryRunResult{DryRun: true, Command: command.Name(), Arguments: args} if err := json.NewEncoder(os.Stdout).Encode(result); err != nil { return err diff --git a/destructive_contract_test.go b/destructive_contract_test.go index 66bd01f..ee6928b 100644 --- a/destructive_contract_test.go +++ b/destructive_contract_test.go @@ -113,6 +113,32 @@ func TestDryRunNeverExecutesNonDestructiveCommand(t *testing.T) { } } +func TestDryRunDefersToOperationOwnedFlag(t *testing.T) { + setDestructiveOperations([]cli.Operation{{Name: "cancel-invite", Method: "POST"}}) + viper.Reset() + viper.Set("agent-dry-run", true) + t.Cleanup(viper.Reset) + + executed := false + command := &cobra.Command{ + Use: "cancel-invite", + RunE: func(command *cobra.Command, args []string) error { + executed = true + return nil + }, + } + command.Flags().Bool("dry-run", false, "Use the API simulation") + if err := enforceDestructiveConfirmation(command, []string{"invite-1"}); err != nil { + t.Fatal(err) + } + if err := command.RunE(command, []string{"invite-1"}); err != nil { + t.Fatal(err) + } + if !executed { + t.Fatal("operation-owned dry run was replaced by the local preview") + } +} + func TestDestructiveConfirmationErrorMetadata(t *testing.T) { err := destructiveConfirmationError{Command: "delete-budget"} if err.ExitCode() != 30 || err.AgentErrorCode() != "DESTRUCTIVE_REQUIRES_CONFIRMATION" { From 05f0179b253becacc0126d1bc4b3da95e3ded51a Mon Sep 17 00:00:00 2001 From: Michael Finson Date: Tue, 4 Aug 2026 15:11:02 +0300 Subject: [PATCH 2/3] fix(safety): distinguish native dry runs --- README.md | 2 +- command_catalog.go | 12 +++++++++--- command_catalog_test.go | 6 ++++++ destructive_contract.go | 37 +++++++++++++++++++++++++++++++++--- destructive_contract_test.go | 31 +++++++++++++++++++++++++++++- skills/dci-cli/SKILL.md | 2 +- 6 files changed, 81 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index cf40e2d..4faf3e7 100644 --- a/README.md +++ b/README.md @@ -124,7 +124,7 @@ dci --no-agent list-budgets | less -S Run `dci status` to see whether agent mode is active and why. -Pass `--dry-run` to preview any API command without sending its request. Commands classified as destructive require `--yes` or `DCI_CONFIRM_DESTRUCTIVE=1` before execution. +Pass `--dry-run` to preview any API command. Most commands use a local preview and send no request; operations with an API-native `dryRun` parameter send a simulation request and return an action marked `"dry_run": true`. The CLI supplies an idempotency key when that simulation requires one. Commands classified as destructive require `--yes` or `DCI_CONFIRM_DESTRUCTIVE=1` before real execution. ## Updating diff --git a/command_catalog.go b/command_catalog.go index ab081b3..2db0b66 100644 --- a/command_catalog.go +++ b/command_catalog.go @@ -52,6 +52,7 @@ type commandCatalogFlag struct { Default interface{} `json:"default,omitempty"` Description string `json:"description,omitempty"` Example interface{} `json:"example,omitempty"` + SafetyRole string `json:"safety_role,omitempty"` } func registerCommandCatalog() { @@ -161,8 +162,13 @@ func buildCommandCatalog(api cli.API) commandCatalog { func appendUniqueCatalogFlags(existing []commandCatalogFlag, additional []commandCatalogFlag) []commandCatalogFlag { seen := make(map[string]bool, len(existing)+len(additional)) - for _, flag := range existing { + for index, flag := range existing { seen[flag.Name] = true + for _, candidate := range additional { + if candidate.Name == flag.Name && existing[index].SafetyRole == "" { + existing[index].SafetyRole = candidate.SafetyRole + } + } } for _, flag := range additional { if seen[flag.Name] { @@ -247,9 +253,9 @@ func catalogFlagsFromFlagSet(flags *pflag.FlagSet) []commandCatalogFlag { func agentContractCatalogFlags() []commandCatalogFlag { flags := []commandCatalogFlag{ - {Name: "--dry-run", Type: "bool", Default: false, Description: "Preview a destructive operation without executing it"}, + {Name: "--dry-run", Type: "bool", Default: false, Description: "Preview a destructive operation without executing it", SafetyRole: "preview_before_execution"}, {Name: "--output", Type: "string", Description: "Select table, JSON, YAML, automatic, or TOON output"}, - {Name: "--yes", Type: "bool", Default: false, Description: "Confirm a destructive operation"}, + {Name: "--yes", Type: "bool", Default: false, Description: "Confirm a destructive operation", SafetyRole: "destructive_confirmation"}, } apiCommand := findDCICommand() if apiCommand == nil { diff --git a/command_catalog_test.go b/command_catalog_test.go index 2976c4f..3134dce 100644 --- a/command_catalog_test.go +++ b/command_catalog_test.go @@ -54,6 +54,9 @@ func TestBuildCommandCatalog(t *testing.T) { for _, flag := range deleteEntry.Flags { if flag.Name == "--yes" { foundConfirmation = true + if flag.SafetyRole != "destructive_confirmation" { + t.Fatalf("yes safety role = %q", flag.SafetyRole) + } } } if !foundConfirmation { @@ -86,6 +89,9 @@ func TestBuildCommandCatalog(t *testing.T) { if flag.Description != "Use the API simulation" { t.Fatalf("dry-run description = %q", flag.Description) } + if flag.SafetyRole != "preview_before_execution" { + t.Fatalf("dry-run safety role = %q", flag.SafetyRole) + } } } if dryRunFlags != 1 { diff --git a/destructive_contract.go b/destructive_contract.go index 14e3e6b..a6e13f2 100644 --- a/destructive_contract.go +++ b/destructive_contract.go @@ -1,6 +1,8 @@ package main import ( + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -13,8 +15,9 @@ import ( ) var ( - destructiveActionName string - destructiveCommandSet = map[string]bool{} + destructiveActionName string + destructiveActionDryRun bool + destructiveCommandSet = map[string]bool{} ) // Non-DELETE operations belong here only when they can revoke access, alter financial contracts, @@ -38,6 +41,7 @@ var explicitlyDestructiveOperations = map[string]bool{ func resetDestructiveContractState() { destructiveActionName = "" + destructiveActionDryRun = false destructiveCommandSet = map[string]bool{} } @@ -74,6 +78,7 @@ type dryRunResult struct { type actionSummary struct { Command string `json:"command"` Status string `json:"status"` + DryRun bool `json:"dry_run,omitempty"` } type actionResult struct { @@ -90,8 +95,12 @@ func (guard destructiveActionSummaryGuard) Format(response cli.Response) error { if isErrorResponseBody(response) { return guard.next.Format(response) } + status := "completed" + if destructiveActionDryRun { + status = "simulated" + } response.Body = actionResult{ - Action: actionSummary{Command: destructiveActionName, Status: "completed"}, + Action: actionSummary{Command: destructiveActionName, Status: status, DryRun: destructiveActionDryRun}, Result: response.Body, } } @@ -141,6 +150,11 @@ func isDestructiveCommand(command *cobra.Command) bool { func enforceDestructiveConfirmation(command *cobra.Command, args []string) error { if viper.GetBool("agent-dry-run") { if command.LocalNonPersistentFlags().Lookup("dry-run") != nil { + if err := ensureDryRunIdempotencyKey(command); err != nil { + return err + } + destructiveActionName = command.Name() + destructiveActionDryRun = true return nil } result := dryRunResult{DryRun: true, Command: command.Name(), Arguments: args} @@ -148,6 +162,7 @@ func enforceDestructiveConfirmation(command *cobra.Command, args []string) error return err } destructiveActionName = "" + destructiveActionDryRun = false command.Run = nil command.RunE = func(command *cobra.Command, args []string) error { return nil } return nil @@ -159,6 +174,7 @@ func enforceDestructiveConfirmation(command *cobra.Command, args []string) error return nil } destructiveActionName = command.Name() + destructiveActionDryRun = false confirmed := viper.GetBool("agent-confirm-destructive") if !confirmed { confirmed, _ = parseBoolish(os.Getenv("DCI_CONFIRM_DESTRUCTIVE")) @@ -169,3 +185,18 @@ func enforceDestructiveConfirmation(command *cobra.Command, args []string) error } return nil } + +func ensureDryRunIdempotencyKey(command *cobra.Command) error { + flag := command.LocalNonPersistentFlags().Lookup("idempotency-key") + if flag == nil || flag.Changed { + return nil + } + bytes := make([]byte, 16) + if _, err := rand.Read(bytes); err != nil { + return fmt.Errorf("generate dry-run idempotency key: %w", err) + } + if err := command.Flags().Set("idempotency-key", "dci-dry-run-"+hex.EncodeToString(bytes)); err != nil { + return fmt.Errorf("set dry-run idempotency key: %w", err) + } + return nil +} diff --git a/destructive_contract_test.go b/destructive_contract_test.go index ee6928b..362ccfa 100644 --- a/destructive_contract_test.go +++ b/destructive_contract_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "io" "os" + "strings" "testing" "github.com/rest-sh/restish/cli" @@ -115,9 +116,13 @@ func TestDryRunNeverExecutesNonDestructiveCommand(t *testing.T) { func TestDryRunDefersToOperationOwnedFlag(t *testing.T) { setDestructiveOperations([]cli.Operation{{Name: "cancel-invite", Method: "POST"}}) + resetDestructiveContractState() viper.Reset() viper.Set("agent-dry-run", true) - t.Cleanup(viper.Reset) + t.Cleanup(func() { + viper.Reset() + resetDestructiveContractState() + }) executed := false command := &cobra.Command{ @@ -128,15 +133,39 @@ func TestDryRunDefersToOperationOwnedFlag(t *testing.T) { }, } command.Flags().Bool("dry-run", false, "Use the API simulation") + command.Flags().String("idempotency-key", "", "Idempotency key") + if err := command.Flags().Set("dry-run", "true"); err != nil { + t.Fatal(err) + } if err := enforceDestructiveConfirmation(command, []string{"invite-1"}); err != nil { t.Fatal(err) } + if flag := command.Flags().Lookup("dry-run"); flag == nil || !flag.Changed || flag.Value.String() != "true" { + t.Fatalf("dry-run flag was not preserved: %+v", flag) + } + idempotencyFlag := command.Flags().Lookup("idempotency-key") + if idempotencyFlag == nil || !idempotencyFlag.Changed || !strings.HasPrefix(idempotencyFlag.Value.String(), "dci-dry-run-") { + t.Fatalf("idempotency flag was not synthesized: %+v", idempotencyFlag) + } if err := command.RunE(command, []string{"invite-1"}); err != nil { t.Fatal(err) } if !executed { t.Fatal("operation-owned dry run was replaced by the local preview") } + + next := &recordingFormatter{} + guard := destructiveActionSummaryGuard{next: next} + if err := guard.Format(cli.Response{Status: 200, Body: map[string]interface{}{"cancelled": false}}); err != nil { + t.Fatal(err) + } + result, ok := next.got.Body.(actionResult) + if !ok { + t.Fatalf("response body = %#v", next.got.Body) + } + if result.Action.Status != "simulated" || !result.Action.DryRun { + t.Fatalf("action summary = %+v", result.Action) + } } func TestDestructiveConfirmationErrorMetadata(t *testing.T) { diff --git a/skills/dci-cli/SKILL.md b/skills/dci-cli/SKILL.md index 5150f32..9c1c276 100644 --- a/skills/dci-cli/SKILL.md +++ b/skills/dci-cli/SKILL.md @@ -40,7 +40,7 @@ Load [query-patterns.md](references/query-patterns.md) when you need query examp - Prefer env-scoped `DCI_CUSTOMER_CONTEXT= dci ...` over `dci customer-context set` unless the user explicitly wants a persistent local change. - Treat `create-*`, `update-*`, `delete-*`, invite, ingest, and comment-post commands as side-effectful. - Use `dci commands --json` when you need machine-readable argument, flag, output-shape, authentication, and destructive-operation metadata. -- Run a side-effectful command with `--dry-run` first; it prints the intended command and arguments without sending the API request. +- Run a side-effectful command with `--dry-run` first. Most commands print a local preview without sending a request; commands with an API-native `dryRun` parameter send a simulation request and return an action marked `"dry_run": true`. - Pass `--yes` only after the user has approved a command classified as destructive. Do not set `DCI_CONFIRM_DESTRUCTIVE=1` as a blanket bypass. - Keep shared examples anonymized. Redact customer IDs, report IDs, emails, and URLs unless the user explicitly asks for live values. - When a command may fail because of permissions or context, explain that `dci login` proves authentication but not authorization. From 83f799ccaa765c01588456724e7e11ca059a491f Mon Sep 17 00:00:00 2001 From: Michael Finson Date: Tue, 4 Aug 2026 15:18:11 +0300 Subject: [PATCH 3/3] fix(catalog): mark required idempotency flags --- command_catalog.go | 7 +++++++ command_catalog_test.go | 10 ++++++++++ destructive_contract_test.go | 25 +++++++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/command_catalog.go b/command_catalog.go index 2db0b66..d71b607 100644 --- a/command_catalog.go +++ b/command_catalog.go @@ -52,9 +52,15 @@ type commandCatalogFlag struct { Default interface{} `json:"default,omitempty"` Description string `json:"description,omitempty"` Example interface{} `json:"example,omitempty"` + Required bool `json:"required"` SafetyRole string `json:"safety_role,omitempty"` } +var requiredOperationFlags = map[string]map[string]bool{ + "cancel-invite": {"idempotency-key": true}, + "resend-invite": {"idempotency-key": true}, +} + func registerCommandCatalog() { command := &cobra.Command{ Use: "commands", @@ -121,6 +127,7 @@ func buildCommandCatalog(api cli.API) commandCatalog { Default: parameter.Default, Description: parameter.Description, Example: parameter.Example, + Required: requiredOperationFlags[operation.Name][parameter.OptionName()], }) } flags = appendUniqueCatalogFlags(flags, agentContractCatalogFlags()) diff --git a/command_catalog_test.go b/command_catalog_test.go index 3134dce..fe35d98 100644 --- a/command_catalog_test.go +++ b/command_catalog_test.go @@ -34,6 +34,9 @@ func TestBuildCommandCatalog(t *testing.T) { QueryParams: []*cli.Param{ {Name: "dryRun", Type: "boolean", Description: "Use the API simulation"}, }, + HeaderParams: []*cli.Param{ + {Name: "Idempotency-Key", Type: "string", Description: "Idempotency key"}, + }, }, {Name: "list-budgets", Short: "List budgets", Method: "GET"}, }} @@ -83,6 +86,7 @@ func TestBuildCommandCatalog(t *testing.T) { } } dryRunFlags := 0 + foundRequiredIdempotencyKey := false for _, flag := range cancelInviteEntry.Flags { if flag.Name == "--dry-run" { dryRunFlags++ @@ -93,10 +97,16 @@ func TestBuildCommandCatalog(t *testing.T) { t.Fatalf("dry-run safety role = %q", flag.SafetyRole) } } + if flag.Name == "--idempotency-key" && flag.Required { + foundRequiredIdempotencyKey = true + } } if dryRunFlags != 1 { t.Fatalf("dry-run flags = %d", dryRunFlags) } + if !foundRequiredIdempotencyKey { + t.Fatal("catalog did not mark --idempotency-key as required") + } } func TestCatalogAndRuntimeDestructiveClassificationMatch(t *testing.T) { diff --git a/destructive_contract_test.go b/destructive_contract_test.go index 362ccfa..fd34059 100644 --- a/destructive_contract_test.go +++ b/destructive_contract_test.go @@ -168,6 +168,31 @@ func TestDryRunDefersToOperationOwnedFlag(t *testing.T) { } } +func TestDryRunPreservesExplicitIdempotencyKey(t *testing.T) { + viper.Reset() + viper.Set("agent-dry-run", true) + t.Cleanup(func() { + viper.Reset() + resetDestructiveContractState() + }) + + command := &cobra.Command{Use: "cancel-invite"} + command.Flags().Bool("dry-run", false, "Use the API simulation") + command.Flags().String("idempotency-key", "", "Idempotency key") + if err := command.Flags().Set("dry-run", "true"); err != nil { + t.Fatal(err) + } + if err := command.Flags().Set("idempotency-key", "caller-key"); err != nil { + t.Fatal(err) + } + if err := enforceDestructiveConfirmation(command, nil); err != nil { + t.Fatal(err) + } + if got := command.Flags().Lookup("idempotency-key").Value.String(); got != "caller-key" { + t.Fatalf("idempotency key = %q, want caller-key", got) + } +} + func TestDestructiveConfirmationErrorMetadata(t *testing.T) { err := destructiveConfirmationError{Command: "delete-budget"} if err.ExitCode() != 30 || err.AgentErrorCode() != "DESTRUCTIVE_REQUIRES_CONFIRMATION" {