From bf73242f86fc0a6f29e99e786693066d8fce99a4 Mon Sep 17 00:00:00 2001 From: ysyneu Date: Fri, 11 Sep 2026 01:54:09 -0700 Subject: [PATCH] fix(cli): report a reduced page's facts from the payload that is printed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A paginated envelope over the structured-output limit is re-fit several times, each pass bounding the same rows against a smaller budget. Two things went wrong there. The value-shortening path clipped the caller's row maps in place, so a later pass measured data an earlier pass had already clipped and the facts it returned described only the last step: stderr could announce "every value intact" while an emitted value ended in "...", and the payload carried emitted_rows — which documents that paging repairs the rest — next to a clipped value paging cannot restore. The path now builds fresh rows and never modifies the input. The marker keys were set only, never cleared, so a key stamped by an earlier pass could survive into a payload of a different shape; both are now stamped from the current pass alone. The bounding helper takes the published limit alongside the framing the payload costs around the rows, so the note and the refusal quote the documented 16 KiB cap rather than an internal remainder, and the refusal states the row list's size and the limit without comparing numbers measured in different spaces (the row array's own encoding vs the payload's cap). Its size now counts the rendered bytes the fit test measured. --- cmd/flashduty/main_test.go | 2 +- internal/cli/command_test.go | 6 +-- internal/cli/fieldproject.go | 68 +++++++++++++++++-------- internal/cli/fieldproject_test.go | 52 +++++++++++++++++-- internal/cli/gen_support.go | 44 +++++++++------- internal/cli/gen_support_test.go | 85 +++++++++++++++++++++++++++++++ 6 files changed, 207 insertions(+), 50 deletions(-) diff --git a/cmd/flashduty/main_test.go b/cmd/flashduty/main_test.go index 4f52b74..013d8ea 100644 --- a/cmd/flashduty/main_test.go +++ b/cmd/flashduty/main_test.go @@ -152,7 +152,7 @@ func TestProjectionOverflowFailsHard(t *testing.T) { if stdout.Len() != 0 { t.Errorf("[#79] a failed projection must write nothing to stdout, got %d bytes:\n%s", stdout.Len(), stdout.String()) } - if !strings.Contains(stderr.String(), "Error: projected list is") || !strings.Contains(stderr.String(), "exceeds the 16384-byte limit") { + if !strings.Contains(stderr.String(), "Error: projected list is") || !strings.Contains(stderr.String(), "16384-byte structured-output limit") { t.Errorf("[#79] stderr should report the byte-limit refusal, got:\n%s", stderr.String()) } } diff --git a/internal/cli/command_test.go b/internal/cli/command_test.go index e65232e..9187b25 100644 --- a/internal/cli/command_test.go +++ b/internal/cli/command_test.go @@ -1721,13 +1721,13 @@ func TestCommandListProjectionOverflowFails(t *testing.T) { stub.data = map[string]any{"items": []any{row}, "total": 1} out, stderrText, err := execCommandSplit("incident", "list", "--fields", "incident_id,labels", "--output-format", "json") - if err == nil || !strings.Contains(err.Error(), "exceeds the 16384-byte limit") { + if err == nil || !strings.Contains(err.Error(), "16384-byte structured-output limit") { t.Fatalf("irreducible projection error = %v, want the byte-limit refusal", err) } if out != "" { t.Errorf("a failed projection must write nothing to stdout, got %d bytes", len(out)) } - if strings.Contains(stderrText, "exceeds the") { + if strings.Contains(stderrText, "structured-output limit") { t.Errorf("the error is returned for the entrypoint to report, not printed mid-run, got:\n%s", stderrText) } }) @@ -1747,7 +1747,7 @@ func TestCommandListProjectionOverflowFails(t *testing.T) { stub.data = map[string]any{"items": []any{row}, "total": 1} out, _, err := execCommandSplit("alert-event", "list", "--fields", "event_id,labels", "--output-format", "json") - if err == nil || !strings.Contains(err.Error(), "exceeds the 16384-byte limit") { + if err == nil || !strings.Contains(err.Error(), "16384-byte structured-output limit") { t.Fatalf("irreducible projection error = %v, want the byte-limit refusal", err) } if out != "" { diff --git a/internal/cli/fieldproject.go b/internal/cli/fieldproject.go index 1f9ae82..47245d0 100644 --- a/internal/cli/fieldproject.go +++ b/internal/cli/fieldproject.go @@ -98,7 +98,8 @@ type projectionBound struct { shortened int valuesTotal int fields []string - // maxBytes is the budget the reduction was sized against. + // maxBytes is the published limit the payload had to fit under — not the + // rows' share of it, which is smaller when they ride inside an envelope. maxBytes int } @@ -203,20 +204,29 @@ func noteProjectionBound(cmd *cobra.Command, bound projectionBound) { // callers turn it into the command-appropriate failure with // explainProjectionOverflow. type projectionOverflow struct { - detail bool - bytes int - rows int + detail bool + bytes int + rows int + // maxBytes is the published limit, so the error names that cap rather than + // the rows' internal share of it. maxBytes int largest string } func (o *projectionOverflow) Error() string { if o.detail { - return fmt.Sprintf("projected detail is %d bytes, exceeds the %d-byte limit; largest fields: %s", + return fmt.Sprintf("projected detail is %d bytes and does not fit under the %d-byte structured-output limit; largest fields: %s", o.bytes, o.maxBytes, o.largest) } - return fmt.Sprintf("projected list is %d bytes across %d rows, exceeds the %d-byte limit; largest fields: %s", - o.bytes, o.rows, o.maxBytes, o.largest) + // The count is the rows' own encoding, the limit the payload's: inside an + // envelope the two live in different spaces, so the sentence states the + // size and the refusal separately rather than comparing them. + rowNoun := "rows" + if o.rows == 1 { + rowNoun = "row" + } + return fmt.Sprintf("projected list is %d bytes across %d %s and cannot be reduced to fit under the %d-byte structured-output limit; largest fields: %s", + o.bytes, o.rows, rowNoun, o.maxBytes, o.largest) } // explainProjectionOverflow returns err with the remedy for cmd appended when @@ -259,7 +269,7 @@ func boundProjectedOutput(data any, maxBytes int) (any, projectionBound, error) case map[string]any: return value, projectionBound{}, boundProjectedDetail(value, maxBytes) case []map[string]any: - return boundProjectedList(value, maxBytes) + return boundProjectedList(value, maxBytes, 0) default: return nil, projectionBound{}, fmt.Errorf("internal error: unsupported projected output %T", data) } @@ -325,7 +335,9 @@ func boundProjectedDetail(row map[string]any, maxBytes int) error { if err != nil { return err } - return &projectionOverflow{detail: true, bytes: len(encoded), maxBytes: maxBytes, largest: largest} + // +1 for the trailing newline the printer appends, so the reported size is + // the one the fit test just measured. + return &projectionOverflow{detail: true, bytes: len(encoded) + 1, maxBytes: maxBytes, largest: largest} } // isIdentifierField reports whether a projected field is an identifier: @@ -356,12 +368,21 @@ func isIdentifierField(key string) bool { // command fails with a small error instead of emitting values that look // real but aren't. Whatever it reduces or clips, it reports back in the // returned projectionBound. -func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, projectionBound, error) { +// +// The rows are sized against limit minus framing, where framing is the bytes +// the payload costs around them besides the row array (0 when the rows are the +// payload), while the returned facts name limit: the caller's note quotes the +// published cap, not an internal remainder. The input rows are never modified, +// so a caller that re-bounds them against a smaller budget (the envelope +// re-fit) measures every pass from the original values and reports facts that +// match the payload it prints. +func boundProjectedList(rows []map[string]any, limit, framing int) ([]map[string]any, projectionBound, error) { + budget := limit - framing encoded, err := marshalStructured(rows) if err != nil { return nil, projectionBound{}, err } - if len(encoded)+1 < maxBytes { + if len(encoded)+1 < budget { return rows, projectionBound{}, nil } @@ -372,15 +393,17 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, if err != nil { return nil, projectionBound{}, err } - return nil, projectionBound{}, &projectionOverflow{bytes: len(encoded), rows: len(rows), maxBytes: maxBytes, largest: largest} + // +1 for the trailing newline the printer appends, so the reported size + // is the one the fit test just measured. + return nil, projectionBound{}, &projectionOverflow{bytes: len(encoded) + 1, rows: len(rows), maxBytes: limit, largest: largest} } - kept, err := largestFittingPrefix(rows, maxBytes) + kept, err := largestFittingPrefix(rows, budget) if err != nil { return nil, projectionBound{}, err } if kept > 0 { - return rows[:kept], projectionBound{rowsEmitted: kept, rowsTotal: len(rows), maxBytes: maxBytes}, nil + return rows[:kept], projectionBound{rowsEmitted: kept, rowsTotal: len(rows), maxBytes: limit}, nil } maxLen := 0 @@ -415,7 +438,7 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, if err != nil { return false, err } - return len(trialEncoded)+1 < maxBytes, nil + return len(trialEncoded)+1 < budget, nil } // minMarkedTruncationCap is the smallest cap for which truncateUTF8Bytes @@ -453,13 +476,13 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, shortened, total := 0, 0 fields := map[string]bool{} - for _, row := range rows { + bounded := make([]map[string]any, len(rows)) + for i, row := range rows { + boundedRow := make(map[string]any, len(row)) for key, value := range row { - if isIdentifierField(key) { - continue - } text, ok := value.(string) - if !ok { + if !ok || isIdentifierField(key) { + boundedRow[key] = value continue } total++ @@ -468,8 +491,9 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, shortened++ fields[key] = true } - row[key] = clipped + boundedRow[key] = clipped } + bounded[i] = boundedRow } if shortened == 0 { return rows, projectionBound{}, nil @@ -479,7 +503,7 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, names = append(names, name) } sort.Strings(names) - return rows, projectionBound{shortened: shortened, valuesTotal: total, fields: names, maxBytes: maxBytes}, nil + return bounded, projectionBound{shortened: shortened, valuesTotal: total, fields: names, maxBytes: limit}, nil } // largestFittingPrefix returns the largest n < len(rows) whose encoded prefix diff --git a/internal/cli/fieldproject_test.go b/internal/cli/fieldproject_test.go index 907a0ad..ee4f5a8 100644 --- a/internal/cli/fieldproject_test.go +++ b/internal/cli/fieldproject_test.go @@ -38,22 +38,34 @@ func TestBoundProjectedOutputCapsStructuredFormats(t *testing.T) { t.Run(format, func(t *testing.T) { saveAndResetGlobals(t) flagOutputFormat = format + longTitle := strings.Repeat("数据库故障", 2000) rows := []map[string]any{{ "incident_id": "inc-1", - "title": strings.Repeat("数据库故障", 2000), + "title": longTitle, }} - if _, _, err := boundProjectedOutput(rows, 512); err != nil { + bounded, _, err := boundProjectedOutput(rows, 512) + if err != nil { t.Fatalf("bound projected output: %v", err) } - encoded, err := marshalStructured(rows) + boundedRows, ok := bounded.([]map[string]any) + if !ok { + t.Fatalf("bounded output = %T, want []map[string]any", bounded) + } + encoded, err := marshalStructured(boundedRows) if err != nil { t.Fatalf("marshal bounded output: %v", err) } if len(encoded)+1 >= 512 { t.Fatalf("bounded %s output is %d bytes, want <512", format, len(encoded)+1) } - title := rows[0]["title"].(string) + // The input stays untouched: the caller prints the returned value, + // and the envelope re-fit re-bounds the same rows against a smaller + // budget, so every pass must see the original data. + if original, _ := rows[0]["title"].(string); original != longTitle { + t.Fatalf("bounding modified the input row: title is %d bytes, want it untouched", len(original)) + } + title := boundedRows[0]["title"].(string) if !utf8.ValidString(title) || !strings.HasSuffix(title, "...") { t.Fatalf("truncated title = %q, want valid UTF-8 with marker", title) } @@ -61,6 +73,36 @@ func TestBoundProjectedOutputCapsStructuredFormats(t *testing.T) { } } +// TestBoundProjectedOutputRefusesOversizeDetail pins the detail half of the +// refusal: a single-object projection is never modified (a clipped id or status +// would pass for a real value), so one that overflows the detail limit fails +// instead, and its message reports the rendered size the fit test measured +// against the detail cap — the two numbers live in the same space here, so the +// sentence can state them together. +func TestBoundProjectedOutputRefusesOversizeDetail(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + row := map[string]any{ + "incident_id": "inc-1", + "counts": make([]int, 5000), + } + + _, _, err := boundProjectedOutput(row, compactDetailOutputLimit) + if err == nil { + t.Fatal("oversize detail = nil error, want the overflow refusal") + } + if !strings.Contains(err.Error(), "projected detail is ") { + t.Fatalf("detail refusal should name the detail payload, got: %v", err) + } + want := fmt.Sprintf("does not fit under the %d-byte structured-output limit", compactDetailOutputLimit) + if !strings.Contains(err.Error(), want) || !strings.Contains(err.Error(), "counts") { + t.Fatalf("detail refusal = %v, want %q and the largest field", err, want) + } + if strings.Contains(err.Error(), "exceeds") { + t.Fatalf("the refusal must not claim a size exceeds a limit it was measured against in another space, got: %v", err) + } +} + func TestBoundProjectedOutputRejectsIrreducibleMetadata(t *testing.T) { saveAndResetGlobals(t) flagOutputFormat = "json" @@ -73,7 +115,7 @@ func TestBoundProjectedOutputRejectsIrreducibleMetadata(t *testing.T) { if err == nil { t.Fatal("irreducible output = nil error, want the overflow refusal") } - if !strings.Contains(err.Error(), "exceeds the 512-byte limit") || !strings.Contains(err.Error(), "counts") { + if !strings.Contains(err.Error(), "cannot be reduced to fit under the 512-byte structured-output limit") || !strings.Contains(err.Error(), "counts") { t.Fatalf("irreducible output error = %v, want the byte budget and the largest field", err) } // The byte-bounder's own message is deliberately flag-neutral: it knows diff --git a/internal/cli/gen_support.go b/internal/cli/gen_support.go index 7ca3dab..91e1e59 100644 --- a/internal/cli/gen_support.go +++ b/internal/cli/gen_support.go @@ -380,7 +380,7 @@ func printBoundedGenericResult(ctx *RunContext, data any) error { if !ok { return ctx.Printer.Print(data, nil) } - bounded, bound, err := boundProjectedList(rows, compactListOutputLimit) + bounded, bound, err := boundProjectedList(rows, compactListOutputLimit, 0) if err != nil { return explainProjectionOverflow(ctx.Cmd, err) } @@ -399,30 +399,36 @@ func printBoundedGenericResult(ctx *RunContext, data any) error { if !ok { return ctx.Printer.Print(data, nil) } - // boundProjectedList sizes the rows standalone, but printed inside the - // envelope they share the budget with the pagination siblings (and, in - // indented JSON, sit one indent level deeper). Fit against the full - // limit, then re-fit with the observed envelope overhead subtracted - // until the whole payload is under it. - budget := compactListOutputLimit + // The rows are first sized against the whole limit, but printed inside + // the envelope they share the budget with the pagination siblings (and, + // in indented JSON, sit one indent level deeper). Each re-fit sizes + // them against the limit minus the framing seen so far, growing that + // framing by each observed overflow until the whole payload fits. + framing := 0 for { - bounded, bound, err := boundProjectedList(rows, budget) + bounded, bound, err := boundProjectedList(rows, compactListOutputLimit, framing) if err != nil { return explainProjectionOverflow(ctx.Cmd, err) } value[key] = bounded + // In-payload, not just on stderr: scripts discard stderr, and + // the pagination siblings keep describing the server page, so a + // reduced page would otherwise read as complete. emitted_rows + // is set only when rows were withheld (a prefix was dropped), + // which is the case paging can repair; when instead long values + // were clipped, truncated rides alone — re-requesting cannot + // restore them, narrowing --fields can. Stamp both keys from this + // pass alone, so a key left over from an earlier re-fit cannot + // describe a reduction this payload does not carry. if bound.reduced() { - // In-payload, not just on stderr: scripts discard stderr, and - // the pagination siblings keep describing the server page, so a - // reduced page would otherwise read as complete. emitted_rows - // is set only when rows were WITHHELD (a prefix was dropped), - // which is the case paging can repair; when instead long values - // were clipped, truncated rides alone — re-requesting cannot - // restore them, narrowing --fields can. value["truncated"] = true - if len(bounded) < len(rows) { - value["emitted_rows"] = len(bounded) - } + } else { + delete(value, "truncated") + } + if len(bounded) < len(rows) { + value["emitted_rows"] = len(bounded) + } else { + delete(value, "emitted_rows") } out, err := marshalStructured(value) if err != nil { @@ -432,7 +438,7 @@ func printBoundedGenericResult(ctx *RunContext, data any) error { noteProjectionBound(ctx.Cmd, bound) return ctx.Printer.Print(value, nil) } - budget -= len(out) + 2 - compactListOutputLimit + framing += len(out) + 2 - compactListOutputLimit } default: return ctx.Printer.Print(data, nil) diff --git a/internal/cli/gen_support_test.go b/internal/cli/gen_support_test.go index 96d0b20..a1a26d4 100644 --- a/internal/cli/gen_support_test.go +++ b/internal/cli/gen_support_test.go @@ -314,3 +314,88 @@ func TestPrintGenericResultCompleteEnvelopeUnmarked(t *testing.T) { t.Errorf("within-budget envelope must not announce a reduction, got:\n%s", stderrText) } } + +// TestBoundedEnvelopeReductionFactsMatchPayload pins the reduction contract a +// --json consumer walks: emitted_rows appears only when rows were WITHHELD +// with every emitted value intact (a prefix reduction paging can repair), and a +// clipped value (it ends in "...") never rides under that marker, because +// paging cannot restore a clipped value — only a narrower --fields can. The +// fixture sweeps the first row across the band where the envelope re-fit +// switches between the two reductions, since that is where the reported facts +// and the printed payload can drift apart. +func TestBoundedEnvelopeReductionFactsMatchPayload(t *testing.T) { + for _, titleBytes := range []int{15000, 15200, 15400, 15600, 15800, 16000, 16200, 16400, 16600, 16800} { + t.Run(fmt.Sprintf("title=%d", titleBytes), func(t *testing.T) { + saveAndResetGlobals(t) + stub := newGFStub(t) + stub.data = map[string]any{ + "items": []any{ + map[string]any{"incident_id": "inc-1", "title": strings.Repeat("x", titleBytes), "severity": "Critical"}, + map[string]any{"incident_id": "inc-2", "title": "small follower", "severity": "Info"}, + }, + "total": 2, + "has_next_page": true, + } + + out, stderrText, err := execCommandSplit("insight", "incident-list", + "--start-time", "7d", "--end-time", "now", "--output-format", "json") + if err != nil { + t.Fatalf("execCommandSplit: %v", err) + } + if len([]byte(out)) >= compactListOutputLimit { + t.Errorf("bounded envelope is %d bytes, want <%d", len([]byte(out)), compactListOutputLimit) + } + var envelope map[string]any + if err := json.Unmarshal([]byte(strings.TrimSpace(out)), &envelope); err != nil { + t.Fatalf("bounded output is not a JSON object: %v\n%s", err, out) + } + items, ok := envelope["items"].([]any) + if !ok { + t.Fatalf("bounded output lost the items array: %v", envelope) + } + _, rowsWithheld := envelope["emitted_rows"] + clippedAt := "" + for _, item := range items { + row, ok := item.(map[string]any) + if !ok { + t.Fatalf("emitted row is not an object: %v", item) + } + for key, value := range row { + if text, ok := value.(string); ok && strings.HasSuffix(text, "...") { + clippedAt = fmt.Sprintf("%s (%d bytes emitted)", key, len(text)) + } + } + } + mode := fmt.Sprintf("items=%d emitted_rows=%v clipped=%q", len(items), envelope["emitted_rows"], clippedAt) + clipped := clippedAt != "" + t.Log(mode) + + if envelope["truncated"] != true { + t.Errorf("reduced envelope must carry truncated: %s", mode) + } + if !rowsWithheld && !clipped { + t.Errorf("a marked payload was reduced one way or the other: %s", mode) + } + if rowsWithheld && clipped { + t.Errorf("emitted_rows tells a paging walk the withheld rows are recoverable, but the payload also carries a clipped value that paging cannot restore: %s", mode) + } + if clipped { + if strings.Contains(stderrText, "every value intact") { + t.Errorf("a clipped value cannot be announced as \"every value intact\": %s\ngot: %s", mode, stderrText) + } + if !strings.Contains(stderrText, "were shortened to fit") { + t.Errorf("a clipped value should be announced as shortened on stderr: %s\ngot: %s", mode, stderrText) + } + // The re-fit sizes the rows against the limit minus the + // envelope's framing, but the note names the published cap: + // an internal remainder tells the reader nothing they can act + // on, and it contradicts the documented limit. + if published := fmt.Sprintf("the %d-byte limit", compactListOutputLimit); !strings.Contains(stderrText, published) { + t.Errorf("the shortened note should name the published limit (%s): %s\ngot: %s", published, mode, stderrText) + } + } else if !strings.Contains(stderrText, "every value intact") { + t.Errorf("a rows-withheld reduction should announce intact values on stderr: %s\ngot: %s", mode, stderrText) + } + }) + } +}