fix: address verified review findings (terminal-output-1)

Signed-off-by: bircni <bircni@icloud.com>

internal/terminal/format.go

@@ -45,7 +45,7 @@ func StatusIcon(status, conclusion string) Cell {
 	switch status {
 	case "success":
 		return Cell{Text: "✓", Color: "green"}
-	case "failure", "error":
+	case "failure", "error", "warning":
 		return Cell{Text: "X", Color: "red"}
 	case "cancelled", "canceled", "skipped": //nolint:misspell // Gitea API spelling.
 		return Cell{Text: "-", Muted: true}

internal/terminal/format_test.go

@@ -57,6 +57,7 @@ func TestStatusIcon(t *testing.T) {
 		{"success", "", Cell{Text: "✓", Color: "green"}},
 		{"completed", "failure", Cell{Text: "X", Color: "red"}},
 		{"error", "", Cell{Text: "X", Color: "red"}},
+		{"warning", "", Cell{Text: "X", Color: "red"}},
 		{"completed", "cancelled", Cell{Text: "-", Muted: true}}, //nolint:misspell // Gitea API spelling.
 		{"skipped", "", Cell{Text: "-", Muted: true}},
 		{"queued", "", Cell{Text: "*", Color: "yellow"}},

internal/terminal/output.go

@@ -19,10 +19,16 @@ import (
 type JSONOutput struct {
 	// Fields holds the comma-separated fields to keep; "" and "*" keep every field.
 	Fields string
+	// Allowed lists the fields the command documents. A listed field that an item omits (an empty
+	// omitempty value) is selected as null instead of being reported unknown; nil allows none.
+	Allowed []string
 	// Query is a jq expression applied after field selection.
 	Query string
 	// Pretty indents the JSON for a terminal and strips control characters from raw jq strings.
 	Pretty bool
+	// Safe strips control characters from raw jq strings and encoded text like Pretty, for terminal
+	// output that must stay one line per value.
+	Safe bool
 	// Color highlights pretty JSON keys and values.
 	Color bool
 }
@@ -51,6 +57,9 @@ func (o JSONOutput) Write(w io.Writer, value any) error {
 			selected := map[string]any{}
 			for _, name := range names {
 				v, exists := object[name]
+				if !exists && slices.Contains(o.Allowed, name) {
+					v, exists = nil, true
+				}
 				if !exists {
 					return nil, fmt.Errorf("unknown JSON field %q", name)
 				}
@@ -90,7 +99,7 @@ func (o JSONOutput) Write(w io.Writer, value any) error {
 			return fmt.Errorf("evaluate --jq: %w", err)
 		}
 		if str, ok := value.(string); ok {
-			if o.Pretty {
+			if o.Pretty || o.Safe {
 				str = SafeText(str)
 			}
 			if _, err := fmt.Fprintln(w, str); err != nil {
@@ -114,7 +123,7 @@ func (o JSONOutput) encode(w io.Writer, value any) error {
 		return err
 	}
 	text := buf.String()
-	if o.Pretty {
+	if o.Pretty || o.Safe {
 		// The encoder escapes control characters but not format characters such as bidirectional overrides.
 		text = SafeText(text)
 	}

internal/terminal/output_test.go

@@ -24,6 +24,27 @@ func TestJSONFieldSelectionAndJQ(t *testing.T) {
 	require.EqualError(t, ValidateOutput("number,,title", "", []string{"number", "title"}), "--json field names cannot be empty; use --json alone for every field")
 }
 
+func TestJSONFieldSelectionKeepsDocumentedFieldsOmittedByOmitempty(t *testing.T) {
+	type item struct {
+		Name   string  `json:"name"`
+		Parent *string `json:"parent,omitempty"`
+	}
+	var out bytes.Buffer
+	require.NoError(t, JSONOutput{Fields: "name,parent", Allowed: []string{"name", "parent"}}.Write(&out, []item{{Name: "x"}}))
+	require.Equal(t, "[{\"name\":\"x\",\"parent\":null}]\n", out.String())
+	require.EqualError(t, JSONOutput{Fields: "parent", Allowed: []string{"name"}}.Write(&out, []item{{Name: "x"}}), `unknown JSON field "parent"`)
+}
+
+func TestJSONOutputSafeStripsControlsAndStaysOnOneLine(t *testing.T) {
+	value := map[string]any{"title": "a\x1b]0;x\x07b\u202e"}
+	var out bytes.Buffer
+	require.NoError(t, JSONOutput{Query: ".title", Safe: true}.Write(&out, value))
+	require.Equal(t, "a]0;xb\n", out.String())
+	out.Reset()
+	require.NoError(t, JSONOutput{Safe: true}.Write(&out, value))
+	require.Equal(t, "{\"title\":\"a\\u001b]0;x\\u0007b\"}\n", out.String())
+}
+
 func TestJSONOutputIsIndentedAndColoredForTerminals(t *testing.T) {
 	var out bytes.Buffer
 	value := []map[string]any{{"title": "a \u202e\x1b[31mb", "done": true, "due": nil, "number": 1}}

internal/terminal/safe_writer.go

@@ -10,7 +10,7 @@ import (
 )
 
 // maxSGRLength bounds how many bytes of a possible SGR sequence are held back between writes.
-const maxSGRLength = 32
+const maxSGRLength = 256
 
 // SafeWriter returns w unchanged when tty is false, so piped output stays byte-exact.
 // On a terminal it filters streamed server text like SafeText, and with allowSGR it keeps

internal/terminal/safe_writer_test.go

@@ -5,13 +5,30 @@ package terminal
 
 import (
 	"bytes"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 )
 
+func TestSafeWriterKeepsLongSGRSequences(t *testing.T) {
+	const colored = "\x1b[38;2;255;255;255;48;2;255;255;255mwhite\x1b[0m\n"
+	for _, chunk := range []int{len(colored), 1, 3} {
+		var out bytes.Buffer
+		w := SafeWriter(&out, true, true)
+		for data := []byte(colored); len(data) > 0; {
+			n := min(chunk, len(data))
+			_, err := w.Write(data[:n])
+			require.NoError(t, err)
+			data = data[n:]
+		}
+		require.Equal(t, colored, out.String(), "chunk size %d", chunk)
+	}
+}
+
 func TestSafeWriter(t *testing.T) {
-	const hostile = "+hello \x1b]0;PWNED\x07 \x1b[1;31mred\x1b[0m\x1b[2J\r\n\u009b2J\u202eend\t\xff\x1b[1111111111111111111111111111111111m\n"
+	long := strings.Repeat("1", 300)
+	hostile := "+hello \x1b]0;PWNED\x07 \x1b[1;31mred\x1b[0m\x1b[2J\r\n\u009b2J\u202eend\t\xff\x1b[" + long + "m\n"
 	var piped bytes.Buffer
 	_, err := SafeWriter(&piped, false, false).Write([]byte(hostile))
 	require.NoError(t, err)
@@ -21,8 +38,8 @@ func TestSafeWriter(t *testing.T) {
 		allowSGR bool
 		want     string
 	}{
-		{"drops every escape", false, "+hello ]0;PWNED [1;31mred[0m[2J\n2Jend\t\uFFFD[1111111111111111111111111111111111m\n"},
-		{"keeps colors", true, "+hello ]0;PWNED \x1b[1;31mred\x1b[0m[2J\n2Jend\t\uFFFD[1111111111111111111111111111111111m\n"},
+		{"drops every escape", false, "+hello ]0;PWNED [1;31mred[0m[2J\n2Jend\t\uFFFD[" + long + "m\n"},
+		{"keeps colors", true, "+hello ]0;PWNED \x1b[1;31mred\x1b[0m[2J\n2Jend\t\uFFFD[" + long + "m\n"},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			if !tc.allowSGR {

internal/terminal/streams.go

@@ -188,15 +188,32 @@ type Confirmation struct {
 	NonTTY string
 }
 
+// questionEnd returns the index of the first question mark outside double quotes, or -1, so a quoted
+// name such as "why? not" does not end the question.
+func questionEnd(prompt string) int {
+	quoted := false
+	for i, r := range prompt {
+		switch {
+		case r == '"':
+			quoted = !quoted
+		case r == '?' && !quoted:
+			return i
+		}
+	}
+	return -1
+}
+
 // ConfirmAction asks c unless yes is set. A declined or aborted confirmation returns ErrCanceled.
 func ConfirmAction(ctx context.Context, c Confirmation, yes bool, streams Streams, policy Policy, prompt Prompter) error {
 	if yes {
 		return nil
 	}
+	// Names in the question come from the server, so keep control characters off the terminal.
+	c.Prompt, c.NonTTY = SafeLine(c.Prompt), SafeLine(c.NonTTY)
 	if !policy.CanPrompt(streams) {
 		action := c.NonTTY
-		if question, _, ok := strings.Cut(c.Prompt, "?"); action == "" && ok && !c.DefaultYes && question != "" {
-			action = strings.ToLower(question[:1]) + question[1:]
+		if question := questionEnd(c.Prompt); action == "" && question > 0 && !c.DefaultYes {
+			action = strings.ToLower(c.Prompt[:1]) + c.Prompt[1:question]
 		}
 		if action != "" {
 			action = " to " + action

internal/terminal/streams_test.go

@@ -99,6 +99,8 @@ func TestConfirmAction(t *testing.T) {
 		{name: "accepted by flag", c: deleteLabel, yes: true, prompt: &askingPrompt{}},
 		{name: "names the action without a terminal", c: deleteLabel, prompt: &askingPrompt{}, code: clierr.Usage, message: `confirmation required to delete label "bug" from alice/x; rerun with --yes`},
 		{name: "names the prompted action without a terminal", c: Confirmation{Prompt: "Delete alice/x? It becomes read-only."}, prompt: &askingPrompt{}, code: clierr.Usage, message: "confirmation required to delete alice/x; rerun with --yes"},
+		{name: "keeps a quoted question mark", c: Confirmation{Prompt: `Delete label "why? not" from alice/x?`}, prompt: &askingPrompt{}, code: clierr.Usage, message: `confirmation required to delete label "why? not" from alice/x; rerun with --yes`},
+		{name: "drops escape sequences from server names", c: Confirmation{Prompt: "Delete attachment \"x\x1b]0;PWNED\x07\"?"}, prompt: &askingPrompt{}, code: clierr.Usage, message: `confirmation required to delete attachment "x]0;PWNED"; rerun with --yes`},
 		{name: "without a terminal or action", c: Confirmation{Prompt: "Go?", DefaultYes: true}, prompt: &askingPrompt{}, code: clierr.Usage, message: "confirmation required; rerun with --yes"},
 		{name: "accepted", c: deleteLabel, streams: terminal, prompt: &askingPrompt{accept: true}, defaults: []bool{false}},
 		{name: "passes the default", c: Confirmation{Prompt: "Create pull request?", DefaultYes: true}, streams: terminal, prompt: &askingPrompt{accept: true}, defaults: []bool{true}},

internal/terminal/view.go

@@ -46,6 +46,8 @@ type View struct {
 	Meta   string
 	Fields []Field
 	Body   string
+	// Plain marks the body as plain text, such as a commit message, which is never rendered as markdown.
+	Plain bool
 	// Bodyless resources, such as webhooks, have no body: the view ends with the fields and URL.
 	Bodyless bool
 	// Kind names the resource in the footer, such as "issue".
@@ -113,7 +115,7 @@ func (v View) Render(d Display) string {
 		switch {
 		case body == "":
 			body = muted("No description provided")
-		case d.Markdown:
+		case d.Markdown && !v.Plain:
 			body = Markdown(body, d)
 		default:
 			body = SafeText(body)

internal/terminal/view_test.go

@@ -96,6 +96,11 @@ func TestViewRendersMarkdownOnlyWhenEnabled(t *testing.T) {
 	require.Contains(t, view.Render(Display{TTY: true, Markdown: true, Color: true, Links: true}), "\x1b]8;id=")
 }
 
+func TestViewKeepsPlainBodiesVerbatim(t *testing.T) {
+	view := View{Title: "main", Body: "fix <parser> __init__\n# not a heading\x1b[2J", Plain: true}
+	require.Contains(t, view.Render(Display{TTY: true, Markdown: true}), "\nfix <parser> __init__\n# not a heading[2J\n")
+}
+
 func TestMarkdownSanitizesServerText(t *testing.T) {
 	rendered := Markdown("Hello \x1b]0;PWNED\x07world\n\n`\x1b[2Jcode`", Display{Color: true, Links: true})
 	require.NotContains(t, rendered, "\x1b]0;")