From e10acb5d8fd6b65e070408f03b176e3366218506 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:08:48 +0000 Subject: [PATCH 1/7] Add five task actions on one shared runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit approve, deny, comment, close and reassign each hand-rolled the same thirty lines: resolve the task id, build a body, POST /action/, parse, print one line. Five more actions would have made ten copies, so the runner comes first and all ten share it. The five existing commands are byte-identical before and after — help text and dry-run output diffed against origin/main. New: restart, reset, skip-step, process, update-grant-duration. Each was driven against a live task and verified by its effect, not its exit code: restart and skip-step rotate the current policy step and append one history entry, reset appends four (it restarts the policy rather than the step), and update-grant-duration lands as grantDuration on the task. Deferred: escalate refuses with "action not permitted" even with emergency grants enabled on the entitlement and a valid step id, so its behaviour cannot be demonstrated here; update-request-data takes a free-form object that wants a body-file flag; approve-with-step-up needs a step-up transaction id the CLI cannot obtain. Co-Authored-By: Claude Opus 5 --- cmd/tasks_action.go | 109 ++++++++++++++++ cmd/tasks_action_test.go | 197 +++++++++++++++++++++++++++++ cmd/tasks_approve.go | 54 ++------ cmd/tasks_close.go | 49 ++----- cmd/tasks_comment.go | 53 +++----- cmd/tasks_deny.go | 59 ++------- cmd/tasks_process.go | 40 ++++++ cmd/tasks_reassign.go | 84 ++++-------- cmd/tasks_reset.go | 35 +++++ cmd/tasks_restart.go | 50 ++++++++ cmd/tasks_skip_step.go | 38 ++++++ cmd/tasks_update_grant_duration.go | 58 +++++++++ 12 files changed, 598 insertions(+), 228 deletions(-) create mode 100644 cmd/tasks_action.go create mode 100644 cmd/tasks_action_test.go create mode 100644 cmd/tasks_process.go create mode 100644 cmd/tasks_reset.go create mode 100644 cmd/tasks_restart.go create mode 100644 cmd/tasks_skip_step.go create mode 100644 cmd/tasks_update_grant_duration.go diff --git a/cmd/tasks_action.go b/cmd/tasks_action.go new file mode 100644 index 0000000..bc1463c --- /dev/null +++ b/cmd/tasks_action.go @@ -0,0 +1,109 @@ +package cmd + +import ( + "fmt" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" +) + +// policyStepMode says whether an action's request needs a policy step id. +type policyStepMode int + +const ( + stepUnused policyStepMode = iota // the endpoint takes no policyStepId + stepOptional // send it when it can be resolved, omit otherwise + stepRequired // the server rejects the call without it +) + +// taskAction describes one POST /api/v1/tasks/{id}/action/{verb} command. The +// eleven action commands differ only in these fields, so they share one RunE: +// hand-rolling each was already six near-identical copies before this. +type taskAction struct { + verb string // the path segment, e.g. "restart" + step policyStepMode + // extraBody adds fields beyond comment/policyStepId. It runs before the + // request is built, so it may also reject bad flag combinations. + extraBody func(cmd *cobra.Command, body map[string]any) error + // confirm formats the success line. State is passed but most actions must + // not print it — see runTaskAction. + confirm func(id, state, stepID string) string +} + +// runTaskAction is the shared RunE. Ordering matters and matches the rest of +// the repo: flags are validated before a client is built, so a usage error +// exits 2 rather than failing on credentials first. +func (a taskAction) runTaskAction(cmd *cobra.Command, args []string) error { + var comment string + if cmd.Flags().Lookup("comment") != nil { + comment, _ = cmd.Flags().GetString("comment") + } + + body := map[string]any{} + if a.extraBody != nil { + if err := a.extraBody(cmd, body); err != nil { + return err + } + } + + taskID := args[0] + path := client.Path("/api/v1/tasks/%s/action/%s", taskID, a.verb) + if comment != "" { + body["comment"] = comment + } + + // An action that needs no policy step needs no client to preview, so + // --dry-run works without credentials. Resolving a step requires a GET, so + // those actions must authenticate first even for a preview — which is what + // each command did before sharing this runner. + if a.step == stepUnused { + if dryRunActive() { + return printDryRun(cmd, "POST", path, body) + } + } + + baseURL, err := GetBaseURL() + if err != nil { + return err + } + c, err := newClient(cmd, baseURL) + if err != nil { + return fmt.Errorf("authentication failed: %w", err) + } + + var stepID string + if a.step != stepUnused { + explicit, _ := cmd.Flags().GetString("policy-step-id") + stepID, err = resolvePolicyStepID(cmd.Context(), c, taskID, explicit, a.step == stepRequired) + if err != nil { + return err + } + if stepID != "" { + body["policyStepId"] = stepID + } + if dryRunActive() { + return printDryRun(cmd, "POST", path, body) + } + } + + data, err := c.Post(cmd.Context(), path, body) + if err != nil { + return fmt.Errorf("API error: %w", err) + } + id, state, err := parseTaskActionResponse(data) + if err != nil { + return fmt.Errorf("failed to parse response: %w", err) + } + + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s", a.confirm(id, state, stepID)) + return nil +} + +// addTaskActionFlags registers the flags the shared RunE reads. Only --comment +// is universal; --policy-step-id is registered when the action uses one. +func addTaskActionFlags(cmd *cobra.Command, step policyStepMode, stepUsage string) { + cmd.Flags().String("comment", "", "Optional comment") + if step != stepUnused { + cmd.Flags().String("policy-step-id", "", stepUsage) + } +} diff --git a/cmd/tasks_action_test.go b/cmd/tasks_action_test.go new file mode 100644 index 0000000..585ce8f --- /dev/null +++ b/cmd/tasks_action_test.go @@ -0,0 +1,197 @@ +package cmd + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +const actionTestTaskID = "zz-c1i-test-task-2" + +// taskActionRecorder answers every action POST with a success body and records +// the path and body it received. +type taskActionRecorder struct { + srv *httptest.Server + paths []string + bodies []map[string]any +} + +func newTaskActionRecorder(t *testing.T, state, currentStepID string) *taskActionRecorder { + t.Helper() + r := &taskActionRecorder{} + r.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Set("Content-Type", "application/json") + // A GET is the policy-step lookup resolvePolicyStepID performs. + if req.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"taskView":{"task":{"id":"` + actionTestTaskID + + `","state":"` + state + `","policy":{"current":{"id":"` + currentStepID + `"}}}}}`)) + return + } + raw, _ := io.ReadAll(req.Body) + var body map[string]any + if len(raw) > 0 { + if err := json.Unmarshal(raw, &body); err != nil { + t.Errorf("decoding body for %s: %v", req.URL.Path, err) + } + } + r.paths = append(r.paths, req.URL.Path) + r.bodies = append(r.bodies, body) + _, _ = w.Write([]byte(taskActionResponse(actionTestTaskID, state))) + })) + t.Cleanup(r.srv.Close) + return r +} + +// TestTaskActionsPostTheirOwnVerb pins that each command hits its own action +// path. Sharing one RunE makes a copied verb the plausible mistake, and the +// wrong verb would silently perform a different action on the task. +func TestTaskActionsPostTheirOwnVerb(t *testing.T) { + cases := []struct { + cmdName string + wantPath string + }{ + {"restart", "/api/v1/tasks/" + actionTestTaskID + "/action/restart"}, + {"reset", "/api/v1/tasks/" + actionTestTaskID + "/action/reset"}, + {"skip-step", "/api/v1/tasks/" + actionTestTaskID + "/action/skip-step"}, + {"process", "/api/v1/tasks/" + actionTestTaskID + "/action/process"}, + {"close", "/api/v1/tasks/" + actionTestTaskID + "/action/close"}, + {"approve", "/api/v1/tasks/" + actionTestTaskID + "/action/approve"}, + {"deny", "/api/v1/tasks/" + actionTestTaskID + "/action/deny"}, + } + for _, tc := range cases { + t.Run(tc.cmdName, func(t *testing.T) { + cmd := findTasksSubcommand(t, tc.cmdName) + resetCmds(t, cmd) + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("%s: %v", tc.cmdName, err) + } + if len(r.paths) != 1 { + t.Fatalf("%s posted %d times, want 1: %v", tc.cmdName, len(r.paths), r.paths) + } + if r.paths[0] != tc.wantPath { + t.Errorf("%s posted %q, want %q", tc.cmdName, r.paths[0], tc.wantPath) + } + }) + } +} + +// TestTaskActionsSendPolicyStepOnlyWhenTheyUseOne pins the three step modes. +// Sending policyStepId to an endpoint that takes none, or omitting it where the +// server requires it, are both silent-wrong-request failures. +func TestTaskActionsSendPolicyStepOnlyWhenTheyUseOne(t *testing.T) { + const step = "zz-step-1111111111111111111" + cases := []struct { + cmdName string + wantStep bool + }{ + {"restart", true}, + {"skip-step", true}, + {"approve", true}, + {"reset", false}, + {"process", false}, + {"close", false}, + } + for _, tc := range cases { + t.Run(tc.cmdName, func(t *testing.T) { + cmd := findTasksSubcommand(t, tc.cmdName) + resetCmds(t, cmd) + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", step) + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("%s: %v", tc.cmdName, err) + } + got, ok := r.bodies[0]["policyStepId"] + if tc.wantStep { + if !ok || got != step { + t.Errorf("%s body policyStepId = %v (present=%v), want %q", tc.cmdName, got, ok, step) + } + return + } + if ok { + t.Errorf("%s sent policyStepId=%v to an endpoint that takes none", tc.cmdName, got) + } + }) + } +} + +// TestTaskActionsNeverEchoResponseState extends the guarantee close already +// had to every action that does not intend to print state: these endpoints +// return the task as it was BEFORE the action, so echoing it reports the old +// state as though the action had not happened. +func TestTaskActionsNeverEchoResponseState(t *testing.T) { + for _, name := range []string{"restart", "reset", "skip-step", "process", "close", "comment", "update-grant-duration"} { + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if name == "comment" { + _ = cmd.Flags().Set("comment", "zz") + } + if name == "update-grant-duration" { + _ = cmd.Flags().Set("duration", "3600s") + } + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + out, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + if strings.Contains(out, "TASK_STATE_OPEN") { + t.Errorf("%s echoed the pre-action state: %q", name, out) + } + if !strings.Contains(out, actionTestTaskID) { + t.Errorf("%s did not report the task id: %q", name, out) + } + }) + } +} + +// TestTasksRestartOmitsEmptyPolicyStepField pins that a closed task, which has +// no current step, does not produce "policy_step_id=" with nothing after it. +func TestTasksRestartOmitsEmptyPolicyStepField(t *testing.T) { + cmd := findTasksSubcommand(t, "restart") + resetCmds(t, cmd) + r := newTaskActionRecorder(t, "TASK_STATE_CLOSED", "") + out, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID) + if err != nil { + t.Fatalf("restart: %v", err) + } + if strings.Contains(out, "policy_step_id=\n") || strings.HasSuffix(strings.TrimRight(out, "\n"), "policy_step_id=") { + t.Errorf("restart printed an empty policy_step_id field: %q", out) + } +} + +// TestTasksUpdateGrantDurationRequiresDuration pins the usage error rather than +// letting the server answer "value is required" after a round trip. +func TestTasksUpdateGrantDurationRequiresDuration(t *testing.T) { + cmd := findTasksSubcommand(t, "update-grant-duration") + resetCmds(t, cmd) + _ = cmd.Flags().Set("duration", "") + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID) + if err == nil { + t.Fatal("expected a usage error for an empty --duration") + } + if got := exitCode(err); got != exitUsage { + t.Errorf("exitCode = %d, want %d (exitUsage); err = %v", got, exitUsage, err) + } + if len(r.paths) != 0 { + t.Errorf("a request was sent despite the usage error: %v", r.paths) + } +} + +// findTasksSubcommand looks the command up in the real tree, so a command that +// stops being registered fails here instead of silently going untested. +func findTasksSubcommand(t *testing.T, name string) *cobra.Command { + t.Helper() + for _, c := range tasksCmd.Commands() { + if c.Name() == name { + return c + } + } + t.Fatalf("tasks has no %q subcommand", name) + return nil +} diff --git a/cmd/tasks_approve.go b/cmd/tasks_approve.go index ccbddb1..619fed9 100644 --- a/cmd/tasks_approve.go +++ b/cmd/tasks_approve.go @@ -3,10 +3,17 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksApproveAction = taskAction{ + verb: "approve", + step: stepRequired, + confirm: func(id, state, _ string) string { + return fmt.Sprintf("Approved task: task_id=%s state=%s\n", id, state) + }, +} + var tasksApproveCmd = &cobra.Command{ Use: "approve ", Short: "Approve an access request task", @@ -17,50 +24,7 @@ If omitted, the task's currently executing step is fetched and used automatically; if it cannot be determined the command errors and asks you to pass --policy-step-id explicitly (approve requires a step).`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - policyStepID, _ := cmd.Flags().GetString("policy-step-id") - - stepID, err := resolvePolicyStepID(cmd.Context(), c, taskID, policyStepID, true) - if err != nil { - return err - } - - body := map[string]any{ - "policyStepId": stepID, - } - if comment != "" { - body["comment"] = comment - } - - path := client.Path("/api/v1/tasks/%s/action/approve", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - id, state, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Approved task: task_id=%s state=%s\n", id, state) - return nil - }, + RunE: tasksApproveAction.runTaskAction, } func init() { diff --git a/cmd/tasks_close.go b/cmd/tasks_close.go index 7626a66..e39195a 100644 --- a/cmd/tasks_close.go +++ b/cmd/tasks_close.go @@ -3,10 +3,17 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksCloseAction = taskAction{ + verb: "close", + step: stepUnused, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Closed task: task_id=%s\n", id) + }, +} + var tasksCloseCmd = &cobra.Command{ Use: "close ", Short: "Close a task without approving or denying it", @@ -16,45 +23,7 @@ Closing cancels the task and records no approval decision; use approve/deny to record an outcome. The confirmation reports only the task id — the action endpoints echo the task's pre-close state, so printing it would be wrong.`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - - body := map[string]any{} - if comment != "" { - body["comment"] = comment - } - - path := client.Path("/api/v1/tasks/%s/action/close", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - // State is deliberately not echoed: the action endpoints return the - // task as it was *before* the action, so a live close prints - // TASK_STATE_OPEN. Parsing still guards the response shape. - id, _, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Closed task: task_id=%s\n", id) - return nil - }, + RunE: tasksCloseAction.runTaskAction, } func init() { diff --git a/cmd/tasks_comment.go b/cmd/tasks_comment.go index 8e59207..631f9f2 100644 --- a/cmd/tasks_comment.go +++ b/cmd/tasks_comment.go @@ -3,49 +3,30 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksCommentAction = taskAction{ + verb: "comment", + step: stepUnused, + // Sent unconditionally, unlike every other action: the comment IS the + // payload here, so an explicit --comment "" must reach the server rather + // than being omitted as an absent optional field. + extraBody: func(cmd *cobra.Command, body map[string]any) error { + comment, _ := cmd.Flags().GetString("comment") + body["comment"] = comment + return nil + }, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Comment added: task_id=%s\n", id) + }, +} + var tasksCommentCmd = &cobra.Command{ Use: "comment ", Short: "Add a comment to a task", Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - - body := map[string]any{ - "comment": comment, - } - - path := client.Path("/api/v1/tasks/%s/action/comment", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - id, _, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Comment added: task_id=%s\n", id) - return nil - }, + RunE: tasksCommentAction.runTaskAction, } func init() { diff --git a/cmd/tasks_deny.go b/cmd/tasks_deny.go index 0428357..c520c2a 100644 --- a/cmd/tasks_deny.go +++ b/cmd/tasks_deny.go @@ -3,10 +3,19 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksDenyAction = taskAction{ + verb: "deny", + // Optional, not required: when the current step cannot be determined the + // field is omitted rather than blocking the denial. + step: stepOptional, + confirm: func(id, state, _ string) string { + return fmt.Sprintf("Denied task: task_id=%s state=%s\n", id, state) + }, +} + var tasksDenyCmd = &cobra.Command{ Use: "deny ", Short: "Deny an access request task", @@ -16,53 +25,7 @@ var tasksDenyCmd = &cobra.Command{ currently executing step is used when it can be derived, and simply left off otherwise — deny does not require a step, so it proceeds either way.`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - policyStepID, _ := cmd.Flags().GetString("policy-step-id") - - // policyStepId is optional for deny; include it when we can target a - // specific step (needed on multi-step policies) but don't require one. - stepID, err := resolvePolicyStepID(cmd.Context(), c, taskID, policyStepID, false) - if err != nil { - return err - } - - body := map[string]any{} - if stepID != "" { - body["policyStepId"] = stepID - } - if comment != "" { - body["comment"] = comment - } - - path := client.Path("/api/v1/tasks/%s/action/deny", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - id, state, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Denied task: task_id=%s state=%s\n", id, state) - return nil - }, + RunE: tasksDenyAction.runTaskAction, } func init() { diff --git a/cmd/tasks_process.go b/cmd/tasks_process.go new file mode 100644 index 0000000..efa71b8 --- /dev/null +++ b/cmd/tasks_process.go @@ -0,0 +1,40 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksProcessAction = taskAction{ + verb: "process", + step: stepUnused, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Queued task for processing: task_id=%s\n", id) + }, +} + +var tasksProcessCmd = &cobra.Command{ + Use: "process ", + Short: "Process a task now rather than waiting for the next cycle", + Long: `Ask C1 to process a task immediately instead of on its normal schedule. + +Useful when a task looks stuck: it re-runs the policy evaluation without +changing the task's approval state. The request body is empty — this action +takes neither a comment nor a policy step. + +On a healthy task nothing observable changes: state, current policy step and +history are all identical afterwards. Expect a result only where processing had +genuinely stalled. + +The confirmation reports only the task id: the action endpoints echo the task's +state from before the action, and processing is asynchronous, so re-read the +task with "c1i requests get " to see the result.`, + Args: cobra.ExactArgs(1), + RunE: tasksProcessAction.runTaskAction, +} + +func init() { + // No --comment: the ProcessNow request body carries only expandMask. + tasksCmd.AddCommand(tasksProcessCmd) +} diff --git a/cmd/tasks_reassign.go b/cmd/tasks_reassign.go index 51ce5b9..f484ee6 100644 --- a/cmd/tasks_reassign.go +++ b/cmd/tasks_reassign.go @@ -3,10 +3,33 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksReassignAction = taskAction{ + verb: "reassign", + // The API does not require policyStepId here (approve does). We require a + // resolvable step anyway: a reassign with no step is ambiguous, and failing + // loudly beats sending it. + step: stepRequired, + extraBody: func(cmd *cobra.Command, body map[string]any) error { + // Cobra's required check only proves the flag was set; the accessor is + // what rejects an empty occurrence that would post a blank approver id. + toUserIDs, err := repeatableStringFlag(cmd, "to-user-id") + if err != nil { + return err + } + if len(toUserIDs) == 0 { + return &usageError{fmt.Errorf("flag --to-user-id requires at least one value")} + } + body["newStepUserIds"] = toUserIDs + return nil + }, + confirm: func(id, _, stepID string) string { + return fmt.Sprintf("Reassigned task: task_id=%s policy_step_id=%s\n", id, stepID) + }, +} + var tasksReassignCmd = &cobra.Command{ Use: "reassign ", Short: "Reassign a task's approval step to other users", @@ -22,64 +45,7 @@ the command errors and asks you to pass --policy-step-id explicitly. The confirmation reports the task id and the policy step acted on, never a state: the action endpoints echo the task's state from before the action.`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - // Cobra's required check only proves the flag was set; the accessor is - // what rejects an empty occurrence that would post a blank approver id. - toUserIDs, err := repeatableStringFlag(cmd, "to-user-id") - if err != nil { - return err - } - if len(toUserIDs) == 0 { - return &usageError{fmt.Errorf("flag --to-user-id requires at least one value")} - } - - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - policyStepID, _ := cmd.Flags().GetString("policy-step-id") - - // The API does not require policyStepId here (approve does). We require a - // resolvable step anyway: a reassign with no step is ambiguous, and failing - // loudly beats sending it. - stepID, err := resolvePolicyStepID(cmd.Context(), c, taskID, policyStepID, true) - if err != nil { - return err - } - - body := map[string]any{ - "newStepUserIds": toUserIDs, - "policyStepId": stepID, - } - if comment != "" { - body["comment"] = comment - } - - path := client.Path("/api/v1/tasks/%s/action/reassign", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - id, _, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Reassigned task: task_id=%s policy_step_id=%s\n", id, stepID) - return nil - }, + RunE: tasksReassignAction.runTaskAction, } func init() { diff --git a/cmd/tasks_reset.go b/cmd/tasks_reset.go new file mode 100644 index 0000000..ec5ea4e --- /dev/null +++ b/cmd/tasks_reset.go @@ -0,0 +1,35 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksResetAction = taskAction{ + verb: "reset", + step: stepUnused, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Reset task: task_id=%s\n", id) + }, +} + +var tasksResetCmd = &cobra.Command{ + Use: "reset ", + Short: "Hard-reset a task to the start of its policy", + Long: `Hard-reset a task, returning it to the beginning of its policy. + +Unlike "restart", which re-runs the current step, this discards the task's +approval progress and starts the policy over. The endpoint is +/action/reset and takes no policy step. + +The confirmation reports only the task id: the action endpoints echo the task's +state from before the action.`, + Args: cobra.ExactArgs(1), + RunE: tasksResetAction.runTaskAction, +} + +func init() { + addTaskActionFlags(tasksResetCmd, tasksResetAction.step, "") + tasksCmd.AddCommand(tasksResetCmd) +} diff --git a/cmd/tasks_restart.go b/cmd/tasks_restart.go new file mode 100644 index 0000000..770c0ca --- /dev/null +++ b/cmd/tasks_restart.go @@ -0,0 +1,50 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksRestartAction = taskAction{ + verb: "restart", + step: stepOptional, + confirm: func(id, _, stepID string) string { + // A closed task has no current step, so the field would print empty. + if stepID == "" { + return fmt.Sprintf("Restarted task: task_id=%s\n", id) + } + return fmt.Sprintf("Restarted task: task_id=%s policy_step_id=%s\n", id, stepID) + }, +} + +var tasksRestartCmd = &cobra.Command{ + Use: "restart ", + Short: "Restart a task's approval step", + Long: `Restart a task's approval step, sending it back for a fresh decision. + +On an open task this rotates the current policy step and records the restart in +the task's policy history. It does NOT reopen a closed task: the API offers +restart on some closed tasks, but the call leaves the state CLOSED and only +appends history, so use it to re-run a live approval, not to undo a close. + +Whether restart is available at all depends on the task; the server refuses +with "action not permitted" otherwise. Check the task's own action list: + c1i api --path /api/v1/tasks/ --fields actions + +Every action rotates the current policy step, so a --policy-step-id captured +earlier goes stale and the server answers: + this action is no longer available: the request has advanced to a new approval step +Omit the flag to act on whatever step is current. + +The confirmation reports the task id and the step acted on, never a state: the +action endpoints echo the task's state from before the action.`, + Args: cobra.ExactArgs(1), + RunE: tasksRestartAction.runTaskAction, +} + +func init() { + addTaskActionFlags(tasksRestartCmd, tasksRestartAction.step, + "Policy step to restart (defaults to the task's current step)") + tasksCmd.AddCommand(tasksRestartCmd) +} diff --git a/cmd/tasks_skip_step.go b/cmd/tasks_skip_step.go new file mode 100644 index 0000000..5ad6144 --- /dev/null +++ b/cmd/tasks_skip_step.go @@ -0,0 +1,38 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksSkipStepAction = taskAction{ + verb: "skip-step", + step: stepRequired, + confirm: func(id, _, stepID string) string { + return fmt.Sprintf("Skipped policy step: task_id=%s policy_step_id=%s\n", id, stepID) + }, +} + +var tasksSkipStepCmd = &cobra.Command{ + Use: "skip-step ", + Short: "Skip a task's current policy step", + Long: `Skip a task's current approval step, advancing the policy without a decision. + +The step id is required by the server, which rejects a missing one with: + invalid TaskActionsServiceSkipStepRequest.PolicyStepId: value does not match regex pattern "^[a-zA-Z0-9]{27}$" +It defaults to the task's currently executing step, so pass --policy-step-id +only to target a different one. A step id captured before another action is +stale, and the server answers "this action is no longer available". + +The confirmation reports the task id and the step skipped, never a state: the +action endpoints echo the task's state from before the action.`, + Args: cobra.ExactArgs(1), + RunE: tasksSkipStepAction.runTaskAction, +} + +func init() { + addTaskActionFlags(tasksSkipStepCmd, tasksSkipStepAction.step, + "Policy step to skip (defaults to the task's current step)") + tasksCmd.AddCommand(tasksSkipStepCmd) +} diff --git a/cmd/tasks_update_grant_duration.go b/cmd/tasks_update_grant_duration.go new file mode 100644 index 0000000..4d50c7e --- /dev/null +++ b/cmd/tasks_update_grant_duration.go @@ -0,0 +1,58 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksUpdateGrantDurationAction = taskAction{ + verb: "update-grant-duration", + step: stepUnused, + extraBody: func(cmd *cobra.Command, body map[string]any) error { + duration, err := requireNonEmptyIfSet(cmd, "duration") + if err != nil { + return err + } + if duration == "" { + return &usageError{fmt.Errorf("flag --duration is required; the server rejects a missing one with " + + `invalid TaskActionsServiceUpdateGrantDurationRequest.Duration: value is required`)} + } + body["duration"] = duration + return nil + }, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Updated grant duration: task_id=%s\n", id) + }, +} + +var tasksUpdateGrantDurationCmd = &cobra.Command{ + Use: "update-grant-duration ", + Short: "Change the grant duration a task will provision", + Long: `Change how long the access a grant task provisions will last. + +--duration takes a protobuf duration, not a Go one: seconds with an "s" +suffix, e.g. 3600s. "1h" is refused by the server with: + invalid google.protobuf.Duration value "1h" + +The task must still be at an approval step. Once it reaches provisioning the +server refuses with: + cannot update grant duration for a ticket in a provision step + +The new value lands on the task as "grantDuration"; read it back with +"c1i requests get ". + +The confirmation reports only the task id: the action endpoints echo the task's +state from before the action.`, + Args: cobra.ExactArgs(1), + RunE: tasksUpdateGrantDurationAction.runTaskAction, +} + +func init() { + // No --comment: the UpdateGrantDuration request body carries only duration + // and expandMask. + tasksUpdateGrantDurationCmd.Flags().String("duration", "", + `Grant duration as a protobuf duration, e.g. 3600s (required; "1h" is refused)`) + markRequired(tasksUpdateGrantDurationCmd, "duration") + tasksCmd.AddCommand(tasksUpdateGrantDurationCmd) +} From 7ecb49669493a0a8274ed2f6c569a51863ec4790 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:11:03 +0000 Subject: [PATCH 2/7] Document the five new task actions Only claims that were observed live: which actions a task accepts depends on its state, every action rotates the current policy step so a captured --policy-step-id goes stale, restart re-runs a step while reset restarts the policy, neither reopens a closed task, process changes nothing observable on a healthy task, and update-grant-duration lands as grantDuration but is refused once the task reaches provisioning. The README guard caught "" parsing as two positionals. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 26 ++++++++++++++++++++++++++ README.md | 25 +++++++++++++++++++++++++ cmd/agents.md | 17 ++++++++++++++++- 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7676425..088af42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,32 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Added +- **`c1i tasks restart`, `reset`, `skip-step`, `process` and + `update-grant-duration`.** The task action family was five commands wrapping + thirteen server routes; these add the five whose behaviour could be + demonstrated. All ten now share one runner — `approve`, `deny`, `comment`, + `close` and `reassign` each hand-rolled the same request-and-confirm + sequence, and their help text and dry-run output are unchanged. + + Verified by effect rather than exit code. `restart` and `skip-step` rotate + the task's current policy step and add one history entry; `reset` adds four, + because it restarts the policy rather than the step. Neither `restart` nor + `reset` reopens a closed task — the state stays `TASK_STATE_CLOSED`. + `process` changes nothing observable on a healthy task, so expect a result + only where processing had stalled. `update-grant-duration` lands as + `grantDuration` on the task, takes a protobuf duration (`3600s`, not `1h`), + and is refused once the task reaches provisioning with `cannot update grant + duration for a ticket in a provision step`. + + Which actions a task accepts depends on its state; the rest are refused with + `action not permitted`. Read the task's own list with + `c1i api --path /api/v1/tasks/ --fields actions`. + + Not wrapped: `escalate` could not be demonstrated even with emergency grants + enabled on the entitlement, `update-request-data` takes a free-form object + that wants a body-file flag, and `approve-with-step-up` needs a step-up + transaction id the CLI cannot obtain. + - **`c1i entitlements create`.** Modelling a manually-managed app took three raw `api` calls -- resource type, resource, then entitlement -- with the ids hand-carried between them. One command now does it, and reuses objects you diff --git a/README.md b/README.md index ffcd97d..df89793 100644 --- a/README.md +++ b/README.md @@ -204,8 +204,33 @@ c1i tasks deny [--policy-step-id ] [--comment ] c1i tasks comment --comment c1i tasks close [--comment ] c1i tasks reassign --to-user-id [--to-user-id ...] [--policy-step-id ] [--comment ] +c1i tasks restart [--policy-step-id ] [--comment ] +c1i tasks reset [--comment ] +c1i tasks skip-step [--policy-step-id ] [--comment ] +c1i tasks process +c1i tasks update-grant-duration --duration ``` +Every action rotates the task's current policy step, so a `--policy-step-id` +captured earlier goes stale — the server answers `this action is no longer +available: the request has advanced to a new approval step`. Omit the flag to +act on whatever step is current. + +Which actions a task accepts depends on its state; the server refuses the rest +with `action not permitted`. Read the task's own list with +`c1i api --path /api/v1/tasks/ --fields actions`. + +`restart` re-runs the current approval step; `reset` restarts the whole policy. +Neither reopens a closed task. `process` re-evaluates a stalled task and +changes nothing observable on a healthy one. `update-grant-duration` takes a +protobuf duration (`3600s`, not `1h`) and only applies before the task reaches +provisioning, after which the server answers `cannot update grant duration for +a ticket in a provision step`; the value lands as `grantDuration`. + +`escalate`, `update-request-data` and `approve-with-step-up` are not wrapped; +reach them through `c1i api`. + + `approve`/`deny`/`reassign` target a specific policy step. If `--policy-step-id` is omitted, the task's currently executing step is fetched and used automatically for all three — but `approve` and `reassign` require a resolvable diff --git a/cmd/agents.md b/cmd/agents.md index 17448e9..555b0bf 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -326,7 +326,22 @@ resource with `--resource-id` likewise means you drop close`/`reassign` therefore never print a state (`close` reports `task_id`, `reassign` also the `policy_step_id`); if you call these actions through `api`, read the task back rather than trusting the response's - `state`. + `state`. The same holds for `restart`, `reset`, `skip-step`, `process` and + `update-grant-duration`. +- Which actions a task accepts depends on its state, and the server refuses the + rest with `action not permitted`. Read the task's own list first: + `c1i api --path /api/v1/tasks/ --fields actions`. Every action also + rotates the current policy step, so a `--policy-step-id` captured before + another call is stale and answers `this action is no longer available: the + request has advanced to a new approval step` -- omit the flag to act on + whatever step is current. +- `restart` re-runs the current approval step (one new history entry); `reset` + restarts the whole policy (four, measured). Neither reopens a closed task -- + the state stays `TASK_STATE_CLOSED`. `process` changes nothing observable on + a healthy task. `update-grant-duration` needs a protobuf duration (`3600s`, + not `1h`) and only works before provisioning, after which the server says + `cannot update grant duration for a ticket in a provision step`; the value + lands as `grantDuration`. - Entitlement ids are unique only within an app — some system-builtin entitlements reuse the same id across every app that has one. Always key on `(app_id, id)` together, never `id` alone. From b598d9c1a4a95adebf6f2807746220d271d10c9b Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:25:51 +0000 Subject: [PATCH 3/7] Address the review: restore URL validation, pin every action, narrow the docs The runner previewed --dry-run before resolving the URL, so close and comment stopped rejecting a malformed --url and stopped warning when the target came from config rather than the flag. A typo'd tenant previewed happily at exit 0. The URL is resolved first now; credentials are still deferred, which is what each command did before. Parity with origin/main re-diffed for all five. The test tables covered 7 of 10 and 6 of 10 commands, so four plausible regressions passed the whole suite: update-grant-duration posting to /action/reset, update-grant-duration gaining a policy step, deny's stepOptional becoming stepRequired, and comment losing the extraBody that makes an explicit --comment "" reach the wire. One table now seeds from tasksCmd.Commands(), so a command added without a row fails instead of going untested, and deny's omit path and comment's always-send are asserted on the wire. All four regressions now fail. "Every action rotates the current policy step" was both unmeasured and contradicted two paragraphs later by process. Narrowed to the three measured. The stalled-task effect of process is stated as intent, not observation. --comment on restart, reset and skip-step is confirmed to land: commentCount incremented on each. addTaskActionFlags is gone -- three of ten callers, and nothing to enforce since two actions deliberately take no comment. Also drops an unreachable --duration branch, two stale counts in a comment, and refreshes resolvePolicyStepID's doc, which described approve/deny only. Co-Authored-By: Claude Opus 5 --- README.md | 15 +-- cmd/agents.md | 11 +- cmd/tasks.go | 15 +-- cmd/tasks_action.go | 34 +++--- cmd/tasks_action_test.go | 165 ++++++++++++++++++++--------- cmd/tasks_reset.go | 2 +- cmd/tasks_restart.go | 8 +- cmd/tasks_skip_step.go | 4 +- cmd/tasks_update_grant_duration.go | 4 - 9 files changed, 154 insertions(+), 104 deletions(-) diff --git a/README.md b/README.md index df89793..4e2ef33 100644 --- a/README.md +++ b/README.md @@ -211,18 +211,20 @@ c1i tasks process c1i tasks update-grant-duration --duration ``` -Every action rotates the task's current policy step, so a `--policy-step-id` -captured earlier goes stale — the server answers `this action is no longer -available: the request has advanced to a new approval step`. Omit the flag to -act on whatever step is current. +`restart`, `reset` and `skip-step` each rotate the task's current policy step +(measured), so a `--policy-step-id` captured before one of them goes stale — +the server answers `this action is no longer available: the request has +advanced to a new approval step`. Omit the flag to act on whatever step is +current. `process` and `update-grant-duration` take no step. Which actions a task accepts depends on its state; the server refuses the rest with `action not permitted`. Read the task's own list with `c1i api --path /api/v1/tasks/ --fields actions`. `restart` re-runs the current approval step; `reset` restarts the whole policy. -Neither reopens a closed task. `process` re-evaluates a stalled task and -changes nothing observable on a healthy one. `update-grant-duration` takes a +Neither reopens a closed task. `process` changes nothing observable on a +healthy task — it is intended for one that has stalled, which was not +reproduced here. `update-grant-duration` takes a protobuf duration (`3600s`, not `1h`) and only applies before the task reaches provisioning, after which the server answers `cannot update grant duration for a ticket in a provision step`; the value lands as `grantDuration`. @@ -230,7 +232,6 @@ a ticket in a provision step`; the value lands as `grantDuration`. `escalate`, `update-request-data` and `approve-with-step-up` are not wrapped; reach them through `c1i api`. - `approve`/`deny`/`reassign` target a specific policy step. If `--policy-step-id` is omitted, the task's currently executing step is fetched and used automatically for all three — but `approve` and `reassign` require a resolvable diff --git a/cmd/agents.md b/cmd/agents.md index 555b0bf..851a7a9 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -330,11 +330,12 @@ resource with `--resource-id` likewise means you drop `update-grant-duration`. - Which actions a task accepts depends on its state, and the server refuses the rest with `action not permitted`. Read the task's own list first: - `c1i api --path /api/v1/tasks/ --fields actions`. Every action also - rotates the current policy step, so a `--policy-step-id` captured before - another call is stale and answers `this action is no longer available: the - request has advanced to a new approval step` -- omit the flag to act on - whatever step is current. + `c1i api --path /api/v1/tasks/ --fields actions`. `restart`, `reset` and + `skip-step` each rotate the current policy step (measured), so a + `--policy-step-id` captured before one of them is stale and answers `this + action is no longer available: the request has advanced to a new approval + step` -- omit the flag to act on whatever step is current. `process` and + `update-grant-duration` take no step. - `restart` re-runs the current approval step (one new history entry); `reset` restarts the whole policy (four, measured). Neither reopens a closed task -- the state stays `TASK_STATE_CLOSED`. `process` changes nothing observable on diff --git a/cmd/tasks.go b/cmd/tasks.go index cfa4f73..53e751d 100644 --- a/cmd/tasks.go +++ b/cmd/tasks.go @@ -157,14 +157,15 @@ func parseCurrentPolicyStepID(data []byte) (string, error) { return resp.TaskView.Task.Policy.Current.ID, nil } -// resolvePolicyStepID returns the policy step ID to use for an approve/deny -// action. If the user supplied one explicitly it is used as-is; otherwise the -// task is fetched and its currently executing step ID is used. +// resolvePolicyStepID returns the policy step id an action should target: +// the explicit --policy-step-id when given, otherwise the task's currently +// executing step, fetched with a GET. // -// approve requires policyStepId, so callers pass required=true to turn an -// underivable step into an error. deny treats it as optional (the API does -// not require it), so it passes required=false and simply omits the field -// when no current step can be derived. +// required distinguishes the two modes callers need. Actions the server +// rejects without a step (approve, skip-step) and those we refuse to send +// ambiguously (reassign, restart) pass true and get an error. deny passes +// false: when the step cannot be derived the field is omitted rather than +// blocking the denial. func resolvePolicyStepID(ctx context.Context, c *client.Client, taskID, explicit string, required bool) (string, error) { if explicit != "" { return explicit, nil diff --git a/cmd/tasks_action.go b/cmd/tasks_action.go index bc1463c..29a9fb6 100644 --- a/cmd/tasks_action.go +++ b/cmd/tasks_action.go @@ -17,8 +17,8 @@ const ( ) // taskAction describes one POST /api/v1/tasks/{id}/action/{verb} command. The -// eleven action commands differ only in these fields, so they share one RunE: -// hand-rolling each was already six near-identical copies before this. +// action commands differ only in these fields, so they share one RunE: five +// near-identical copies existed before this. type taskAction struct { verb string // the path segment, e.g. "restart" step policyStepMode @@ -52,20 +52,21 @@ func (a taskAction) runTaskAction(cmd *cobra.Command, args []string) error { body["comment"] = comment } - // An action that needs no policy step needs no client to preview, so - // --dry-run works without credentials. Resolving a step requires a GET, so - // those actions must authenticate first even for a preview — which is what - // each command did before sharing this runner. - if a.step == stepUnused { - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - } - + // The URL is resolved even for a preview: --dry-run answers "am I about to + // do this to the right tenant", so it must still reject a bad --url and + // still warn when the target came from config rather than the flag. baseURL, err := GetBaseURL() if err != nil { return err } + + // Credentials, though, are only needed to send. An action that takes no + // policy step can preview without them; resolving a step needs a GET, so + // those must authenticate first — which is what each command did before + // sharing this runner. + if a.step == stepUnused && dryRunActive() { + return printDryRun(cmd, "POST", path, body) + } c, err := newClient(cmd, baseURL) if err != nil { return fmt.Errorf("authentication failed: %w", err) @@ -98,12 +99,3 @@ func (a taskAction) runTaskAction(cmd *cobra.Command, args []string) error { _, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s", a.confirm(id, state, stepID)) return nil } - -// addTaskActionFlags registers the flags the shared RunE reads. Only --comment -// is universal; --policy-step-id is registered when the action uses one. -func addTaskActionFlags(cmd *cobra.Command, step policyStepMode, stepUsage string) { - cmd.Flags().String("comment", "", "Optional comment") - if step != stepUnused { - cmd.Flags().String("policy-step-id", "", stepUsage) - } -} diff --git a/cmd/tasks_action_test.go b/cmd/tasks_action_test.go index 585ce8f..424a615 100644 --- a/cmd/tasks_action_test.go +++ b/cmd/tasks_action_test.go @@ -47,78 +47,137 @@ func newTaskActionRecorder(t *testing.T, state, currentStepID string) *taskActio return r } -// TestTaskActionsPostTheirOwnVerb pins that each command hits its own action -// path. Sharing one RunE makes a copied verb the plausible mistake, and the -// wrong verb would silently perform a different action on the task. -func TestTaskActionsPostTheirOwnVerb(t *testing.T) { - cases := []struct { - cmdName string - wantPath string - }{ - {"restart", "/api/v1/tasks/" + actionTestTaskID + "/action/restart"}, - {"reset", "/api/v1/tasks/" + actionTestTaskID + "/action/reset"}, - {"skip-step", "/api/v1/tasks/" + actionTestTaskID + "/action/skip-step"}, - {"process", "/api/v1/tasks/" + actionTestTaskID + "/action/process"}, - {"close", "/api/v1/tasks/" + actionTestTaskID + "/action/close"}, - {"approve", "/api/v1/tasks/" + actionTestTaskID + "/action/approve"}, - {"deny", "/api/v1/tasks/" + actionTestTaskID + "/action/deny"}, - } - for _, tc := range cases { - t.Run(tc.cmdName, func(t *testing.T) { - cmd := findTasksSubcommand(t, tc.cmdName) - resetCmds(t, cmd) - r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") - if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { - t.Fatalf("%s: %v", tc.cmdName, err) - } - if len(r.paths) != 1 { - t.Fatalf("%s posted %d times, want 1: %v", tc.cmdName, len(r.paths), r.paths) - } - if r.paths[0] != tc.wantPath { - t.Errorf("%s posted %q, want %q", tc.cmdName, r.paths[0], tc.wantPath) - } - }) +// taskActionExpectations pins, per action command, the path it must POST and +// whether its body carries policyStepId. Seeded against tasksCmd.Commands() +// by TestEveryTaskActionIsPinned, so adding a command without adding a row +// here fails rather than going silently untested. +var taskActionExpectations = map[string]struct { + verb string + wantStep bool + // setup supplies flags the command requires before it will run. + setup func(cmd *cobra.Command) +}{ + "approve": {verb: "approve", wantStep: true}, + "deny": {verb: "deny", wantStep: true}, + "close": {verb: "close", wantStep: false}, + "restart": {verb: "restart", wantStep: true}, + "reset": {verb: "reset", wantStep: false}, + "skip-step": {verb: "skip-step", wantStep: true}, + "process": {verb: "process", wantStep: false}, + "comment": {verb: "comment", wantStep: false, setup: func(c *cobra.Command) { _ = c.Flags().Set("comment", "zz") }}, + "update-grant-duration": {verb: "update-grant-duration", wantStep: false, setup: func(c *cobra.Command) { _ = c.Flags().Set("duration", "3600s") }}, + "reassign": {verb: "reassign", wantStep: true, setup: func(c *cobra.Command) { _ = c.Flags().Set("to-user-id", "zz-user") }}, +} + +// nonActionTaskSubcommands are the tasks subcommands that are not action POSTs. +var nonActionTaskSubcommands = map[string]bool{"list": true} + +// TestEveryTaskActionIsPinned is the guard on the guard: every action command +// in the tree must have a row above. +func TestEveryTaskActionIsPinned(t *testing.T) { + seen := 0 + for _, c := range tasksCmd.Commands() { + name := c.Name() + if nonActionTaskSubcommands[name] { + continue + } + seen++ + if _, ok := taskActionExpectations[name]; !ok { + t.Errorf("tasks %s has no row in taskActionExpectations, so nothing pins its action path or policy-step behaviour", name) + } + } + if seen == 0 { + t.Fatal("found no task action commands — this guard is not looking at what it thinks it is") + } + for name := range taskActionExpectations { + if findTasksSubcommandOrNil(name) == nil { + t.Errorf("taskActionExpectations lists %q, which is no longer a tasks subcommand", name) + } } } -// TestTaskActionsSendPolicyStepOnlyWhenTheyUseOne pins the three step modes. -// Sending policyStepId to an endpoint that takes none, or omitting it where the -// server requires it, are both silent-wrong-request failures. -func TestTaskActionsSendPolicyStepOnlyWhenTheyUseOne(t *testing.T) { +// TestEveryTaskActionPostsItsOwnVerbAndStep drives every pinned command and +// checks both the path and whether policyStepId is on the wire. A copied verb +// would perform a different action on the task while printing success. +func TestEveryTaskActionPostsItsOwnVerbAndStep(t *testing.T) { const step = "zz-step-1111111111111111111" - cases := []struct { - cmdName string - wantStep bool - }{ - {"restart", true}, - {"skip-step", true}, - {"approve", true}, - {"reset", false}, - {"process", false}, - {"close", false}, - } - for _, tc := range cases { - t.Run(tc.cmdName, func(t *testing.T) { - cmd := findTasksSubcommand(t, tc.cmdName) + for name, want := range taskActionExpectations { + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) resetCmds(t, cmd) + if want.setup != nil { + want.setup(cmd) + } r := newTaskActionRecorder(t, "TASK_STATE_OPEN", step) if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { - t.Fatalf("%s: %v", tc.cmdName, err) + t.Fatalf("%s: %v", name, err) + } + if len(r.paths) != 1 { + t.Fatalf("%s posted %d times, want 1: %v", name, len(r.paths), r.paths) + } + if got, wantPath := r.paths[0], "/api/v1/tasks/"+actionTestTaskID+"/action/"+want.verb; got != wantPath { + t.Errorf("%s posted %q, want %q", name, got, wantPath) } got, ok := r.bodies[0]["policyStepId"] - if tc.wantStep { + if want.wantStep { if !ok || got != step { - t.Errorf("%s body policyStepId = %v (present=%v), want %q", tc.cmdName, got, ok, step) + t.Errorf("%s body policyStepId = %v (present=%v), want %q", name, got, ok, step) } return } if ok { - t.Errorf("%s sent policyStepId=%v to an endpoint that takes none", tc.cmdName, got) + t.Errorf("%s sent policyStepId=%v to an endpoint that takes none", name, got) } }) } } +// TestTasksCommentAlwaysSendsTheCommentKey pins the one action whose empty +// value must still reach the wire: the comment IS the payload, so an omitted +// key records nothing while the command still prints success. +func TestTasksCommentAlwaysSendsTheCommentKey(t *testing.T) { + cmd := findTasksSubcommand(t, "comment") + resetCmds(t, cmd) + _ = cmd.Flags().Set("comment", "") + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("comment: %v", err) + } + got, ok := r.bodies[0]["comment"] + if !ok || got != "" { + t.Errorf("comment body = %v (present=%v), want an empty string present", got, ok) + } +} + +// TestTasksDenyOmitsAnUnresolvableStep pins deny's stepOptional mode on the +// wire: when the current step cannot be derived the field must be absent, not +// empty, and the denial must still go through. +func TestTasksDenyOmitsAnUnresolvableStep(t *testing.T) { + cmd := findTasksSubcommand(t, "deny") + resetCmds(t, cmd) + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "") // no current step + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("deny: %v", err) + } + if got, ok := r.bodies[0]["policyStepId"]; ok { + t.Errorf("deny sent policyStepId=%v when no step could be resolved; the field must be omitted", got) + } + if len(r.paths) != 1 { + t.Errorf("deny posted %d times, want 1", len(r.paths)) + } +} + +// findTasksSubcommandOrNil is findTasksSubcommand without the fatal, for the +// reverse direction of the pinning check. +func findTasksSubcommandOrNil(name string) *cobra.Command { + for _, c := range tasksCmd.Commands() { + if c.Name() == name { + return c + } + } + return nil +} + // TestTaskActionsNeverEchoResponseState extends the guarantee close already // had to every action that does not intend to print state: these endpoints // return the task as it was BEFORE the action, so echoing it reports the old diff --git a/cmd/tasks_reset.go b/cmd/tasks_reset.go index ec5ea4e..1f9c6b9 100644 --- a/cmd/tasks_reset.go +++ b/cmd/tasks_reset.go @@ -30,6 +30,6 @@ state from before the action.`, } func init() { - addTaskActionFlags(tasksResetCmd, tasksResetAction.step, "") + tasksResetCmd.Flags().String("comment", "", "Optional comment") tasksCmd.AddCommand(tasksResetCmd) } diff --git a/cmd/tasks_restart.go b/cmd/tasks_restart.go index 770c0ca..849321e 100644 --- a/cmd/tasks_restart.go +++ b/cmd/tasks_restart.go @@ -32,8 +32,8 @@ Whether restart is available at all depends on the task; the server refuses with "action not permitted" otherwise. Check the task's own action list: c1i api --path /api/v1/tasks/ --fields actions -Every action rotates the current policy step, so a --policy-step-id captured -earlier goes stale and the server answers: +restart, reset and skip-step each rotate the current policy step, so a +--policy-step-id captured before one of them goes stale and the server answers: this action is no longer available: the request has advanced to a new approval step Omit the flag to act on whatever step is current. @@ -44,7 +44,7 @@ action endpoints echo the task's state from before the action.`, } func init() { - addTaskActionFlags(tasksRestartCmd, tasksRestartAction.step, - "Policy step to restart (defaults to the task's current step)") + tasksRestartCmd.Flags().String("comment", "", "Optional comment") + tasksRestartCmd.Flags().String("policy-step-id", "", "Policy step to restart (defaults to the task's current step)") tasksCmd.AddCommand(tasksRestartCmd) } diff --git a/cmd/tasks_skip_step.go b/cmd/tasks_skip_step.go index 5ad6144..f2c39da 100644 --- a/cmd/tasks_skip_step.go +++ b/cmd/tasks_skip_step.go @@ -32,7 +32,7 @@ action endpoints echo the task's state from before the action.`, } func init() { - addTaskActionFlags(tasksSkipStepCmd, tasksSkipStepAction.step, - "Policy step to skip (defaults to the task's current step)") + tasksSkipStepCmd.Flags().String("comment", "", "Optional comment") + tasksSkipStepCmd.Flags().String("policy-step-id", "", "Policy step to skip (defaults to the task's current step)") tasksCmd.AddCommand(tasksSkipStepCmd) } diff --git a/cmd/tasks_update_grant_duration.go b/cmd/tasks_update_grant_duration.go index 4d50c7e..989e610 100644 --- a/cmd/tasks_update_grant_duration.go +++ b/cmd/tasks_update_grant_duration.go @@ -14,10 +14,6 @@ var tasksUpdateGrantDurationAction = taskAction{ if err != nil { return err } - if duration == "" { - return &usageError{fmt.Errorf("flag --duration is required; the server rejects a missing one with " + - `invalid TaskActionsServiceUpdateGrantDurationRequest.Duration: value is required`)} - } body["duration"] = duration return nil }, From fb7d450b5e0e4529a38cf7030385b63dc549cfcf Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:39:36 +0000 Subject: [PATCH 4/7] Pin the dry-run contract and the step modes; align the docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings from the delta review. The fix this branch made to --dry-run had no test: reverting it, or deleting either dry-run short-circuit so a preview really POSTs, all passed the suite. Every action is now driven with dry_run set against a server that fails the test on any POST, and the preview path is asserted. A bool could not distinguish stepOptional from stepRequired, so flipping approve or skip-step to optional survived — they would have silently posted with no policyStepId instead of erroring. The table carries the mode itself now, and a second test drives each mode with no derivable step: required must error before sending, optional must send without the field. update-grant-duration's one payload key was unpinned, and "grantDuration" is the plausible wrong name because that is what the response carries and what the docs quote. Pinned both ways. Its unset --duration path is pinned through the root command, since the direct-RunE harness never sees cobra's required check. Docs: reset rotates the step but takes no --policy-step-id, and the narrowed wording had put it in the group that does. The changelog said the opposite of the README about reset, and process's stalled-task effect was hedged in only one of four places. One wording now, true on each surface. resolvePolicyStepID's doc claimed restart passes required=true; it is stepOptional, which is what lets it act on a closed task. Also drops a doubled "(required)" marker caused by a usage string containing "(required;". Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 12 +-- README.md | 5 +- cmd/agents.md | 5 +- cmd/tasks.go | 8 +- cmd/tasks_action_test.go | 137 ++++++++++++++++++++++++++--- cmd/tasks_process.go | 8 +- cmd/tasks_restart.go | 3 +- cmd/tasks_update_grant_duration.go | 2 +- cmd/usage_exit_codes_test.go | 9 ++ 9 files changed, 156 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 088af42..9fb4c5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,12 +15,12 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `close` and `reassign` each hand-rolled the same request-and-confirm sequence, and their help text and dry-run output are unchanged. - Verified by effect rather than exit code. `restart` and `skip-step` rotate - the task's current policy step and add one history entry; `reset` adds four, - because it restarts the policy rather than the step. Neither `restart` nor - `reset` reopens a closed task — the state stays `TASK_STATE_CLOSED`. - `process` changes nothing observable on a healthy task, so expect a result - only where processing had stalled. `update-grant-duration` lands as + Verified by effect rather than exit code. `restart`, `reset` and `skip-step` + all rotate the task's current policy step; `restart` and `skip-step` add one + history entry, `reset` four, because it restarts the policy rather than the + step. Neither `restart` nor `reset` reopens a closed task — the state stays + `TASK_STATE_CLOSED`. `process` changes nothing observable on a healthy task; + it is intended for one that has stalled, which was not reproduced here. `update-grant-duration` lands as `grantDuration` on the task, takes a protobuf duration (`3600s`, not `1h`), and is refused once the task reaches provisioning with `cannot update grant duration for a ticket in a provision step`. diff --git a/README.md b/README.md index 4e2ef33..9891ad4 100644 --- a/README.md +++ b/README.md @@ -214,8 +214,9 @@ c1i tasks update-grant-duration --duration `restart`, `reset` and `skip-step` each rotate the task's current policy step (measured), so a `--policy-step-id` captured before one of them goes stale — the server answers `this action is no longer available: the request has -advanced to a new approval step`. Omit the flag to act on whatever step is -current. `process` and `update-grant-duration` take no step. +advanced to a new approval step`. Only `restart` and `skip-step` accept +`--policy-step-id`; omit it to act on whatever step is current. `reset`, +`process` and `update-grant-duration` take no step argument. Which actions a task accepts depends on its state; the server refuses the rest with `action not permitted`. Read the task's own list with diff --git a/cmd/agents.md b/cmd/agents.md index 851a7a9..dbd3a89 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -334,8 +334,9 @@ resource with `--resource-id` likewise means you drop `skip-step` each rotate the current policy step (measured), so a `--policy-step-id` captured before one of them is stale and answers `this action is no longer available: the request has advanced to a new approval - step` -- omit the flag to act on whatever step is current. `process` and - `update-grant-duration` take no step. + step`. Only `restart` and `skip-step` accept `--policy-step-id`; omit it to + act on whatever step is current. `reset`, `process` and + `update-grant-duration` take no step argument. - `restart` re-runs the current approval step (one new history entry); `reset` restarts the whole policy (four, measured). Neither reopens a closed task -- the state stays `TASK_STATE_CLOSED`. `process` changes nothing observable on diff --git a/cmd/tasks.go b/cmd/tasks.go index 53e751d..c649885 100644 --- a/cmd/tasks.go +++ b/cmd/tasks.go @@ -162,10 +162,10 @@ func parseCurrentPolicyStepID(data []byte) (string, error) { // executing step, fetched with a GET. // // required distinguishes the two modes callers need. Actions the server -// rejects without a step (approve, skip-step) and those we refuse to send -// ambiguously (reassign, restart) pass true and get an error. deny passes -// false: when the step cannot be derived the field is omitted rather than -// blocking the denial. +// rejects without a step (approve, skip-step) and reassign, which we refuse to +// send ambiguously, pass true and get an error. deny and restart pass false: +// when the step cannot be derived the field is omitted rather than blocking +// the action, which is what lets restart act on a closed task. func resolvePolicyStepID(ctx context.Context, c *client.Client, taskID, explicit string, required bool) (string, error) { if explicit != "" { return explicit, nil diff --git a/cmd/tasks_action_test.go b/cmd/tasks_action_test.go index 424a615..9b78d21 100644 --- a/cmd/tasks_action_test.go +++ b/cmd/tasks_action_test.go @@ -8,7 +8,11 @@ import ( "strings" "testing" + "bytes" + "context" + "github.com/spf13/cobra" + "github.com/spf13/viper" ) const actionTestTaskID = "zz-c1i-test-task-2" @@ -52,21 +56,21 @@ func newTaskActionRecorder(t *testing.T, state, currentStepID string) *taskActio // by TestEveryTaskActionIsPinned, so adding a command without adding a row // here fails rather than going silently untested. var taskActionExpectations = map[string]struct { - verb string - wantStep bool + verb string + step policyStepMode // setup supplies flags the command requires before it will run. setup func(cmd *cobra.Command) }{ - "approve": {verb: "approve", wantStep: true}, - "deny": {verb: "deny", wantStep: true}, - "close": {verb: "close", wantStep: false}, - "restart": {verb: "restart", wantStep: true}, - "reset": {verb: "reset", wantStep: false}, - "skip-step": {verb: "skip-step", wantStep: true}, - "process": {verb: "process", wantStep: false}, - "comment": {verb: "comment", wantStep: false, setup: func(c *cobra.Command) { _ = c.Flags().Set("comment", "zz") }}, - "update-grant-duration": {verb: "update-grant-duration", wantStep: false, setup: func(c *cobra.Command) { _ = c.Flags().Set("duration", "3600s") }}, - "reassign": {verb: "reassign", wantStep: true, setup: func(c *cobra.Command) { _ = c.Flags().Set("to-user-id", "zz-user") }}, + "approve": {verb: "approve", step: stepRequired}, + "deny": {verb: "deny", step: stepOptional}, + "close": {verb: "close", step: stepUnused}, + "restart": {verb: "restart", step: stepOptional}, + "reset": {verb: "reset", step: stepUnused}, + "skip-step": {verb: "skip-step", step: stepRequired}, + "process": {verb: "process", step: stepUnused}, + "comment": {verb: "comment", step: stepUnused, setup: func(c *cobra.Command) { _ = c.Flags().Set("comment", "zz") }}, + "update-grant-duration": {verb: "update-grant-duration", step: stepUnused, setup: func(c *cobra.Command) { _ = c.Flags().Set("duration", "3600s") }}, + "reassign": {verb: "reassign", step: stepRequired, setup: func(c *cobra.Command) { _ = c.Flags().Set("to-user-id", "zz-user") }}, } // nonActionTaskSubcommands are the tasks subcommands that are not action POSTs. @@ -119,7 +123,7 @@ func TestEveryTaskActionPostsItsOwnVerbAndStep(t *testing.T) { t.Errorf("%s posted %q, want %q", name, got, wantPath) } got, ok := r.bodies[0]["policyStepId"] - if want.wantStep { + if want.step != stepUnused { if !ok || got != step { t.Errorf("%s body policyStepId = %v (present=%v), want %q", name, got, ok, step) } @@ -254,3 +258,110 @@ func findTasksSubcommand(t *testing.T, name string) *cobra.Command { t.Fatalf("tasks has no %q subcommand", name) return nil } + +// TestEveryTaskActionModeBehavesOnAnUnresolvableStep is what separates +// stepRequired from stepOptional, which a "does it send the field" check +// cannot see: with no derivable step, required must error before sending and +// optional must send without the field. +func TestEveryTaskActionModeBehavesOnAnUnresolvableStep(t *testing.T) { + for name, want := range taskActionExpectations { + if want.step == stepUnused { + continue + } + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if want.setup != nil { + want.setup(cmd) + } + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "") // no current step + _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID) + if want.step == stepRequired { + if err == nil { + t.Fatalf("%s is stepRequired but succeeded with no derivable step", name) + } + if got := exitCode(err); got != exitUsage { + t.Errorf("%s exitCode = %d, want %d (exitUsage); err = %v", name, got, exitUsage, err) + } + if len(r.paths) != 0 { + t.Errorf("%s sent a request despite requiring a step: %v", name, r.paths) + } + return + } + // stepOptional: proceed, with the field omitted. + if err != nil { + t.Fatalf("%s is stepOptional but failed with no derivable step: %v", name, err) + } + if _, ok := r.bodies[0]["policyStepId"]; ok { + t.Errorf("%s sent policyStepId when none could be resolved", name) + } + }) + } +} + +// TestTaskActionsDryRunNeverSends is the guard on this branch's own regression: +// --dry-run previewed before the URL was resolved, so a typo'd tenant previewed +// happily. It must also never reach the wire, for every action. +func TestTaskActionsDryRunNeverSends(t *testing.T) { + for name, want := range taskActionExpectations { + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if want.setup != nil { + want.setup(cmd) + } + var posted []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Set("Content-Type", "application/json") + if req.Method == http.MethodPost { + posted = append(posted, req.URL.Path) + t.Errorf("--dry-run sent a real POST to %s", req.URL.Path) + } + // The GET is the policy-step lookup, which a preview may make. + _, _ = w.Write([]byte(`{"taskView":{"task":{"id":"` + actionTestTaskID + + `","state":"TASK_STATE_OPEN","policy":{"current":{"id":"zz-step-1111111111111111111"}}}}}`)) + })) + t.Cleanup(srv.Close) + + stubNewClient(t, srv) + t.Setenv("C1I_URL", "https://example.invalid") + viper.Set("dry_run", true) + t.Cleanup(func() { viper.Set("dry_run", false) }) + + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetContext(context.Background()) + if err := cmd.RunE(cmd, []string{actionTestTaskID}); err != nil { + t.Fatalf("%s --dry-run: %v", name, err) + } + if len(posted) != 0 { + t.Errorf("%s posted during a dry run: %v", name, posted) + } + if !strings.Contains(out.String(), "[dry-run]") { + t.Errorf("%s printed no preview: %q", name, out.String()) + } + if !strings.Contains(out.String(), "/action/"+want.verb) { + t.Errorf("%s previewed the wrong path: %q", name, out.String()) + } + }) + } +} + +// TestTasksUpdateGrantDurationSendsDurationKey pins the one payload key this +// command exists to send. "grantDuration" is the plausible wrong name — that +// is what the RESPONSE carries, and what the docs quote. +func TestTasksUpdateGrantDurationSendsDurationKey(t *testing.T) { + cmd := findTasksSubcommand(t, "update-grant-duration") + resetCmds(t, cmd) + _ = cmd.Flags().Set("duration", "3600s") + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("update-grant-duration: %v", err) + } + if got, ok := r.bodies[0]["duration"]; !ok || got != "3600s" { + t.Errorf(`body["duration"] = %v (present=%v), want "3600s"`, got, ok) + } + if _, ok := r.bodies[0]["grantDuration"]; ok { + t.Error(`body carries "grantDuration"; that is the response field, not the request's`) + } +} diff --git a/cmd/tasks_process.go b/cmd/tasks_process.go index efa71b8..3edf87d 100644 --- a/cmd/tasks_process.go +++ b/cmd/tasks_process.go @@ -19,13 +19,13 @@ var tasksProcessCmd = &cobra.Command{ Short: "Process a task now rather than waiting for the next cycle", Long: `Ask C1 to process a task immediately instead of on its normal schedule. -Useful when a task looks stuck: it re-runs the policy evaluation without -changing the task's approval state. The request body is empty — this action +Intended for a task that looks stuck: it asks C1 to re-run the policy +evaluation without changing the task's approval state. The request body is empty — this action takes neither a comment nor a policy step. On a healthy task nothing observable changes: state, current policy step and -history are all identical afterwards. Expect a result only where processing had -genuinely stalled. +history are all identical afterwards. The stalled case was not reproduced, so +treat any effect there as unverified. The confirmation reports only the task id: the action endpoints echo the task's state from before the action, and processing is asynchronous, so re-read the diff --git a/cmd/tasks_restart.go b/cmd/tasks_restart.go index 849321e..70e9b45 100644 --- a/cmd/tasks_restart.go +++ b/cmd/tasks_restart.go @@ -35,7 +35,8 @@ with "action not permitted" otherwise. Check the task's own action list: restart, reset and skip-step each rotate the current policy step, so a --policy-step-id captured before one of them goes stale and the server answers: this action is no longer available: the request has advanced to a new approval step -Omit the flag to act on whatever step is current. +Omit the flag to act on whatever step is current. (reset takes no step +argument; it restarts the whole policy.) The confirmation reports the task id and the step acted on, never a state: the action endpoints echo the task's state from before the action.`, diff --git a/cmd/tasks_update_grant_duration.go b/cmd/tasks_update_grant_duration.go index 989e610..8ea6ee9 100644 --- a/cmd/tasks_update_grant_duration.go +++ b/cmd/tasks_update_grant_duration.go @@ -48,7 +48,7 @@ func init() { // No --comment: the UpdateGrantDuration request body carries only duration // and expandMask. tasksUpdateGrantDurationCmd.Flags().String("duration", "", - `Grant duration as a protobuf duration, e.g. 3600s (required; "1h" is refused)`) + `Grant duration as a protobuf duration, e.g. 3600s; "1h" is refused`) markRequired(tasksUpdateGrantDurationCmd, "duration") tasksCmd.AddCommand(tasksUpdateGrantDurationCmd) } diff --git a/cmd/usage_exit_codes_test.go b/cmd/usage_exit_codes_test.go index f852b0b..bdae882 100644 --- a/cmd/usage_exit_codes_test.go +++ b/cmd/usage_exit_codes_test.go @@ -90,6 +90,15 @@ func TestValidationGuardsExitUsage(t *testing.T) { wantMsg: "--classification requires a non-empty value", cmds: []*cobra.Command{mcpToolsSearchCmd}, }, + { + // markRequired is the only thing stopping {"duration": ""} on the + // wire; runTaskActionCmd calls RunE directly and never sees cobra's + // required check, so this row is what pins it. + name: "tasks update-grant-duration: --duration missing", + args: []string{"tasks", "update-grant-duration", "zz-task-1"}, + wantMsg: `required flag(s) "duration" not set`, + cmds: []*cobra.Command{tasksUpdateGrantDurationCmd}, + }, { // The only repeatable flag whose READ was unpinned end to end: the // existing --config-field row passes a non-empty bad pair, which From f6f42f680f6ed092d5ff6b42885b69a76bd5d299 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:41:46 +0000 Subject: [PATCH 5/7] Pin the URL ordering the dry-run fix depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six of the seven mutations the review found were closed by the previous commit; reverting the URL fix itself still passed. The other dry-run tests stub the client and pass a valid URL, so the ordering is invisible to them — it only matters when the URL cannot be resolved. This drives a stepless and a step-using action through the root command with a malformed --url under --dry-run, and requires exit 2 with no preview printed. Verified against the mutation: reverting the ordering now fails. Co-Authored-By: Claude Opus 5 --- cmd/tasks_action_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/cmd/tasks_action_test.go b/cmd/tasks_action_test.go index 9b78d21..b73ebb0 100644 --- a/cmd/tasks_action_test.go +++ b/cmd/tasks_action_test.go @@ -365,3 +365,35 @@ func TestTasksUpdateGrantDurationSendsDurationKey(t *testing.T) { t.Error(`body carries "grantDuration"; that is the response field, not the request's`) } } + +// TestTaskActionsDryRunStillResolvesTheURL is the guard on the ordering this +// branch had to fix twice. --dry-run answers "am I about to do this to the +// right tenant", so it must still reject a bad --url. Previewing before +// resolving the URL made a typo'd tenant preview happily at exit 0, and no +// other test could see it: the rest stub the client and pass a valid URL. +func TestTaskActionsDryRunStillResolvesTheURL(t *testing.T) { + // Both modes, since the runner resolves the URL once for each path. + for _, name := range []string{"close", "restart"} { + t.Run(name, func(t *testing.T) { + resetRootURLFlag(t) + t.Setenv("C1I_URL", "") + viper.Set("dry_run", true) + t.Cleanup(func() { viper.Set("dry_run", false) }) + + var out bytes.Buffer + rootCmd.SetOut(&out) + rootCmd.SetErr(&out) + rootCmd.SetArgs([]string{"tasks", name, actionTestTaskID, "--dry-run", "--url", "not a url"}) + err := rootCmd.ExecuteContext(t.Context()) + if err == nil { + t.Fatalf("tasks %s --dry-run accepted a malformed --url; a typo'd tenant previews as though it were real", name) + } + if got := exitCode(err); got != exitUsage { + t.Errorf("exitCode = %d, want %d (exitUsage); err = %v", got, exitUsage, err) + } + if strings.Contains(out.String(), "[dry-run]") { + t.Errorf("tasks %s printed a preview for an unresolvable URL: %q", name, out.String()) + } + }) + } +} From 6b925fc972ed4b06210daabb611097a43b75d602 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 07:02:11 +0000 Subject: [PATCH 6/7] Cut the prose I kept getting wrong, and shorten the comments The flag enumeration in README and the agent doc was wrong twice: it named two commands as accepting --policy-step-id when five do. The synopsis block above it already states this per command, so the sentence is gone rather than fixed a third time. Pins two contracts that were stated only in prose: a stepless action previews without credentials, and a bad --url still fails under --dry-run. Both were shown to regress silently. A leaked SetOut on package-level commands had made one of those assertions unreachable; dry-run state is now saved and restored like every other site in the repo. Documents that restart and skip-step authenticate under --dry-run, since they fetch the step. Comments trimmed throughout to the non-obvious fact. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 +- README.md | 10 ++- cmd/agents.md | 4 +- cmd/tasks_action.go | 29 ++++----- cmd/tasks_action_test.go | 136 +++++++++++++++++++++++++++++---------- cmd/tasks_comment.go | 5 +- cmd/tasks_restart.go | 8 ++- cmd/tasks_skip_step.go | 3 +- 8 files changed, 128 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fb4c5f..6faaa15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). history entry, `reset` four, because it restarts the policy rather than the step. Neither `restart` nor `reset` reopens a closed task — the state stays `TASK_STATE_CLOSED`. `process` changes nothing observable on a healthy task; - it is intended for one that has stalled, which was not reproduced here. `update-grant-duration` lands as + the stalled case was not reproduced. `update-grant-duration` lands as `grantDuration` on the task, takes a protobuf duration (`3600s`, not `1h`), and is refused once the task reaches provisioning with `cannot update grant duration for a ticket in a provision step`. diff --git a/README.md b/README.md index 9891ad4..4fdba44 100644 --- a/README.md +++ b/README.md @@ -211,12 +211,10 @@ c1i tasks process c1i tasks update-grant-duration --duration ``` -`restart`, `reset` and `skip-step` each rotate the task's current policy step -(measured), so a `--policy-step-id` captured before one of them goes stale — -the server answers `this action is no longer available: the request has -advanced to a new approval step`. Only `restart` and `skip-step` accept -`--policy-step-id`; omit it to act on whatever step is current. `reset`, -`process` and `update-grant-duration` take no step argument. +`restart`, `reset` and `skip-step` each rotate the task's current policy step, +so a `--policy-step-id` captured before one of them goes stale — the server +answers `this action is no longer available: the request has advanced to a new +approval step`. Omit the flag to act on whatever step is current. Which actions a task accepts depends on its state; the server refuses the rest with `action not permitted`. Read the task's own list with diff --git a/cmd/agents.md b/cmd/agents.md index dbd3a89..d4f65f2 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -334,9 +334,7 @@ resource with `--resource-id` likewise means you drop `skip-step` each rotate the current policy step (measured), so a `--policy-step-id` captured before one of them is stale and answers `this action is no longer available: the request has advanced to a new approval - step`. Only `restart` and `skip-step` accept `--policy-step-id`; omit it to - act on whatever step is current. `reset`, `process` and - `update-grant-duration` take no step argument. + step` -- omit the flag to act on whatever step is current. - `restart` re-runs the current approval step (one new history entry); `reset` restarts the whole policy (four, measured). Neither reopens a closed task -- the state stays `TASK_STATE_CLOSED`. `process` changes nothing observable on diff --git a/cmd/tasks_action.go b/cmd/tasks_action.go index 29a9fb6..2ea0877 100644 --- a/cmd/tasks_action.go +++ b/cmd/tasks_action.go @@ -16,23 +16,21 @@ const ( stepRequired // the server rejects the call without it ) -// taskAction describes one POST /api/v1/tasks/{id}/action/{verb} command. The -// action commands differ only in these fields, so they share one RunE: five -// near-identical copies existed before this. +// taskAction describes one POST /api/v1/tasks/{id}/action/{verb} command. +// These fields are all the commands differ by, so they share one RunE. type taskAction struct { verb string // the path segment, e.g. "restart" step policyStepMode - // extraBody adds fields beyond comment/policyStepId. It runs before the - // request is built, so it may also reject bad flag combinations. + // extraBody adds fields beyond comment/policyStepId, and may reject bad + // flag combinations. Runs before any client is built, so it exits 2. extraBody func(cmd *cobra.Command, body map[string]any) error - // confirm formats the success line. State is passed but most actions must - // not print it — see runTaskAction. + // confirm formats the success line. State is passed but is the task's + // PRE-action state, so most actions must not print it. confirm func(id, state, stepID string) string } -// runTaskAction is the shared RunE. Ordering matters and matches the rest of -// the repo: flags are validated before a client is built, so a usage error -// exits 2 rather than failing on credentials first. +// runTaskAction is the shared RunE. Flags are validated before a client is +// built, so a usage error exits 2 rather than failing on credentials. func (a taskAction) runTaskAction(cmd *cobra.Command, args []string) error { var comment string if cmd.Flags().Lookup("comment") != nil { @@ -52,18 +50,15 @@ func (a taskAction) runTaskAction(cmd *cobra.Command, args []string) error { body["comment"] = comment } - // The URL is resolved even for a preview: --dry-run answers "am I about to - // do this to the right tenant", so it must still reject a bad --url and - // still warn when the target came from config rather than the flag. + // Resolved even for a preview, so --dry-run still rejects a bad --url and + // still names the tenant it would hit. baseURL, err := GetBaseURL() if err != nil { return err } - // Credentials, though, are only needed to send. An action that takes no - // policy step can preview without them; resolving a step needs a GET, so - // those must authenticate first — which is what each command did before - // sharing this runner. + // Credentials are only needed to send, or to fetch a step. Matches what + // each command did before sharing this runner. if a.step == stepUnused && dryRunActive() { return printDryRun(cmd, "POST", path, body) } diff --git a/cmd/tasks_action_test.go b/cmd/tasks_action_test.go index b73ebb0..c57ea0e 100644 --- a/cmd/tasks_action_test.go +++ b/cmd/tasks_action_test.go @@ -11,6 +11,9 @@ import ( "bytes" "context" + "errors" + + "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" "github.com/spf13/viper" ) @@ -51,10 +54,9 @@ func newTaskActionRecorder(t *testing.T, state, currentStepID string) *taskActio return r } -// taskActionExpectations pins, per action command, the path it must POST and -// whether its body carries policyStepId. Seeded against tasksCmd.Commands() -// by TestEveryTaskActionIsPinned, so adding a command without adding a row -// here fails rather than going silently untested. +// taskActionExpectations pins each action's path and step mode. +// TestEveryTaskActionIsPinned requires a row per command, so a new command +// cannot go untested. var taskActionExpectations = map[string]struct { verb string step policyStepMode @@ -76,8 +78,7 @@ var taskActionExpectations = map[string]struct { // nonActionTaskSubcommands are the tasks subcommands that are not action POSTs. var nonActionTaskSubcommands = map[string]bool{"list": true} -// TestEveryTaskActionIsPinned is the guard on the guard: every action command -// in the tree must have a row above. +// TestEveryTaskActionIsPinned requires a row for every action in the tree. func TestEveryTaskActionIsPinned(t *testing.T) { seen := 0 for _, c := range tasksCmd.Commands() { @@ -100,9 +101,8 @@ func TestEveryTaskActionIsPinned(t *testing.T) { } } -// TestEveryTaskActionPostsItsOwnVerbAndStep drives every pinned command and -// checks both the path and whether policyStepId is on the wire. A copied verb -// would perform a different action on the task while printing success. +// TestEveryTaskActionPostsItsOwnVerbAndStep checks path and policyStepId on +// the wire. A copied verb performs a different action while printing success. func TestEveryTaskActionPostsItsOwnVerbAndStep(t *testing.T) { const step = "zz-step-1111111111111111111" for name, want := range taskActionExpectations { @@ -136,9 +136,8 @@ func TestEveryTaskActionPostsItsOwnVerbAndStep(t *testing.T) { } } -// TestTasksCommentAlwaysSendsTheCommentKey pins the one action whose empty -// value must still reach the wire: the comment IS the payload, so an omitted -// key records nothing while the command still prints success. +// TestTasksCommentAlwaysSendsTheCommentKey: an omitted key records nothing +// while the command still prints success. func TestTasksCommentAlwaysSendsTheCommentKey(t *testing.T) { cmd := findTasksSubcommand(t, "comment") resetCmds(t, cmd) @@ -153,9 +152,8 @@ func TestTasksCommentAlwaysSendsTheCommentKey(t *testing.T) { } } -// TestTasksDenyOmitsAnUnresolvableStep pins deny's stepOptional mode on the -// wire: when the current step cannot be derived the field must be absent, not -// empty, and the denial must still go through. +// TestTasksDenyOmitsAnUnresolvableStep: the field must be absent, not empty, +// and the denial must still go through. func TestTasksDenyOmitsAnUnresolvableStep(t *testing.T) { cmd := findTasksSubcommand(t, "deny") resetCmds(t, cmd) @@ -259,10 +257,9 @@ func findTasksSubcommand(t *testing.T, name string) *cobra.Command { return nil } -// TestEveryTaskActionModeBehavesOnAnUnresolvableStep is what separates -// stepRequired from stepOptional, which a "does it send the field" check -// cannot see: with no derivable step, required must error before sending and -// optional must send without the field. +// TestEveryTaskActionModeBehavesOnAnUnresolvableStep separates stepRequired +// from stepOptional: with no derivable step, required errors before sending, +// optional sends without the field. func TestEveryTaskActionModeBehavesOnAnUnresolvableStep(t *testing.T) { for name, want := range taskActionExpectations { if want.step == stepUnused { @@ -299,9 +296,7 @@ func TestEveryTaskActionModeBehavesOnAnUnresolvableStep(t *testing.T) { } } -// TestTaskActionsDryRunNeverSends is the guard on this branch's own regression: -// --dry-run previewed before the URL was resolved, so a typo'd tenant previewed -// happily. It must also never reach the wire, for every action. +// TestTaskActionsDryRunNeverSends: --dry-run must never reach the wire. func TestTaskActionsDryRunNeverSends(t *testing.T) { for name, want := range taskActionExpectations { t.Run(name, func(t *testing.T) { @@ -325,11 +320,13 @@ func TestTaskActionsDryRunNeverSends(t *testing.T) { stubNewClient(t, srv) t.Setenv("C1I_URL", "https://example.invalid") - viper.Set("dry_run", true) - t.Cleanup(func() { viper.Set("dry_run", false) }) + withDryRun(t) var out bytes.Buffer cmd.SetOut(&out) + // Package-level singleton: a left-attached buffer swallows this + // command's output in every later test. + t.Cleanup(func() { cmd.SetOut(nil) }) cmd.SetContext(context.Background()) if err := cmd.RunE(cmd, []string{actionTestTaskID}); err != nil { t.Fatalf("%s --dry-run: %v", name, err) @@ -347,9 +344,8 @@ func TestTaskActionsDryRunNeverSends(t *testing.T) { } } -// TestTasksUpdateGrantDurationSendsDurationKey pins the one payload key this -// command exists to send. "grantDuration" is the plausible wrong name — that -// is what the RESPONSE carries, and what the docs quote. +// TestTasksUpdateGrantDurationSendsDurationKey. "grantDuration" is the +// plausible wrong name: it is what the response carries. func TestTasksUpdateGrantDurationSendsDurationKey(t *testing.T) { cmd := findTasksSubcommand(t, "update-grant-duration") resetCmds(t, cmd) @@ -366,19 +362,16 @@ func TestTasksUpdateGrantDurationSendsDurationKey(t *testing.T) { } } -// TestTaskActionsDryRunStillResolvesTheURL is the guard on the ordering this -// branch had to fix twice. --dry-run answers "am I about to do this to the -// right tenant", so it must still reject a bad --url. Previewing before -// resolving the URL made a typo'd tenant preview happily at exit 0, and no -// other test could see it: the rest stub the client and pass a valid URL. +// TestTaskActionsDryRunStillResolvesTheURL: a bad --url must fail even under +// --dry-run. The other dry-run tests pass a valid URL, so only this sees it. func TestTaskActionsDryRunStillResolvesTheURL(t *testing.T) { - // Both modes, since the runner resolves the URL once for each path. + // Both modes: the runner resolves the URL once, before either path. for _, name := range []string{"close", "restart"} { t.Run(name, func(t *testing.T) { resetRootURLFlag(t) + resetRootDryRunFlag(t) t.Setenv("C1I_URL", "") - viper.Set("dry_run", true) - t.Cleanup(func() { viper.Set("dry_run", false) }) + withDryRun(t) var out bytes.Buffer rootCmd.SetOut(&out) @@ -397,3 +390,76 @@ func TestTaskActionsDryRunStillResolvesTheURL(t *testing.T) { }) } } + +// withDryRun turns dry-run on and restores the previous value, matching +// withRealDryRun. Hardcoding false would outrank a leaked pflag. +func withDryRun(t *testing.T) { + t.Helper() + orig := viper.GetBool("dry_run") + viper.Set("dry_run", true) + t.Cleanup(func() { viper.Set("dry_run", orig) }) +} + +// resetRootDryRunFlag clears the persistent flag and its Changed bit, so a +// test passing it through rootCmd cannot leak it. +func resetRootDryRunFlag(t *testing.T) { + t.Helper() + f := rootCmd.PersistentFlags().Lookup("dry-run") + if f == nil { + t.Fatal("rootCmd has no --dry-run flag; this reset is not doing what it thinks") + } + orig, changed := f.Value.String(), f.Changed + t.Cleanup(func() { + _ = f.Value.Set(orig) + f.Changed = changed + }) + _ = f.Value.Set("false") + f.Changed = false +} + +// TestStepUnusedActionsPreviewWithoutCredentials: an action needing no step +// previews without authenticating; one needing a step must authenticate. +func TestStepUnusedActionsPreviewWithoutCredentials(t *testing.T) { + for name, want := range taskActionExpectations { + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if want.setup != nil { + want.setup(cmd) + } + // A client that always fails, standing in for absent credentials. + orig := newClient + newClient = func(_ *cobra.Command, _ string) (*client.Client, error) { + return nil, errNoCredentialsForTest + } + t.Cleanup(func() { newClient = orig }) + + t.Setenv("C1I_URL", "https://example.invalid") + withDryRun(t) + + var out bytes.Buffer + cmd.SetOut(&out) + t.Cleanup(func() { cmd.SetOut(nil) }) + cmd.SetContext(context.Background()) + err := cmd.RunE(cmd, []string{actionTestTaskID}) + + if want.step == stepUnused { + if err != nil { + t.Fatalf("%s --dry-run needs credentials it should not: %v", name, err) + } + if !strings.Contains(out.String(), "[dry-run]") { + t.Errorf("%s printed no preview: %q", name, out.String()) + } + return + } + // Step-using actions must fetch the step, so they authenticate + // first even for a preview — as they did before sharing a runner. + if err == nil { + t.Fatalf("%s --dry-run should have failed without credentials; it resolves a policy step", name) + } + }) + } +} + +// errNoCredentialsForTest stands in for a credential-loading failure. +var errNoCredentialsForTest = errors.New("authentication failed: no credentials found") diff --git a/cmd/tasks_comment.go b/cmd/tasks_comment.go index 631f9f2..2906dc8 100644 --- a/cmd/tasks_comment.go +++ b/cmd/tasks_comment.go @@ -9,9 +9,8 @@ import ( var tasksCommentAction = taskAction{ verb: "comment", step: stepUnused, - // Sent unconditionally, unlike every other action: the comment IS the - // payload here, so an explicit --comment "" must reach the server rather - // than being omitted as an absent optional field. + // Sent unconditionally: the comment is the payload, so --comment "" must + // reach the server rather than be omitted as an absent option. extraBody: func(cmd *cobra.Command, body map[string]any) error { comment, _ := cmd.Flags().GetString("comment") body["comment"] = comment diff --git a/cmd/tasks_restart.go b/cmd/tasks_restart.go index 70e9b45..a65ce60 100644 --- a/cmd/tasks_restart.go +++ b/cmd/tasks_restart.go @@ -10,7 +10,7 @@ var tasksRestartAction = taskAction{ verb: "restart", step: stepOptional, confirm: func(id, _, stepID string) string { - // A closed task has no current step, so the field would print empty. + // A closed task has no current step. if stepID == "" { return fmt.Sprintf("Restarted task: task_id=%s\n", id) } @@ -35,8 +35,10 @@ with "action not permitted" otherwise. Check the task's own action list: restart, reset and skip-step each rotate the current policy step, so a --policy-step-id captured before one of them goes stale and the server answers: this action is no longer available: the request has advanced to a new approval step -Omit the flag to act on whatever step is current. (reset takes no step -argument; it restarts the whole policy.) +Omit the flag to act on whatever step is current. + +Because the step is fetched, --dry-run authenticates and issues a read against +the tenant before printing its preview. The confirmation reports the task id and the step acted on, never a state: the action endpoints echo the task's state from before the action.`, diff --git a/cmd/tasks_skip_step.go b/cmd/tasks_skip_step.go index f2c39da..72805d6 100644 --- a/cmd/tasks_skip_step.go +++ b/cmd/tasks_skip_step.go @@ -22,7 +22,8 @@ var tasksSkipStepCmd = &cobra.Command{ The step id is required by the server, which rejects a missing one with: invalid TaskActionsServiceSkipStepRequest.PolicyStepId: value does not match regex pattern "^[a-zA-Z0-9]{27}$" It defaults to the task's currently executing step, so pass --policy-step-id -only to target a different one. A step id captured before another action is +only to target a different one. Because that step is fetched, --dry-run +authenticates and issues a read against the tenant before previewing. A step id captured before another action is stale, and the server answers "this action is no longer available". The confirmation reports the task id and the step skipped, never a state: the From fa34bb6ac30822a407ad18f03e802000942e7d7f Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 07:16:28 +0000 Subject: [PATCH 7/7] Rewrap an overlong help line Co-Authored-By: Claude Opus 5 --- cmd/tasks_skip_step.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/tasks_skip_step.go b/cmd/tasks_skip_step.go index 72805d6..a02f029 100644 --- a/cmd/tasks_skip_step.go +++ b/cmd/tasks_skip_step.go @@ -23,8 +23,9 @@ The step id is required by the server, which rejects a missing one with: invalid TaskActionsServiceSkipStepRequest.PolicyStepId: value does not match regex pattern "^[a-zA-Z0-9]{27}$" It defaults to the task's currently executing step, so pass --policy-step-id only to target a different one. Because that step is fetched, --dry-run -authenticates and issues a read against the tenant before previewing. A step id captured before another action is -stale, and the server answers "this action is no longer available". +authenticates and issues a read against the tenant before previewing. A step +id captured before another action is stale, and the server answers +"this action is no longer available". The confirmation reports the task id and the step skipped, never a state: the action endpoints echo the task's state from before the action.`,