Merge pull request #141 from flashcatcloud/fix/detail-projection-never-truncates-values

fix(cli): never truncate incident detail --fields values

Author: ysyneu

internal/cli/fieldproject.go

@@ -81,23 +81,75 @@ func noteDefaultProjection(w io.Writer, fields []string) {
 }
 
 // boundProjectedOutput keeps the new agent-oriented projections below their
-// command budget without changing the selected keys. Short values remain byte
-// identical; when retained strings alone would overflow the actual JSON/TOON
-// encoding, they are shortened fairly and marked with "...". If keys and
-// non-string values alone exceed the budget, the command fails with a small
-// error instead of emitting an oversized payload.
+// command budget without changing the selected keys. List rows (many small
+// records) are shortened fairly when they overflow the budget, with
+// shortened values marked with "...". A single-object detail projection is
+// never modified: a truncated id or status string is indistinguishable from
+// a genuinely short value, so silently shortening it would hand the caller
+// wrong data instead of a compact one. If a detail projection doesn't fit,
+// the command fails with an error instead.
 func boundProjectedOutput(data any, maxBytes int) error {
-	var rows []map[string]any
 	switch value := data.(type) {
 	case map[string]any:
-		rows = []map[string]any{value}
+		return boundProjectedDetail(value, maxBytes)
 	case []map[string]any:
-		rows = value
+		return boundProjectedList(value, maxBytes)
 	default:
 		return fmt.Errorf("internal error: unsupported projected output %T", data)
 	}
+}
+
+// boundProjectedDetail rejects an oversized single-object projection instead
+// of truncating it, naming the largest fields so the caller can fix the
+// request in one pass: drop some of them from --fields, or drop --fields
+// entirely for the full, unbounded detail.
+func boundProjectedDetail(row map[string]any, maxBytes int) error {
+	encoded, err := marshalStructured(row)
+	if err != nil {
+		return err
+	}
+	if len(encoded)+1 < maxBytes {
+		return nil
+	}
+
+	type fieldSize struct {
+		name string
+		size int
+	}
+	sizes := make([]fieldSize, 0, len(row))
+	for key, value := range row {
+		fieldEncoded, err := marshalStructured(map[string]any{key: value})
+		if err != nil {
+			return err
+		}
+		sizes = append(sizes, fieldSize{key, len(fieldEncoded)})
+	}
+	// Ties break on name so the same oversized request always names the same
+	// fields, despite Go's randomized map iteration order.
+	sort.Slice(sizes, func(i, j int) bool {
+		if sizes[i].size != sizes[j].size {
+			return sizes[i].size > sizes[j].size
+		}
+		return sizes[i].name < sizes[j].name
+	})
+	if len(sizes) > 3 {
+		sizes = sizes[:3]
+	}
+	largest := make([]string, len(sizes))
+	for i, f := range sizes {
+		largest[i] = fmt.Sprintf("%s (%d bytes)", f.name, f.size)
+	}
+	return fmt.Errorf("projected detail is %d bytes, exceeds the %d-byte limit; largest fields: %s; request fewer --fields, or omit --fields for the full, unbounded detail",
+		len(encoded), maxBytes, strings.Join(largest, ", "))
+}
 
-	encoded, err := marshalStructured(data)
+// boundProjectedList shortens a list projection's string values fairly
+// (across all rows) when the compact rows themselves overflow the budget,
+// marking shortened values with "...". If keys and non-string values alone
+// exceed the budget, the command fails with a small error instead of
+// emitting an oversized payload.
+func boundProjectedList(rows []map[string]any, maxBytes int) error {
+	encoded, err := marshalStructured(rows)
 	if err != nil {
 		return err
 	}
@@ -127,7 +179,7 @@ func boundProjectedOutput(data any, maxBytes int) error {
 			}
 		}
 
-		encoded, err = marshalStructured(data)
+		encoded, err = marshalStructured(rows)
 		if err != nil {
 			return err
 		}

internal/cli/fieldproject_test.go

@@ -3,6 +3,7 @@ package cli
 import (
 	"bytes"
 	"encoding/json"
+	"reflect"
 	"strings"
 	"testing"
 	"unicode/utf8"
@@ -72,6 +73,100 @@ func TestBoundProjectedOutputRejectsIrreducibleMetadata(t *testing.T) {
 	}
 }
 
+// TestBoundProjectedOutputDetailWithinBudgetLeavesValuesUnchanged covers the
+// single-object (map[string]any) shape used by `incident detail --fields`:
+// when the projection already fits, every value must come back byte-for-byte
+// identical to what went in.
+func TestBoundProjectedOutputDetailWithinBudgetLeavesValuesUnchanged(t *testing.T) {
+	saveAndResetGlobals(t)
+	flagOutputFormat = "json"
+	row := map[string]any{
+		"incident_id": "inc-1",
+		"title":       "Disk full on db-01",
+		"progress":    "Triggered",
+	}
+	want := map[string]any{
+		"incident_id": "inc-1",
+		"title":       "Disk full on db-01",
+		"progress":    "Triggered",
+	}
+
+	if err := boundProjectedOutput(row, compactDetailOutputLimit); err != nil {
+		t.Fatalf("bound projected output: %v", err)
+	}
+	if !reflect.DeepEqual(row, want) {
+		t.Fatalf("in-budget detail projection was modified: got %v, want %v", row, want)
+	}
+}
+
+// TestBoundProjectedOutputDetailOversizedErrorsWithoutMutating is the
+// regression guard for the truncation bug: a single-object projection that
+// doesn't fit the budget must fail loudly instead of silently shipping
+// truncated (and indistinguishable-from-real) values. The input map must
+// come back completely untouched.
+func TestBoundProjectedOutputDetailOversizedErrorsWithoutMutating(t *testing.T) {
+	saveAndResetGlobals(t)
+	flagOutputFormat = "json"
+	row := map[string]any{
+		"incident_id": "inc-1",
+		"title":       strings.Repeat("数据库故障", 5000),
+		"root_cause":  strings.Repeat("disk exhaustion details ", 3000),
+	}
+	want := map[string]any{
+		"incident_id": "inc-1",
+		"title":       strings.Repeat("数据库故障", 5000),
+		"root_cause":  strings.Repeat("disk exhaustion details ", 3000),
+	}
+
+	err := boundProjectedOutput(row, 512)
+	if err == nil {
+		t.Fatal("expected an error for an oversized detail projection, got nil")
+	}
+	if !strings.Contains(err.Error(), "512") {
+		t.Errorf("error should name the byte budget (512), got: %v", err)
+	}
+	if !strings.Contains(err.Error(), "title") && !strings.Contains(err.Error(), "root_cause") {
+		t.Errorf("error should name the largest field, got: %v", err)
+	}
+	if !reflect.DeepEqual(row, want) {
+		t.Fatalf("oversized detail projection mutated the input map: got %v, want %v", row, want)
+	}
+}
+
+// TestBoundProjectedOutputDetailErrorIsDeterministic pins the tie-break in the
+// largest-field ranking: with several fields at exactly the same encoded size,
+// Go's randomized map iteration order must not leak into the error text, or the
+// same failing command would name different fields on each run.
+func TestBoundProjectedOutputDetailErrorIsDeterministic(t *testing.T) {
+	saveAndResetGlobals(t)
+	flagOutputFormat = "json"
+
+	first := ""
+	for i := range 20 {
+		row := map[string]any{
+			"alpha":   strings.Repeat("a", 400),
+			"bravo":   strings.Repeat("b", 400),
+			"charlie": strings.Repeat("c", 400),
+			"delta":   strings.Repeat("d", 400),
+			"echo":    strings.Repeat("e", 400),
+		}
+		err := boundProjectedOutput(row, 512)
+		if err == nil {
+			t.Fatal("expected an error for an oversized detail projection, got nil")
+		}
+		if i == 0 {
+			first = err.Error()
+			continue
+		}
+		if err.Error() != first {
+			t.Fatalf("error text varies between runs on equal-sized fields:\n run 0: %s\n run %d: %s", first, i, err.Error())
+		}
+	}
+	if !strings.Contains(first, "alpha") {
+		t.Errorf("equal-sized fields should be ranked by name, expected alpha first, got: %s", first)
+	}
+}
+
 func alertRow() map[string]any {
 	return map[string]any{
 		"alert_id":       "al-1",
@@ -441,11 +536,11 @@ func TestIncidentDetailFieldsProjection(t *testing.T) {
 	saveAndResetGlobals(t)
 	stub := newGFStub(t)
 	row := incidentRow()
-	row["description"] = strings.Repeat("large description ", 500)
-	row["images"] = []map[string]any{{"src": strings.Repeat("https://example.test/image/", 100)}}
-	row["ai_summary"] = strings.Repeat("long AI summary ", 4000)
-	row["root_cause"] = strings.Repeat("long root cause ", 4000)
-	row["resolution"] = strings.Repeat("long resolution ", 4000)
+	row["description"] = "root volume at 98%, mount point /var/lib/db"
+	row["images"] = []map[string]any{{"src": "https://example.test/image/1.png"}}
+	row["ai_summary"] = "Disk usage crossed 98% on db-01 at 03:14 UTC after log rotation stopped."
+	row["root_cause"] = "Log rotation was disabled by the previous deploy's config change."
+	row["resolution"] = "Re-enabled log rotation and cleared the stale archive files."
 	stub.data = row
 
 	fields := []string{"incident_id", "title", "incident_severity", "progress", "ai_summary", "root_cause", "resolution", "alert_cnt", "start_time", "channel_id"}
@@ -472,6 +567,43 @@ func TestIncidentDetailFieldsProjection(t *testing.T) {
 		t.Errorf("projected detail includes description: %v", detail)
 	}
 
+	// The projected values must come back byte-for-byte identical to the
+	// source row — this command must never truncate a detail value.
+	var gotSummary string
+	if err := json.Unmarshal(detail["ai_summary"], &gotSummary); err != nil {
+		t.Fatalf("unmarshal ai_summary: %v", err)
+	}
+	if gotSummary != row["ai_summary"] {
+		t.Fatalf("ai_summary was altered: got %q, want %q", gotSummary, row["ai_summary"])
+	}
+}
+
+// TestIncidentDetailFieldsProjectionOversizedErrors: when the requested
+// --fields don't fit the 8 KiB detail budget, the command must fail with an
+// actionable error instead of silently shipping truncated values.
+func TestIncidentDetailFieldsProjectionOversizedErrors(t *testing.T) {
+	saveAndResetGlobals(t)
+	stub := newGFStub(t)
+	row := incidentRow()
+	row["ai_summary"] = strings.Repeat("long AI summary ", 4000)
+	row["root_cause"] = strings.Repeat("long root cause ", 4000)
+	row["resolution"] = strings.Repeat("long resolution ", 4000)
+	stub.data = row
+
+	fields := []string{"incident_id", "title", "ai_summary", "root_cause", "resolution"}
+	_, err := execCommand("incident", "detail", "inc-1", "--fields", strings.Join(fields, ","), "--output-format", "json")
+	if err == nil {
+		t.Fatal("expected an error for an oversized detail projection, got nil")
+	}
+	if !strings.Contains(err.Error(), "8192") {
+		t.Errorf("error should name the byte budget (8192), got: %v", err)
+	}
+	if !strings.Contains(err.Error(), "ai_summary") && !strings.Contains(err.Error(), "root_cause") && !strings.Contains(err.Error(), "resolution") {
+		t.Errorf("error should name one of the largest fields, got: %v", err)
+	}
+	if !strings.Contains(err.Error(), "--fields") {
+		t.Errorf("error should point at --fields as the remedy, got: %v", err)
+	}
 }
 
 func TestAlertEventListStructuredProjection(t *testing.T) {

internal/cli/incident.go

@@ -1554,7 +1554,7 @@ func newIncidentDetailCmd() *cobra.Command {
 			})
 		},
 	}
-	cmd.Flags().StringVar(&fields, "fields", "", "Comma-separated fields to project in json/toon output (e.g. incident_id,title,incident_severity,progress,root_cause); ignored in table mode. Projected strings are truncated as needed to keep output below 8 KiB; omit --fields for full detail.")
+	cmd.Flags().StringVar(&fields, "fields", "", "Comma-separated fields to project in json/toon output (e.g. incident_id,title,incident_severity,progress,root_cause); ignored in table mode. The projection must fit within 8 KiB or the command fails and names the largest fields; omit --fields for the full, unbounded detail.")
 	return cmd
 }
 

skills/flashduty/reference/incident.md

@@ -71,7 +71,7 @@ fduty incident comment "$ID" --comment-file "$COMMENT_FILE"
 fduty incident resolve <incident-id> --root-cause "DB primary failover delay" --resolution "Failover completed; latency normal."
 ```
 
-Projected `similar` lists stay below 16 KiB, and projected `detail --fields` output stays below 8 KiB. A trailing `...` means a long retained string was shortened; omit `--fields` only when the full unbounded detail is explicitly required.
+Projected `similar` lists stay below 16 KiB; a trailing `...` in a list row means a long retained string was shortened. `detail --fields` is different: it never shortens values — the projection must fit within 8 KiB as requested or the command fails and names the largest fields, so drop some fields (or drop `--fields` for the full unbounded detail) and retry.
 
 `comment` never accepts the text as a command-line argument — only `--comment-file <path>` (or `--comment-file -` to read stdin), so backticks/`$()`/quotes inside the comment are inert. The command also reads back every target's timeline after writing and exits non-zero unless it finds an entry matching what it sent, so `Commented on ...` is proof of content fidelity, not just acceptance — no separate manual read-back is needed. Leading and trailing whitespace is stripped before sending (the server strips it too, so this is what gets stored); everything else, including interior blank lines, is preserved exactly.