Skip to content

Commit 1deaf6d

Browse files
ysyneuysyneu
committed
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.
1 parent 8cd3090 commit 1deaf6d

4 files changed

Lines changed: 201 additions & 17 deletions

File tree

‎internal/cli/fieldproject.go‎

Lines changed: 62 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,23 +81,75 @@ func noteDefaultProjection(w io.Writer, fields []string) {
8181
}
8282

8383
// boundProjectedOutput keeps the new agent-oriented projections below their
84-
// command budget without changing the selected keys. Short values remain byte
85-
// identical; when retained strings alone would overflow the actual JSON/TOON
86-
// encoding, they are shortened fairly and marked with "...". If keys and
87-
// non-string values alone exceed the budget, the command fails with a small
88-
// error instead of emitting an oversized payload.
84+
// command budget without changing the selected keys. List rows (many small
85+
// records) are shortened fairly when they overflow the budget, with
86+
// shortened values marked with "...". A single-object detail projection is
87+
// never modified: a truncated id or status string is indistinguishable from
88+
// a genuinely short value, so silently shortening it would hand the caller
89+
// wrong data instead of a compact one. If a detail projection doesn't fit,
90+
// the command fails with an error instead.
8991
func boundProjectedOutput(data any, maxBytes int) error {
90-
var rows []map[string]any
9192
switch value := data.(type) {
9293
case map[string]any:
93-
rows = []map[string]any{value}
94+
return boundProjectedDetail(value, maxBytes)
9495
case []map[string]any:
95-
rows = value
96+
return boundProjectedList(value, maxBytes)
9697
default:
9798
return fmt.Errorf("internal error: unsupported projected output %T", data)
9899
}
100+
}
101+
102+
// boundProjectedDetail rejects an oversized single-object projection instead
103+
// of truncating it, naming the largest fields so the caller can fix the
104+
// request in one pass: drop some of them from --fields, or drop --fields
105+
// entirely for the full, unbounded detail.
106+
func boundProjectedDetail(row map[string]any, maxBytes int) error {
107+
encoded, err := marshalStructured(row)
108+
if err != nil {
109+
return err
110+
}
111+
if len(encoded)+1 < maxBytes {
112+
return nil
113+
}
114+
115+
type fieldSize struct {
116+
name string
117+
size int
118+
}
119+
sizes := make([]fieldSize, 0, len(row))
120+
for key, value := range row {
121+
fieldEncoded, err := marshalStructured(map[string]any{key: value})
122+
if err != nil {
123+
return err
124+
}
125+
sizes = append(sizes, fieldSize{key, len(fieldEncoded)})
126+
}
127+
// Ties break on name so the same oversized request always names the same
128+
// fields, despite Go's randomized map iteration order.
129+
sort.Slice(sizes, func(i, j int) bool {
130+
if sizes[i].size != sizes[j].size {
131+
return sizes[i].size > sizes[j].size
132+
}
133+
return sizes[i].name < sizes[j].name
134+
})
135+
if len(sizes) > 3 {
136+
sizes = sizes[:3]
137+
}
138+
largest := make([]string, len(sizes))
139+
for i, f := range sizes {
140+
largest[i] = fmt.Sprintf("%s (%d bytes)", f.name, f.size)
141+
}
142+
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",
143+
len(encoded), maxBytes, strings.Join(largest, ", "))
144+
}
99145

100-
encoded, err := marshalStructured(data)
146+
// boundProjectedList shortens a list projection's string values fairly
147+
// (across all rows) when the compact rows themselves overflow the budget,
148+
// marking shortened values with "...". If keys and non-string values alone
149+
// exceed the budget, the command fails with a small error instead of
150+
// emitting an oversized payload.
151+
func boundProjectedList(rows []map[string]any, maxBytes int) error {
152+
encoded, err := marshalStructured(rows)
101153
if err != nil {
102154
return err
103155
}
@@ -127,7 +179,7 @@ func boundProjectedOutput(data any, maxBytes int) error {
127179
}
128180
}
129181

130-
encoded, err = marshalStructured(data)
182+
encoded, err = marshalStructured(rows)
131183
if err != nil {
132184
return err
133185
}

‎internal/cli/fieldproject_test.go‎

Lines changed: 137 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package cli
33
import (
44
"bytes"
55
"encoding/json"
6+
"reflect"
67
"strings"
78
"testing"
89
"unicode/utf8"
@@ -72,6 +73,100 @@ func TestBoundProjectedOutputRejectsIrreducibleMetadata(t *testing.T) {
7273
}
7374
}
7475

76+
// TestBoundProjectedOutputDetailWithinBudgetLeavesValuesUnchanged covers the
77+
// single-object (map[string]any) shape used by `incident detail --fields`:
78+
// when the projection already fits, every value must come back byte-for-byte
79+
// identical to what went in.
80+
func TestBoundProjectedOutputDetailWithinBudgetLeavesValuesUnchanged(t *testing.T) {
81+
saveAndResetGlobals(t)
82+
flagOutputFormat = "json"
83+
row := map[string]any{
84+
"incident_id": "inc-1",
85+
"title": "Disk full on db-01",
86+
"progress": "Triggered",
87+
}
88+
want := map[string]any{
89+
"incident_id": "inc-1",
90+
"title": "Disk full on db-01",
91+
"progress": "Triggered",
92+
}
93+
94+
if err := boundProjectedOutput(row, compactDetailOutputLimit); err != nil {
95+
t.Fatalf("bound projected output: %v", err)
96+
}
97+
if !reflect.DeepEqual(row, want) {
98+
t.Fatalf("in-budget detail projection was modified: got %v, want %v", row, want)
99+
}
100+
}
101+
102+
// TestBoundProjectedOutputDetailOversizedErrorsWithoutMutating is the
103+
// regression guard for the truncation bug: a single-object projection that
104+
// doesn't fit the budget must fail loudly instead of silently shipping
105+
// truncated (and indistinguishable-from-real) values. The input map must
106+
// come back completely untouched.
107+
func TestBoundProjectedOutputDetailOversizedErrorsWithoutMutating(t *testing.T) {
108+
saveAndResetGlobals(t)
109+
flagOutputFormat = "json"
110+
row := map[string]any{
111+
"incident_id": "inc-1",
112+
"title": strings.Repeat("数据库故障", 5000),
113+
"root_cause": strings.Repeat("disk exhaustion details ", 3000),
114+
}
115+
want := map[string]any{
116+
"incident_id": "inc-1",
117+
"title": strings.Repeat("数据库故障", 5000),
118+
"root_cause": strings.Repeat("disk exhaustion details ", 3000),
119+
}
120+
121+
err := boundProjectedOutput(row, 512)
122+
if err == nil {
123+
t.Fatal("expected an error for an oversized detail projection, got nil")
124+
}
125+
if !strings.Contains(err.Error(), "512") {
126+
t.Errorf("error should name the byte budget (512), got: %v", err)
127+
}
128+
if !strings.Contains(err.Error(), "title") && !strings.Contains(err.Error(), "root_cause") {
129+
t.Errorf("error should name the largest field, got: %v", err)
130+
}
131+
if !reflect.DeepEqual(row, want) {
132+
t.Fatalf("oversized detail projection mutated the input map: got %v, want %v", row, want)
133+
}
134+
}
135+
136+
// TestBoundProjectedOutputDetailErrorIsDeterministic pins the tie-break in the
137+
// largest-field ranking: with several fields at exactly the same encoded size,
138+
// Go's randomized map iteration order must not leak into the error text, or the
139+
// same failing command would name different fields on each run.
140+
func TestBoundProjectedOutputDetailErrorIsDeterministic(t *testing.T) {
141+
saveAndResetGlobals(t)
142+
flagOutputFormat = "json"
143+
144+
first := ""
145+
for i := range 20 {
146+
row := map[string]any{
147+
"alpha": strings.Repeat("a", 400),
148+
"bravo": strings.Repeat("b", 400),
149+
"charlie": strings.Repeat("c", 400),
150+
"delta": strings.Repeat("d", 400),
151+
"echo": strings.Repeat("e", 400),
152+
}
153+
err := boundProjectedOutput(row, 512)
154+
if err == nil {
155+
t.Fatal("expected an error for an oversized detail projection, got nil")
156+
}
157+
if i == 0 {
158+
first = err.Error()
159+
continue
160+
}
161+
if err.Error() != first {
162+
t.Fatalf("error text varies between runs on equal-sized fields:\n run 0: %s\n run %d: %s", first, i, err.Error())
163+
}
164+
}
165+
if !strings.Contains(first, "alpha") {
166+
t.Errorf("equal-sized fields should be ranked by name, expected alpha first, got: %s", first)
167+
}
168+
}
169+
75170
func alertRow() map[string]any {
76171
return map[string]any{
77172
"alert_id": "al-1",
@@ -441,11 +536,11 @@ func TestIncidentDetailFieldsProjection(t *testing.T) {
441536
saveAndResetGlobals(t)
442537
stub := newGFStub(t)
443538
row := incidentRow()
444-
row["description"] = strings.Repeat("large description ", 500)
445-
row["images"] = []map[string]any{{"src": strings.Repeat("https://example.test/image/", 100)}}
446-
row["ai_summary"] = strings.Repeat("long AI summary ", 4000)
447-
row["root_cause"] = strings.Repeat("long root cause ", 4000)
448-
row["resolution"] = strings.Repeat("long resolution ", 4000)
539+
row["description"] = "root volume at 98%, mount point /var/lib/db"
540+
row["images"] = []map[string]any{{"src": "https://example.test/image/1.png"}}
541+
row["ai_summary"] = "Disk usage crossed 98% on db-01 at 03:14 UTC after log rotation stopped."
542+
row["root_cause"] = "Log rotation was disabled by the previous deploy's config change."
543+
row["resolution"] = "Re-enabled log rotation and cleared the stale archive files."
449544
stub.data = row
450545

451546
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) {
472567
t.Errorf("projected detail includes description: %v", detail)
473568
}
474569

570+
// The projected values must come back byte-for-byte identical to the
571+
// source row — this command must never truncate a detail value.
572+
var gotSummary string
573+
if err := json.Unmarshal(detail["ai_summary"], &gotSummary); err != nil {
574+
t.Fatalf("unmarshal ai_summary: %v", err)
575+
}
576+
if gotSummary != row["ai_summary"] {
577+
t.Fatalf("ai_summary was altered: got %q, want %q", gotSummary, row["ai_summary"])
578+
}
579+
}
580+
581+
// TestIncidentDetailFieldsProjectionOversizedErrors: when the requested
582+
// --fields don't fit the 8 KiB detail budget, the command must fail with an
583+
// actionable error instead of silently shipping truncated values.
584+
func TestIncidentDetailFieldsProjectionOversizedErrors(t *testing.T) {
585+
saveAndResetGlobals(t)
586+
stub := newGFStub(t)
587+
row := incidentRow()
588+
row["ai_summary"] = strings.Repeat("long AI summary ", 4000)
589+
row["root_cause"] = strings.Repeat("long root cause ", 4000)
590+
row["resolution"] = strings.Repeat("long resolution ", 4000)
591+
stub.data = row
592+
593+
fields := []string{"incident_id", "title", "ai_summary", "root_cause", "resolution"}
594+
_, err := execCommand("incident", "detail", "inc-1", "--fields", strings.Join(fields, ","), "--output-format", "json")
595+
if err == nil {
596+
t.Fatal("expected an error for an oversized detail projection, got nil")
597+
}
598+
if !strings.Contains(err.Error(), "8192") {
599+
t.Errorf("error should name the byte budget (8192), got: %v", err)
600+
}
601+
if !strings.Contains(err.Error(), "ai_summary") && !strings.Contains(err.Error(), "root_cause") && !strings.Contains(err.Error(), "resolution") {
602+
t.Errorf("error should name one of the largest fields, got: %v", err)
603+
}
604+
if !strings.Contains(err.Error(), "--fields") {
605+
t.Errorf("error should point at --fields as the remedy, got: %v", err)
606+
}
475607
}
476608

477609
func TestAlertEventListStructuredProjection(t *testing.T) {

‎internal/cli/incident.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1554,7 +1554,7 @@ func newIncidentDetailCmd() *cobra.Command {
15541554
})
15551555
},
15561556
}
1557-
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.")
1557+
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.")
15581558
return cmd
15591559
}
15601560

‎skills/flashduty/reference/incident.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ fduty incident comment "$ID" --comment-file "$COMMENT_FILE"
7171
fduty incident resolve <incident-id> --root-cause "DB primary failover delay" --resolution "Failover completed; latency normal."
7272
```
7373

74-
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.
74+
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.
7575

7676
`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.
7777

0 commit comments

Comments
 (0)