Add opt-in confirmations for task mutations by leet-c1 · Pull Request #155 · ConductorOne/cone

leet-c1 · 2026-09-19T02:21:08Z

Summary

add an opt-in root --confirm flag for task approve, deny, comment, and escalation mutations
resolve and validate the task before prompting, then send the mutation only after confirmation
reject confirmations in non-interactive mode and on unsupported commands; default script behavior is unchanged

Validation

go test ./...
go vet ./...
live-validated unsupported and non-interactive confirmation behavior against leet.conductor.one
adversarial security review completed

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

github-actions · 2026-09-19T02:24:36Z

+	enabled, _ := cmd.Flags().GetBool(confirmFlag)
+	if !enabled {
+		return nil
+	}
+
+	nonInteractive, _ := cmd.Flags().GetBool(nonInteractiveFlag)


🟠 Bug: Both flags are read straight off the pflag set, bypassing viper. Everywhere else in this repo non-interactive is read as v.GetBool(nonInteractiveFlag) (get_drop_task.go:136,190,449, form_fields.go:37), and getSubViperForProfile binds cmd.Flags() so config-profile and CONE_* env values resolve. Two consequences: CONE_CONFIRM=true / confirm: true in a profile silently does nothing (the safety control fails open, no prompt), and CONE_NON_INTERACTIVE=true combined with --confirm skips the guard and falls through to pterm, which opens /dev/tty directly — so it blocks on a terminal prompt instead of returning the intended error.

Suggest threading the *viper.Viper that all four call sites already have from cmdContext into this helper and using v.GetBool(...) for both reads. Confidence: high.

github-actions · 2026-09-19T02:24:39Z

+	if _, err := c.GetTask(ctx, taskID); err != nil {
+		return err
+	}


🟡 Suggestion: This GetTask runs unconditionally, so every cone task comment invocation now pays an extra API round-trip and gains a new failure mode even when --confirm is not passed — contrary to the PR's "default scripting behavior is unchanged". Approve/deny already needed the task for its policy ID, but here the result is discarded. Consider gating it on the confirm flag, and since you are fetching it anyway, using the display name in the prompt text so the resolved task is actually visible to the user. Same pattern at cmd/cone/task_escalate.go:32. Confidence: high on the behavior change, medium on user impact.

github-actions · 2026-09-19T02:24:46Z

+func TestConfirmMutationIsOptIn(t *testing.T) {
+	if err := confirmMutation(&cobra.Command{}, "creating an access request"); err != nil {
+		t.Fatalf("confirmMutation without --confirm: %v", err)
+	}
+}


🟡 Suggestion: This test passes vacuously. &cobra.Command{} has no confirm flag registered, so GetBool returns an error that confirmMutation discards and enabled is false regardless of the opt-in logic — the test would still pass if the default flipped to opt-out. Register confirmFlag (defaulting false) on the command so the assertion exercises the real path. A case covering rejection (confirmed == false → mutation cancelled) and one for confirmationSupported on an un-annotated command would also be worth adding. Confidence: high.

github-actions · 2026-09-19T02:24:50Z

 	cliCmd.PersistentFlags().StringP("output", "o", "table", "Output format. Valid values: table, json, json-pretty, wide.")
 	cliCmd.PersistentFlags().Bool("debug", false, "Enable HTTP debug logging")
 	cliCmd.PersistentFlags().String("log-level", "", "Set log level (debug, info, warn, error)")
+	cliCmd.PersistentFlags().Bool(confirmFlag, false, "Prompt before supported task mutations")


🟡 Suggestion: --confirm is registered as a root persistent flag, so it appears in the help output of every command (cone login --confirm, cone get --confirm, …) while the PersistentPreRunE guard rejects all but the four annotated task commands. The guard also only fires for runnable commands: cone task --confirm returns flag.ErrHelp before PersistentPreRunE runs, so it silently prints help instead of erroring. Registering the flag on the four supported commands directly would make the surface self-describing and remove the need for the annotation plumbing. Confidence: high.

github-actions · 2026-09-19T02:25:15Z

General PR Review: Add opt-in confirmations for task mutations

Blocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base 5ffe74330f1c.
Review mode: full
View review run

Review Summary

Scanned the full PR diff for security and correctness: a new --confirm root persistent flag, the confirmMutation/supportsConfirmation helpers, the confirmation hook in task approve|deny|comment|escalate, a root RunE so the unsupported-command guard can fire, and a new unit test. No dependency manifests changed and no security issues were found. One blocking correctness issue: confirmMutation reads both confirm and non-interactive directly from the pflag set instead of viper, which makes the feature fail open when configured via env/profile and makes the non-interactive escape hatch ineffective.

Security Issues

None found.

Correctness Issues

cmd/cone/confirmation.go:28-33 — flags read via cmd.Flags().GetBool bypass viper, so CONE_CONFIRM / profile confirm: true silently skips the prompt, and CONE_NON_INTERACTIVE=true with --confirm falls through to a pterm prompt on /dev/tty instead of erroring.

Suggestions

cmd/cone/task_comment.go:34 and cmd/cone/task_escalate.go:32 — the new GetTask call runs unconditionally, adding an API round-trip and a new failure mode even when --confirm is absent; its result is discarded.
cmd/cone/confirmation_test.go:24-28 — TestConfirmMutationIsOptIn passes vacuously because the bare cobra.Command has no confirm flag registered; cancel-path and confirmationSupported cases are untested.
cmd/cone/main.go:68 — --confirm is advertised in every command's help but supported on four; cone task --confirm silently prints help because flag.ErrHelp short-circuits before PersistentPreRunE.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `cmd/cone/confirmation.go`:
- Around lines 28-33: `confirmMutation` reads `confirm` and `non-interactive` with
  `cmd.Flags().GetBool(...)`, which only sees values passed as command-line flags. The rest
  of the codebase reads `non-interactive` as `v.GetBool(nonInteractiveFlag)` (see
  get_drop_task.go:136,190,449 and form_fields.go:37), and `getSubViperForProfile` binds
  `cmd.Flags()` into viper so config-profile and `CONE_*` env values resolve. As written,
  `CONE_CONFIRM=true` or `confirm: true` in a profile silently produces no prompt, and
  `CONE_NON_INTERACTIVE=true` combined with `--confirm` skips the interactive guard and
  falls through to pterm, which opens /dev/tty directly and blocks on a prompt instead of
  returning the intended error. Fix: change the signature to
  `confirmMutation(cmd *cobra.Command, v *viper.Viper, action string)` and use
  `v.GetBool(confirmFlag)` and `v.GetBool(nonInteractiveFlag)`. All four call sites
  (task_approve_deny.go, task_comment.go, task_escalate.go) already have `v` from
  `cmdContext`, so pass it through.

## Suggestions

In `cmd/cone/task_comment.go` and `cmd/cone/task_escalate.go`:
- task_comment.go around lines 34-36 and task_escalate.go around lines 32-34: the new
  `c.GetTask(ctx, taskID)` call runs on every invocation and its result is discarded, so
  non-confirm runs now pay an extra API round-trip and can fail where they previously
  succeeded. Gate the call on the confirm flag being enabled. Since the task is fetched
  anyway, consider including the task display name in the confirmation prompt text so the
  user sees the resolved task rather than just the ID.

In `cmd/cone/confirmation_test.go`:
- Around lines 24-28: `TestConfirmMutationIsOptIn` uses a bare `&cobra.Command{}` with no
  `confirm` flag registered, so `GetBool` errors, `enabled` is false regardless of the
  logic, and the test would still pass if the default flipped to opt-out. Register
  `confirmFlag` with a false default on the command. Add a test for the rejection path
  (confirmed == false yields the "mutation cancelled" error) and one for
  `confirmationSupported` returning false on a command without the annotation.

In `cmd/cone/main.go`:
- Around line 68: `--confirm` is registered as a root persistent flag, so it shows in the
  help for every command even though only four accept it, and the `PersistentPreRunE`
  guard does not fire for non-runnable commands (`cone task --confirm` returns
  flag.ErrHelp before PersistentPreRunE and silently prints help). Consider registering
  the flag on the four supported commands directly instead, which removes the need for
  the annotation and the root-level guard.

github-actions

Blocking issues found — see review comments.

feat: add opt-in task mutation confirmations

3aeff47

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

github-actions Bot reviewed Sep 19, 2026

View reviewed changes

github-actions Bot requested changes Sep 19, 2026

View reviewed changes

Navigation Menu

Add opt-in confirmations for task mutations - #155

Add opt-in confirmations for task mutations#155
leet-c1 wants to merge 1 commit into
mainfrom
lee.tschetter/cone-mutation-confirmation

leet-c1 commented Sep 19, 2026

Uh oh!

github-actions Bot Sep 19, 2026

Uh oh!

github-actions Bot Sep 19, 2026

Uh oh!

github-actions Bot Sep 19, 2026

Uh oh!

github-actions Bot Sep 19, 2026

Uh oh!

github-actions Bot commented Sep 19, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

leet-c1 commented Sep 19, 2026

Summary

Validation

Uh oh!

github-actions Bot Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 19, 2026

General PR Review: Add opt-in confirmations for task mutations

Review Summary

Security Issues

Correctness Issues

Suggestions

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant