From 1deaf6d159e15b5b178dbcb3ec82fc8bb52f137f Mon Sep 17 00:00:00 2001 From: ysyneu Date: Mon, 10 Aug 2026 21:21:21 -0700 Subject: [PATCH] fix(cli): never truncate incident detail --fields values boundProjectedOutput applied one budget strategy to both shapes it receives: list projections (many small rows) and the single-object projection behind `incident detail --fields`. On overflow it shortened every string in place, then halved the per-field limit and retried. On a single object that silently corrupts data. The limit is a global budget divided by the number of string slots, so short fields are punished for long ones sharing the object, and every round re-reads the already-shortened value, compounding it. Once the limit drops below 4, truncateUTF8Bytes stops appending the "..." marker, so a 24-character id can arrive as "6" and "Warning" as "W" -- indistinguishable from a genuinely short value. The loop's fieldLimit == 0 error exit is unreachable: by then every string is empty, so the payload always fits and the command exits 0 with hollowed-out fields. Split the two shapes. Lists keep the existing shorten-and-mark behavior byte for byte. A detail projection is now never modified: if it does not fit, the command fails and names the largest fields with their sizes, so the caller can drop some --fields, or omit --fields for the full, unbounded detail -- which returns more data, not less, so the error is always actionable. Dropping keys instead was rejected: an absent key is indistinguishable from a null one to a jq consumer, which reintroduces the same class of bug this removes. Ties in the largest-field ranking break on name, so the same oversized request never names different fields between runs. --- internal/cli/fieldproject.go | 72 +++++++++++-- internal/cli/fieldproject_test.go | 142 ++++++++++++++++++++++++- internal/cli/incident.go | 2 +- skills/flashduty/reference/incident.md | 2 +- 4 files changed, 201 insertions(+), 17 deletions(-) diff --git a/internal/cli/fieldproject.go b/internal/cli/fieldproject.go index 02a7a63..b754875 100644 --- a/internal/cli/fieldproject.go +++ b/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 } diff --git a/internal/cli/fieldproject_test.go b/internal/cli/fieldproject_test.go index 8bbf251..3437219 100644 --- a/internal/cli/fieldproject_test.go +++ b/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) { diff --git a/internal/cli/incident.go b/internal/cli/incident.go index 5deb072..9d9f2f5 100644 --- a/internal/cli/incident.go +++ b/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 } diff --git a/skills/flashduty/reference/incident.md b/skills/flashduty/reference/incident.md index 1cdbb0f..5239d73 100644 --- a/skills/flashduty/reference/incident.md +++ b/skills/flashduty/reference/incident.md @@ -71,7 +71,7 @@ fduty incident comment "$ID" --comment-file "$COMMENT_FILE" fduty incident resolve --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 ` (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.