From 94e30f8f429d65005e65c6999d288d6d3fa95a2a Mon Sep 17 00:00:00 2001 From: Thibault Le Ouay Ducasse Date: Fri, 16 Oct 2026 16:18:45 +0200 Subject: [PATCH] fix(incident): address review feedback - approve: only report a missing postmortem when the server says so - permission denied: keep the server reason and append the role hint - declare: validate passed flags before opening the wizard - list: ignore ANSI codes when measuring table column widths - read from stdin: tell TTY users to press Ctrl-D - postmortem edit --file: trim one trailing newline like stdin does - status-report create: confirm the incident link from the response - ci: use go.mod for the test workflow Go version Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test.yml | 2 +- internal/incident/errors.go | 25 +++++++++++---- internal/incident/export_test.go | 1 + internal/incident/incident.go | 19 ++++++++++- internal/incident/incident_declare.go | 32 +++++++++++++++++++ internal/incident/incident_declare_test.go | 18 +++++++++++ internal/incident/incident_list.go | 2 +- internal/incident/incident_postmortem.go | 2 +- internal/incident/incident_postmortem_test.go | 18 ++++++++++- internal/statusreport/statusreport_create.go | 24 +++++++++++--- 10 files changed, 127 insertions(+), 16 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b164f1c..abcc711 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -19,6 +19,6 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v5 with: - go-version: '>=1.22.0' + go-version-file: go.mod - name: Run test run: go test -timeout 30s -race -count=1 ./... diff --git a/internal/incident/errors.go b/internal/incident/errors.go index e21d05a..52cd82f 100644 --- a/internal/incident/errors.go +++ b/internal/incident/errors.go @@ -41,18 +41,20 @@ func formatIncidentError(err error, id string, o op) error { case connect.CodeNotFound: switch o { case opApprove: - return fmt.Errorf("no postmortem written yet for incident %s. Write one first: openstatus inc postmortem edit %s --file postmortem.md", id, id) + if strings.Contains(strings.ToLower(msg), "postmortem") { + return fmt.Errorf("no postmortem written yet for incident %s. Write one first: openstatus inc postmortem edit %s --file postmortem.md", id, id) + } case opLink, opDeclare: if msg != "" { return errors.New(msg) } } case connect.CodePermissionDenied: - switch o { - case opClose, opApprove: - return errors.New("permission denied: requires a workspace owner or admin, or the incident commander. API keys without a creator cannot perform this action") - case opDelete: - return errors.New("permission denied: requires a workspace owner or admin. API keys without a creator cannot perform this action") + if hint := permissionHint(o); hint != "" { + if msg == "" { + return fmt.Errorf("permission denied. %s", hint) + } + return fmt.Errorf("permission denied: %s. %s", msg, hint) } } @@ -71,3 +73,14 @@ func preconditionHint(id string, o op) string { return "" } } + +func permissionHint(o op) string { + switch o { + case opClose, opApprove: + return "This action requires a workspace owner or admin, or the incident commander. API keys without a creator cannot perform it." + case opDelete: + return "This action requires a workspace owner or admin. API keys without a creator cannot perform it." + default: + return "" + } +} diff --git a/internal/incident/export_test.go b/internal/incident/export_test.go index cd45237..ca2882e 100644 --- a/internal/incident/export_test.go +++ b/internal/incident/export_test.go @@ -17,6 +17,7 @@ var ( BuildListRequest = buildListRequest BuildDeclareRequest = buildDeclareRequest MissingDeclareFlags = missingDeclareFlags + ValidateDeclareFlags = validateProvidedDeclareFlags BuildUpdateRequest = buildUpdateRequest BuildSetStatusRequest = buildSetStatusRequest BuildNoteRequest = buildNoteRequest diff --git a/internal/incident/incident.go b/internal/incident/incident.go index 51d2b78..13c15cd 100644 --- a/internal/incident/incident.go +++ b/internal/incident/incident.go @@ -8,13 +8,16 @@ import ( "net/http" "net/mail" "os" + "regexp" "strings" "time" + "unicode/utf8" "buf.build/gen/go/openstatus/api/connectrpc/gosimple/openstatus/incident/v1/incidentv1connect" incidentv1 "buf.build/gen/go/openstatus/api/protocolbuffers/go/openstatus/incident/v1" "connectrpc.com/connect" "github.com/fatih/color" + "github.com/mattn/go-isatty" "github.com/olekukonko/tablewriter" "github.com/olekukonko/tablewriter/renderer" "github.com/olekukonko/tablewriter/tw" @@ -360,17 +363,31 @@ func readTextArg(value string, stdin io.Reader) (string, error) { if value != "-" { return value, nil } + if f, ok := stdin.(*os.File); ok && (isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd())) { + fmt.Fprintln(os.Stderr, "Reading from stdin, press Ctrl-D to finish.") + } data, err := io.ReadAll(stdin) if err != nil { return "", fmt.Errorf("failed to read stdin: %w", err) } - text := strings.TrimSuffix(strings.TrimSuffix(string(data), "\n"), "\r") + text := trimTrailingNewline(string(data)) if strings.TrimSpace(text) == "" { return "", errors.New("no input received on stdin") } return text, nil } +func trimTrailingNewline(s string) string { + return strings.TrimSuffix(strings.TrimSuffix(s, "\n"), "\r") +} + +var ansiEscape = regexp.MustCompile(`\x1b\[[0-9;]*m`) + +// visibleWidth ignores ANSI color codes so colored table cells stay aligned. +func visibleWidth(s string) int { + return utf8.RuneCountInString(ansiEscape.ReplaceAllString(s, "")) +} + func nextStepHint(inc *incidentv1.Incident) string { if inc.GetClosedAt() != "" { return "" diff --git a/internal/incident/incident_declare.go b/internal/incident/incident_declare.go index 7f6e1e2..2cf0c50 100644 --- a/internal/incident/incident_declare.go +++ b/internal/incident/incident_declare.go @@ -58,6 +58,35 @@ func missingDeclareFlags(p DeclareParams) []string { return missing } +// validateProvidedDeclareFlags checks the flags that were passed so the wizard +// does not prompt for the rest before rejecting a bad value. +func validateProvidedDeclareFlags(p DeclareParams) error { + if p.Title != "" { + if err := validateTitle(p.Title); err != nil { + return err + } + } + if p.Severity != "" { + if _, err := severityToSDK(p.Severity); err != nil { + return err + } + } + if err := validateSummary(p.Summary); err != nil { + return err + } + if p.Commander != "" { + if err := validateEmail("--commander", p.Commander); err != nil { + return err + } + } + if p.StartedAt != "" { + if err := validateTimestamp("--started-at", p.StartedAt); err != nil { + return err + } + } + return nil +} + func buildDeclareRequest(p DeclareParams) (*incidentv1.DeclareIncidentRequest, error) { if err := validateTitle(p.Title); err != nil { return nil, err @@ -180,6 +209,9 @@ In scripts, pass every required flag.`, if output.IsJSONOutput() || !output.IsStdinTerminal() { return fmt.Errorf("missing required flags: %s", strings.Join(missing, ", ")) } + if err := validateProvidedDeclareFlags(p); err != nil { + return err + } var err error p, err = runDeclareWizard(p) if err != nil { diff --git a/internal/incident/incident_declare_test.go b/internal/incident/incident_declare_test.go index 8b5249c..d128ed9 100644 --- a/internal/incident/incident_declare_test.go +++ b/internal/incident/incident_declare_test.go @@ -86,6 +86,24 @@ func Test_MissingDeclareFlags(t *testing.T) { } } +func Test_ValidateDeclareFlags(t *testing.T) { + t.Parallel() + + if err := incident.ValidateDeclareFlags(incident.DeclareParams{}); err != nil { + t.Errorf("empty params should be valid before the wizard, got %v", err) + } + for _, p := range []incident.DeclareParams{ + {Severity: "foo"}, + {Commander: "not-an-email"}, + {StartedAt: "yesterday"}, + {Title: " "}, + } { + if err := incident.ValidateDeclareFlags(p); err == nil { + t.Errorf("expected error for %+v", p) + } + } +} + func Test_DeclareIncident(t *testing.T) { t.Run("sends request and prints hint", func(t *testing.T) { srv := newServer(http.StatusOK, incidentResponse()) diff --git a/internal/incident/incident_list.go b/internal/incident/incident_list.go index dd3be15..4d11e50 100644 --- a/internal/incident/incident_list.go +++ b/internal/incident/incident_list.go @@ -105,7 +105,7 @@ func ListIncidents(ctx context.Context, client incidentv1connect.IncidentService columnFmt := color.New(color.FgYellow).SprintfFunc() tbl := table.New("ID", "Title", "Severity", "Status", "Commander", "Declared", "Report") - tbl.WithHeaderFormatter(headerFmt).WithFirstColumnFormatter(columnFmt) + tbl.WithHeaderFormatter(headerFmt).WithFirstColumnFormatter(columnFmt).WithWidthFunc(visibleWidth) for _, inc := range incidents { report := "—" diff --git a/internal/incident/incident_postmortem.go b/internal/incident/incident_postmortem.go index cf4737a..5cec249 100644 --- a/internal/incident/incident_postmortem.go +++ b/internal/incident/incident_postmortem.go @@ -147,7 +147,7 @@ func resolvePostmortemContent(file, content string, fileSet, contentSet bool, st if err != nil { return "", fmt.Errorf("read postmortem file %q: %w", file, err) } - text = string(data) + text = trimTrailingNewline(string(data)) } } diff --git a/internal/incident/incident_postmortem_test.go b/internal/incident/incident_postmortem_test.go index ec31034..edc1df5 100644 --- a/internal/incident/incident_postmortem_test.go +++ b/internal/incident/incident_postmortem_test.go @@ -130,7 +130,7 @@ func Test_ResolvePostmortemContent(t *testing.T) { want string wantErr string }{ - {name: "file", file: path, fileSet: true, want: "# From file\n"}, + {name: "file", file: path, fileSet: true, want: "# From file"}, {name: "stdin", file: "-", fileSet: true, stdin: "# From stdin\n", want: "# From stdin"}, {name: "content", content: "# Inline", contentSet: true, want: "# Inline"}, {name: "neither", wantErr: "exactly one of --file or --content"}, @@ -243,6 +243,22 @@ func Test_ApprovePostmortem(t *testing.T) { t.Errorf("unexpected error %v", err) } }) + + t.Run("missing incident is not reported as missing postmortem", func(t *testing.T) { + srv := errorServer(http.StatusNotFound, "not_found", "incident not found") + err := incident.ApprovePostmortemWithHTTPClient(context.Background(), srv.client(), "token", "42", false) + if err == nil || !strings.Contains(err.Error(), "incident 42 not found") { + t.Errorf("unexpected error %v", err) + } + }) + + t.Run("permission denied keeps the server reason", func(t *testing.T) { + srv := errorServer(http.StatusForbidden, "permission_denied", "api key lacks the incidents scope") + err := incident.ApprovePostmortemWithHTTPClient(context.Background(), srv.client(), "token", "42", false) + if err == nil || !strings.Contains(err.Error(), "api key lacks the incidents scope") || !strings.Contains(err.Error(), "or the incident commander") { + t.Errorf("unexpected error %v", err) + } + }) } func Test_WritePostmortemHeader(t *testing.T) { diff --git a/internal/statusreport/statusreport_create.go b/internal/statusreport/statusreport_create.go index 7a18fa2..0a09273 100644 --- a/internal/statusreport/statusreport_create.go +++ b/internal/statusreport/statusreport_create.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "os" "strings" "time" @@ -28,10 +29,18 @@ type CreateStatusReportParams struct { } func CreateStatusReport(ctx context.Context, client status_reportv1connect.StatusReportServiceClient, p CreateStatusReportParams) (string, error) { - sdkStatus, err := statusToSDK(p.Status) + report, err := createStatusReport(ctx, client, p) if err != nil { return "", err } + return report.GetId(), nil +} + +func createStatusReport(ctx context.Context, client status_reportv1connect.StatusReportServiceClient, p CreateStatusReportParams) (*status_reportv1.StatusReport, error) { + sdkStatus, err := statusToSDK(p.Status) + if err != nil { + return nil, err + } req := &status_reportv1.CreateStatusReportRequest{ Title: p.Title, @@ -59,10 +68,10 @@ func CreateStatusReport(ctx context.Context, client status_reportv1connect.Statu resp, err := client.CreateStatusReport(ctx, req) if err != nil { - return "", output.FormatError(err, "status-report", "") + return nil, output.FormatError(err, "status-report", "") } - return resp.GetStatusReport().GetId(), nil + return resp.GetStatusReport(), nil } func CreateStatusReportWithHTTPClient(ctx context.Context, httpClient *http.Client, apiKey string, p CreateStatusReportParams) (string, error) { @@ -188,7 +197,7 @@ func GetStatusReportCreateCmd() *cli.Command { client := NewStatusReportClient(apiKey) s := output.StartSpinner("Creating status report...") - id, err := CreateStatusReport(ctx, client, CreateStatusReportParams{ + report, err := createStatusReport(ctx, client, CreateStatusReportParams{ Title: inputs.Title, Status: inputs.Status, Message: inputs.Message, @@ -204,9 +213,14 @@ func GetStatusReportCreateCmd() *cli.Command { return cli.Exit(err.Error(), 1) } + id := report.GetId() fmt.Printf("Status report created successfully (ID: %s)\n", id) if incidentID := cmd.String("incident-id"); incidentID != "" { - fmt.Printf("Linked to incident %s\n", incidentID) + if report.HasIncidentId() { + fmt.Printf("Linked to incident %s\n", report.GetIncidentId()) + } else { + fmt.Fprintf(os.Stderr, "Warning: the report was not linked to incident %s. Link it with: openstatus inc link-report %s %s\n", incidentID, incidentID, id) + } } fmt.Printf("To add updates, run: openstatus status-report add-update %s --status identified --message '...'\n", id) fmt.Println("Tip: pass --impact = to update per-component impact.") -- knotmirror 0.0.1