Skip to content

Commit 4851e44

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 70098e9 commit 4851e44

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
@@ -70,23 +70,75 @@ func projectFields(items any, fields []string) ([]map[string]any, error) {
7070
}
7171

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

89-
encoded, err := marshalStructured(data)
135+
// boundProjectedList shortens a list projection's string values fairly
136+
// (across all rows) when the compact rows themselves overflow the budget,
137+
// marking shortened values with "...". If keys and non-string values alone
138+
// exceed the budget, the command fails with a small error instead of
139+
// emitting an oversized payload.
140+
func boundProjectedList(rows []map[string]any, maxBytes int) error {
141+
encoded, err := marshalStructured(rows)
90142
if err != nil {
91143
return err
92144
}
@@ -116,7 +168,7 @@ func boundProjectedOutput(data any, maxBytes int) error {
116168
}
117169
}
118170

119-
encoded, err = marshalStructured(data)
171+
encoded, err = marshalStructured(rows)
120172
if err != nil {
121173
return err
122174
}

‎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",
@@ -430,11 +525,11 @@ func TestIncidentDetailFieldsProjection(t *testing.T) {
430525
saveAndResetGlobals(t)
431526
stub := newGFStub(t)
432527
row := incidentRow()
433-
row["description"] = strings.Repeat("large description ", 500)
434-
row["images"] = []map[string]any{{"src": strings.Repeat("https://example.test/image/", 100)}}
435-
row["ai_summary"] = strings.Repeat("long AI summary ", 4000)
436-
row["root_cause"] = strings.Repeat("long root cause ", 4000)
437-
row["resolution"] = strings.Repeat("long resolution ", 4000)
528+
row["description"] = "root volume at 98%, mount point /var/lib/db"
529+
row["images"] = []map[string]any{{"src": "https://example.test/image/1.png"}}
530+
row["ai_summary"] = "Disk usage crossed 98% on db-01 at 03:14 UTC after log rotation stopped."
531+
row["root_cause"] = "Log rotation was disabled by the previous deploy's config change."
532+
row["resolution"] = "Re-enabled log rotation and cleared the stale archive files."
438533
stub.data = row
439534

440535
fields := []string{"incident_id", "title", "incident_severity", "progress", "ai_summary", "root_cause", "resolution", "alert_cnt", "start_time", "channel_id"}
@@ -461,6 +556,43 @@ func TestIncidentDetailFieldsProjection(t *testing.T) {
461556
t.Errorf("projected detail includes description: %v", detail)
462557
}
463558

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

466598
func TestAlertEventListStructuredProjection(t *testing.T) {

‎internal/cli/incident.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1587,7 +1587,7 @@ func newIncidentDetailCmd() *cobra.Command {
15871587
})
15881588
},
15891589
}
1590-
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.")
1590+
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.")
15911591
return cmd
15921592
}
15931593

‎skills/flashduty/reference/incident.md‎

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

77-
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.
77+
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.
7878

7979
`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.
8080

0 commit comments

Comments
 (0)