From 5755b75d5b1cdb180c6f50e2cb857d5888bfd12b Mon Sep 17 00:00:00 2001 From: ysyneu Date: Wed, 9 Sep 2026 23:30:32 -0700 Subject: [PATCH 1/3] fix(cli): scope projection-limit advice to the command's own flags A reduced structured list announces the reduction on stderr with advice to narrow --fields or lower --limit, but those flags are registered per verb, not globally. The note was composed inside the byte-bounding helper, which knows bytes and nothing about the running command, so a generated verb that declares neither flag (e.g. channel silence-rule-list, whose only flags are --channel-id and --data) was told to pass a flag it rejects (unknown flag: --limit), costing a round trip that cannot be obeyed. boundProjectedList now returns a projectionBound describing what it removed (rows kept/total, or values shortened and in which fields) instead of prose. noteProjectionBound composes the note against cmd's real flag set, naming only the flags the verb declares and saying plainly when it declares none. A --fields-only verb (channel escalate-rule-list) is no longer told to lower --limit, and a --limit-only verb (insight incident-list, monit rule-list-basic) is no longer told to narrow --fields. Tests cover a verb with --fields and --limit (incident list, alert-event list), one with --fields only, one with neither, and the --limit-only generated verbs, asserting the reduction is still announced with every emitted value intact. --- internal/cli/alert_event.go | 4 +- internal/cli/channel.go | 4 +- internal/cli/fieldproject.go | 104 ++++++++++++++++++++-------- internal/cli/fieldproject_test.go | 109 +++++++++++++++++++++++++++--- internal/cli/gen_support.go | 8 +-- internal/cli/gen_support_test.go | 16 +++++ internal/cli/incident.go | 12 ++-- internal/cli/insight.go | 4 +- 8 files changed, 207 insertions(+), 54 deletions(-) diff --git a/internal/cli/alert_event.go b/internal/cli/alert_event.go index a707ba5..19d0dbe 100644 --- a/internal/cli/alert_event.go +++ b/internal/cli/alert_event.go @@ -97,12 +97,12 @@ func newAlertEventListCmd() *cobra.Command { if err != nil { return err } - bounded, note, err := boundProjectedOutput(proj, compactListOutputLimit) + bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { return err } proj = bounded.([]map[string]any) - noteProjectionBound(cmd.ErrOrStderr(), note) + noteProjectionBound(cmd, bound) effectiveLimit := limit if len(proj) < len(result.Items) { effectiveLimit = len(proj) diff --git a/internal/cli/channel.go b/internal/cli/channel.go index 6d56e08..3f25869 100644 --- a/internal/cli/channel.go +++ b/internal/cli/channel.go @@ -195,12 +195,12 @@ func newChannelEscalateRuleListCmd() *cobra.Command { if err != nil { return err } - bounded, note, err := boundProjectedOutput(proj, compactListOutputLimit) + bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { return err } proj = bounded.([]map[string]any) - noteProjectionBound(cmd.ErrOrStderr(), note) + noteProjectionBound(cmd, bound) return ctx.PrintTotal(proj, nil, len(proj)) } diff --git a/internal/cli/fieldproject.go b/internal/cli/fieldproject.go index 44a86e7..3bf3c4e 100644 --- a/internal/cli/fieldproject.go +++ b/internal/cli/fieldproject.go @@ -7,6 +7,8 @@ import ( "sort" "strings" "unicode/utf8" + + "github.com/spf13/cobra" ) const ( @@ -80,16 +82,64 @@ func noteDefaultProjection(w io.Writer, fields []string) { strings.Join(fields, ",")) } -// noteProjectionBound relays a boundProjectedOutput note to the caller on -// stderr. Without it a reduced page or a shortened value is only visible to +// projectionBound reports how boundProjectedList reduced an over-budget +// projection — the facts, not the prose. The byte-bounding helper knows bytes +// and nothing about the command it runs under, so it cannot name the flags a +// caller should narrow; the caller that owns the command turns this into the +// stderr note via noteProjectionBound. A prefix reduction and a value +// shortening are mutually exclusive, and the zero value means nothing was +// reduced. +type projectionBound struct { + // A prefix reduction kept rowsEmitted of rowsTotal rows, every value intact. + rowsEmitted int + rowsTotal int + // A value shortening clipped shortened of valuesTotal string values, in fields. + shortened int + valuesTotal int + fields []string + // maxBytes is the budget the reduction was sized against. + maxBytes int +} + +// noteProjectionBound announces on stderr how an over-budget projection was +// reduced. Without it a reduced page or a shortened value is only visible to // a reader, not to the jq filter or exact match a --json consumer runs over // it, so a query that silently matches nothing looks like an empty result // rather than a bounded one. -func noteProjectionBound(w io.Writer, note string) { - if note == "" { - return +// +// The advice is composed HERE, against cmd's real flag set, because --fields +// and --limit are registered per verb: a byte-bounding helper composing the +// sentence in ignorance of the command would tell a verb that has neither to +// "lower --limit", costing the caller a round trip on a flag it rejects +// (unknown flag: --limit). +func noteProjectionBound(cmd *cobra.Command, bound projectionBound) { + w := cmd.ErrOrStderr() + switch { + case bound.rowsTotal > 0: + _, _ = fmt.Fprintf(w, "note: emitted %d of %d projected rows (every value intact) to stay below the %d-byte structured-output limit; %s — the rows past the first %d were not emitted\n", + bound.rowsEmitted, bound.rowsTotal, bound.maxBytes, narrowingAdvice(cmd, "to fit more rows per page"), bound.rowsEmitted) + case bound.shortened > 0: + _, _ = fmt.Fprintf(w, "note: %d of %d string values were shortened to fit the %d-byte limit and now end with \"...\" (fields: %s); matching or filtering on those fields will miss — %s\n", + bound.shortened, bound.valuesTotal, bound.maxBytes, strings.Join(bound.fields, ", "), narrowingAdvice(cmd, "for untruncated values")) + } +} + +// narrowingAdvice names the flags cmd actually declares that can shrink an +// over-budget projection, suffixed with what they buy (purpose). A command +// that declares neither says so instead of naming a flag it would reject. +func narrowingAdvice(cmd *cobra.Command, purpose string) string { + fields := cmd.Flags().Lookup("fields") != nil + limit := cmd.Flags().Lookup("limit") != nil + switch { + case fields && limit: + return "narrow --fields or lower --limit " + purpose + case fields: + return "narrow --fields " + purpose + case limit: + return "lower --limit " + purpose + default: + return "this command declares no --fields or --limit flag to narrow the output" } - _, _ = fmt.Fprintln(w, note) } // boundProjectedOutput keeps the new agent-oriented projections below their @@ -104,18 +154,18 @@ func noteProjectionBound(w io.Writer, note string) { // error instead. // // It returns the bounded data with the same type it was given, plus a -// caller-printable note (empty when nothing was reduced or shortened), so -// the caller can announce the loss on stderr — the "..." marker is only -// visible to something that reads the value, never to the filter a --json -// consumer runs over it. -func boundProjectedOutput(data any, maxBytes int) (any, string, error) { +// projectionBound describing what was reduced (its zero value when nothing +// was), so the caller can announce the loss on stderr via noteProjectionBound +// — the "..." marker is only visible to something that reads the value, never +// to the filter a --json consumer runs over it. +func boundProjectedOutput(data any, maxBytes int) (any, projectionBound, error) { switch value := data.(type) { case map[string]any: - return value, "", boundProjectedDetail(value, maxBytes) + return value, projectionBound{}, boundProjectedDetail(value, maxBytes) case []map[string]any: return boundProjectedList(value, maxBytes) default: - return nil, "", fmt.Errorf("internal error: unsupported projected output %T", data) + return nil, projectionBound{}, fmt.Errorf("internal error: unsupported projected output %T", data) } } @@ -210,34 +260,33 @@ func isIdentifierField(key string) bool { // from a genuinely short one; if no cap at or above that floor fits, the // 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 note. -func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, string, error) { +// returned projectionBound. +func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, projectionBound, error) { encoded, err := marshalStructured(rows) if err != nil { - return nil, "", err + return nil, projectionBound{}, err } if len(encoded)+1 < maxBytes { - return rows, "", nil + return rows, projectionBound{}, nil } // The overflow error names the fields responsible, exactly as the detail // path does, so the request can be narrowed in one pass. - tooBig := func() ([]map[string]any, string, error) { + tooBig := func() ([]map[string]any, projectionBound, error) { largest, err := largestProjectedFields(rows) if err != nil { - return nil, "", err + return nil, projectionBound{}, err } - return nil, "", fmt.Errorf("projected list is %d bytes across %d rows, exceeds the %d-byte limit; largest fields: %s; request fewer rows (--limit) or fewer --fields", + return nil, projectionBound{}, fmt.Errorf("projected list is %d bytes across %d rows, exceeds the %d-byte limit; largest fields: %s; request fewer rows (--limit) or fewer --fields", len(encoded), len(rows), maxBytes, largest) } kept, err := largestFittingPrefix(rows, maxBytes) if err != nil { - return nil, "", err + return nil, projectionBound{}, err } if kept > 0 { - return rows[:kept], fmt.Sprintf("note: emitted %d of %d projected rows (every value intact) to stay below the %d-byte structured-output limit; narrow --fields or lower --limit to fit more rows per page — the rows past the first %d were not emitted", - kept, len(rows), maxBytes, kept), nil + return rows[:kept], projectionBound{rowsEmitted: kept, rowsTotal: len(rows), maxBytes: maxBytes}, nil } maxLen := 0 @@ -285,7 +334,7 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, return tooBig() } if ok, err := fits(minMarkedTruncationCap); err != nil { - return nil, "", err + return nil, projectionBound{}, err } else if !ok { return tooBig() } @@ -299,7 +348,7 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, mid := lo + (hi-lo+1)/2 ok, err := fits(mid) if err != nil { - return nil, "", err + return nil, projectionBound{}, err } if ok { lo = mid @@ -329,15 +378,14 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, } } if shortened == 0 { - return rows, "", nil + return rows, projectionBound{}, nil } names := make([]string, 0, len(fields)) for name := range fields { names = append(names, name) } sort.Strings(names) - return rows, fmt.Sprintf("note: %d of %d string values were shortened to fit the %d-byte limit and now end with \"...\" (fields: %s); matching or filtering on those fields will miss — narrow --fields or --limit for untruncated values", - shortened, total, maxBytes, strings.Join(names, ", ")), nil + return rows, projectionBound{shortened: shortened, valuesTotal: total, fields: names, maxBytes: maxBytes}, 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 610ca70..14ec03f 100644 --- a/internal/cli/fieldproject_test.go +++ b/internal/cli/fieldproject_test.go @@ -13,6 +13,22 @@ import ( toon "github.com/toon-format/toon-go" ) +// projectionNote renders noteProjectionBound against a synthetic command that +// declares exactly the named flags, so a unit test can assert the composed note +// text without standing up a whole verb. The flag sets real verbs register are +// covered by the end-to-end tests. +func projectionNote(t *testing.T, bound projectionBound, flags ...string) string { + t.Helper() + cmd := &cobra.Command{} + for _, name := range flags { + cmd.Flags().String(name, "", "") + } + var buf bytes.Buffer + cmd.SetErr(&buf) + noteProjectionBound(cmd, bound) + return buf.String() +} + // incidentRow / alertRow are multi-field stub payloads with the nested blobs // (responders/labels/alerts, events/incident/labels) that bloat the full dump. // The SDK structs carry no `omitempty`, so the full toon/json marshal always @@ -941,7 +957,7 @@ func TestBoundProjectedListNeverEmitsUnmarkedTruncation(t *testing.T) { originals[i] = clone } - bounded, note, err := boundProjectedOutput(rows, compactListOutputLimit) + bounded, bound, err := boundProjectedOutput(rows, compactListOutputLimit) if err != nil { t.Fatalf("bound: %v", err) } @@ -979,10 +995,10 @@ func TestBoundProjectedListNeverEmitsUnmarkedTruncation(t *testing.T) { if len(encoded)+1 >= compactListOutputLimit { t.Fatalf("bounded output is %d bytes, want <%d", len(encoded)+1, compactListOutputLimit) } - wantNote := fmt.Sprintf("note: emitted %d of %d projected rows (every value intact) to stay below the %d-byte structured-output limit; narrow --fields or lower --limit to fit more rows per page — the rows past the first %d were not emitted", + wantNote := fmt.Sprintf("note: emitted %d of %d projected rows (every value intact) to stay below the %d-byte structured-output limit; narrow --fields or lower --limit to fit more rows per page — the rows past the first %d were not emitted\n", len(kept), len(rows), compactListOutputLimit, len(kept)) - if note != wantNote { - t.Fatalf("note = %q, want %q", note, wantNote) + if got := projectionNote(t, bound, "fields", "limit"); got != wantNote { + t.Fatalf("note = %q, want %q", got, wantNote) } } @@ -1031,10 +1047,11 @@ func TestBoundProjectedListAnnouncesShortening(t *testing.T) { "title": strings.Repeat("payment-gateway timeout ", 200), }} - _, note, err := boundProjectedOutput(rows, 512) + _, bound, err := boundProjectedOutput(rows, 512) if err != nil { t.Fatalf("bound projected output: %v", err) } + note := projectionNote(t, bound, "fields", "limit") if note == "" { t.Fatalf("shortened projection returned no note; caller cannot tell values were clipped") } @@ -1052,11 +1069,11 @@ func TestBoundProjectedListNoNoteWhenNothingShortened(t *testing.T) { flagOutputFormat = "json" rows := []map[string]any{{"incident_id": "inc-1", "title": "disk full"}} - _, note, err := boundProjectedOutput(rows, 512) + _, bound, err := boundProjectedOutput(rows, 512) if err != nil { t.Fatalf("bound projected output: %v", err) } - if note != "" { + if note := projectionNote(t, bound, "fields", "limit"); note != "" { t.Fatalf("fitting projection returned note %q, want none", note) } } @@ -1259,6 +1276,77 @@ func TestChannelEscalateRuleListStructuredProjection(t *testing.T) { }) } +// TestReducedPageNoteNamesOnlyDeclaredFlags pins that the reduced-page note +// advises only the flags the running verb actually carries: --fields/--limit +// are registered per verb, so a note composed without the command's flag set +// sends the caller to a flag the verb rejects (unknown flag: --limit), which +// costs a wasted round trip for advice that cannot be obeyed. +func TestReducedPageNoteNamesOnlyDeclaredFlags(t *testing.T) { + // channel escalate-rule-list declares --fields but neither --limit nor + // --page: the advice may name --fields alone. + t.Run("fields without limit", func(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + stub := newGFStub(t) + items := make([]any, 60) + for i := range items { + items[i] = map[string]any{ + "rule_id": fmt.Sprintf("%024x", i), + "rule_name": strings.Repeat(fmt.Sprintf("rule %d ", i), 40), + } + } + stub.data = map[string]any{"items": items} + + _, stderrText, err := execCommandSplit("channel", "escalate-rule-list", "4201", + "--fields", "rule_id,rule_name", "--output-format", "json") + if err != nil { + t.Fatalf("execCommandSplit: %v", err) + } + if !strings.Contains(stderrText, "note: emitted") { + t.Fatalf("over-budget page should announce the reduction, got:\n%s", stderrText) + } + if !strings.Contains(stderrText, "narrow --fields to fit more rows per page") { + t.Errorf("advice should name --fields, the flag this verb declares, got:\n%s", stderrText) + } + if strings.Contains(stderrText, "--limit") { + t.Errorf("advice must not name --limit; this verb declares no such flag, got:\n%s", stderrText) + } + }) + + // channel silence-rule-list is a generated verb that declares neither + // --fields nor --limit: the note must not invent one. + t.Run("neither flag", func(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + stub := newGFStub(t) + items := make([]any, 60) + for i := range items { + items[i] = map[string]any{ + "rule_id": fmt.Sprintf("%024x", i), + "rule_name": fmt.Sprintf("silence rule %d", i), + "description": strings.Repeat(fmt.Sprintf("row %d noise-suppression detail ", i), 40), + } + } + stub.data = map[string]any{"items": items} + + _, stderrText, err := execCommandSplit("channel", "silence-rule-list", "4201", "--output-format", "json") + if err != nil { + t.Fatalf("execCommandSplit: %v", err) + } + if !strings.Contains(stderrText, "note: emitted") { + t.Fatalf("over-budget page should announce the reduction, got:\n%s", stderrText) + } + if !strings.Contains(stderrText, "declares no --fields or --limit flag") { + t.Errorf("advice should say this verb has no narrowing flag, got:\n%s", stderrText) + } + for _, flag := range []string{"--fields", "--limit"} { + if strings.Contains(stderrText, "narrow "+flag) || strings.Contains(stderrText, "lower "+flag) { + t.Errorf("advice must not tell the caller to %s on a verb that rejects it, got:\n%s", flag, stderrText) + } + } + }) +} + // TestBoundProjectedListNeverShortensIdentifierFields pins the identifier // exemption: keys ending in _id/_key carry values a consumer matches, // filters, or passes back verbatim (a jq exact-match over --json output, a @@ -1282,7 +1370,7 @@ func TestBoundProjectedListNeverShortensIdentifierFields(t *testing.T) { }} const budget = 1400 - bounded, note, err := boundProjectedOutput(rows, budget) + bounded, bound, err := boundProjectedOutput(rows, budget) if err != nil { t.Fatalf("bound projected output: %v", err) } @@ -1301,6 +1389,7 @@ func TestBoundProjectedListNeverShortensIdentifierFields(t *testing.T) { t.Errorf("title should be shortened with the \"...\" marker, got %q", title) } + note := projectionNote(t, bound, "fields", "limit") if note == "" { t.Fatal("shortened projection returned no note; caller cannot tell values were clipped") } @@ -1340,7 +1429,7 @@ func TestBoundProjectedListIdentifierOnlyOverflowReducesPage(t *testing.T) { originals[i] = row["incident_id"].(string) } - bounded, note, err := boundProjectedOutput(rows, 512) + bounded, bound, err := boundProjectedOutput(rows, 512) if err != nil { t.Fatalf("identifier-only overflow should reduce the page, not error: %v", err) } @@ -1357,7 +1446,7 @@ func TestBoundProjectedListIdentifierOnlyOverflowReducesPage(t *testing.T) { } } wantNote := fmt.Sprintf("emitted %d of %d", len(kept), len(rows)) - if !strings.Contains(note, wantNote) { + if note := projectionNote(t, bound, "fields", "limit"); !strings.Contains(note, wantNote) { t.Fatalf("note = %q, want it to name the emitted count (%q)", note, wantNote) } encoded, err := marshalStructured(kept) diff --git a/internal/cli/gen_support.go b/internal/cli/gen_support.go index 0c83d62..5eeda82 100644 --- a/internal/cli/gen_support.go +++ b/internal/cli/gen_support.go @@ -378,11 +378,11 @@ func printBoundedGenericResult(ctx *RunContext, data any) error { if !ok { return ctx.Printer.Print(data, nil) } - bounded, note, err := boundProjectedList(rows, compactListOutputLimit) + bounded, bound, err := boundProjectedList(rows, compactListOutputLimit) if err != nil { return err } - noteProjectionBound(ctx.Cmd.ErrOrStderr(), note) + noteProjectionBound(ctx.Cmd, bound) return ctx.Printer.Print(bounded, nil) case map[string]any: key, ok := listEnvelopeKey(value) @@ -402,7 +402,7 @@ func printBoundedGenericResult(ctx *RunContext, data any) error { // until the whole payload is under it. budget := compactListOutputLimit for { - bounded, note, err := boundProjectedList(rows, budget) + bounded, bound, err := boundProjectedList(rows, budget) if err != nil { return err } @@ -412,7 +412,7 @@ func printBoundedGenericResult(ctx *RunContext, data any) error { return err } if len(out)+1 < compactListOutputLimit { - noteProjectionBound(ctx.Cmd.ErrOrStderr(), note) + noteProjectionBound(ctx.Cmd, bound) return ctx.Printer.Print(value, nil) } budget -= len(out) + 2 - compactListOutputLimit diff --git a/internal/cli/gen_support_test.go b/internal/cli/gen_support_test.go index 6ba00bf..e344d58 100644 --- a/internal/cli/gen_support_test.go +++ b/internal/cli/gen_support_test.go @@ -76,6 +76,14 @@ func TestPrintGenericResultBoundsListEnvelope(t *testing.T) { if !strings.Contains(stderrText, "note: emitted") { t.Errorf("reduced %s page should announce itself on stderr, got:\n%s", format, stderrText) } + // insight incident-list declares --limit but no --fields: the advice + // may name only the flag the verb actually carries. + if !strings.Contains(stderrText, "lower --limit") { + t.Errorf("advice should name --limit, the flag this verb declares, got:\n%s", stderrText) + } + if strings.Contains(stderrText, "--fields") { + t.Errorf("advice must not name --fields; this verb declares no such flag, got:\n%s", stderrText) + } // The first row survives intact; the last was dropped by the // prefix reduction. if !strings.Contains(out, ids[0]) { @@ -132,6 +140,14 @@ func TestPrintGenericResultBoundsTopLevelArray(t *testing.T) { if !strings.Contains(stderrText, "note: emitted") { t.Errorf("reduced page should announce itself on stderr, got:\n%s", stderrText) } + // monit rule-list-basic declares --limit but no --fields: the advice may + // name only the flag the verb actually carries. + if !strings.Contains(stderrText, "lower --limit") { + t.Errorf("advice should name --limit, the flag this verb declares, got:\n%s", stderrText) + } + if strings.Contains(stderrText, "--fields") { + t.Errorf("advice must not name --fields; this verb declares no such flag, got:\n%s", stderrText) + } var decoded []map[string]any if err := json.Unmarshal([]byte(strings.TrimSpace(out)), &decoded); err != nil { t.Fatalf("bounded top-level array is not a JSON array: %v\n%s", err, out) diff --git a/internal/cli/incident.go b/internal/cli/incident.go index a4dceb3..298ce6e 100644 --- a/internal/cli/incident.go +++ b/internal/cli/incident.go @@ -137,12 +137,12 @@ func newIncidentListCmd() *cobra.Command { if err != nil { return err } - bounded, note, err := boundProjectedOutput(proj, compactListOutputLimit) + bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { return err } proj = bounded.([]map[string]any) - noteProjectionBound(cmd.ErrOrStderr(), note) + noteProjectionBound(cmd, bound) effectiveLimit := limit if len(proj) < len(result.Items) { effectiveLimit = len(proj) @@ -638,12 +638,12 @@ func newIncidentSimilarCmd() *cobra.Command { if err != nil { return err } - bounded, note, err := boundProjectedOutput(proj, compactListOutputLimit) + bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { return err } proj = bounded.([]map[string]any) - noteProjectionBound(cmd.ErrOrStderr(), note) + noteProjectionBound(cmd, bound) return ctx.Printer.Print(proj, nil) } @@ -1613,11 +1613,11 @@ func newIncidentDetailCmd() *cobra.Command { if err != nil { return err } - _, note, err := boundProjectedOutput(proj[0], compactDetailOutputLimit) + _, bound, err := boundProjectedOutput(proj[0], compactDetailOutputLimit) if err != nil { return err } - noteProjectionBound(cmd.ErrOrStderr(), note) + noteProjectionBound(cmd, bound) return ctx.Printer.Print(proj[0], nil) } return ctx.Printer.Print(result, nil) diff --git a/internal/cli/insight.go b/internal/cli/insight.go index d404df8..a4c5a1a 100644 --- a/internal/cli/insight.go +++ b/internal/cli/insight.go @@ -129,12 +129,12 @@ func newInsightIncidentsCmd() *cobra.Command { if err != nil { return err } - bounded, note, err := boundProjectedOutput(proj, compactListOutputLimit) + bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { return err } proj = bounded.([]map[string]any) - noteProjectionBound(cmd.ErrOrStderr(), note) + noteProjectionBound(cmd, bound) effectiveLimit := limit if len(proj) < len(result.Items) { effectiveLimit = len(proj) From 2add3574c74fc95b555b54f99e32de96b8f7ff79 Mon Sep 17 00:00:00 2001 From: ysyneu Date: Wed, 9 Sep 2026 23:54:47 -0700 Subject: [PATCH 2/3] fix(cli): name projection flags only where the command declares them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reduction note and the overflow error told the caller to narrow --fields or lower --limit whenever those flags existed, but a flag's name does not imply its effect: monit rule-update-fields spells a request-body write selector --fields, so obeying the advice would change a write instead of the output; safari knowledge-file-list documents its --limit as ignored; monit rule-list-basic honors its own only alongside --include-descendants. Commands now declare, in their own definition, the flags that narrow their structured output — a projection flag that shrinks each row, and a rows flag that requests fewer rows (declareOutputNarrowing, set next to the flag registrations). The hand-written projection verbs declare theirs; generated verbs declare none, because the spec has no structured contract for whether a --limit bounds the response, so their note names no flag at all. The byte-bounding helper composes no flag advice: it returns facts, and an irreducible overflow comes back as a flag-neutral error the caller completes (explainProjectionOverflow). The single-row shortening branch no longer offers a rows flag, since requesting fewer rows cannot shrink the one row that already overflows. Tests drive real verbs end to end: a write-selector --fields gets no --fields advice, an ignored or conditional --limit gets no --limit advice, the shortening note offers only the projection flag, and the overflow error names only flags the command declared. --- internal/cli/alert_event.go | 3 +- internal/cli/channel.go | 3 +- internal/cli/fieldproject.go | 155 ++++++++++---- internal/cli/fieldproject_test.go | 324 +++++++++++++++++++++++------- internal/cli/gen_support.go | 4 +- internal/cli/gen_support_test.go | 27 +-- internal/cli/incident.go | 9 +- internal/cli/insight.go | 3 +- internal/cmd/cligen/main.go | 7 + 9 files changed, 406 insertions(+), 129 deletions(-) diff --git a/internal/cli/alert_event.go b/internal/cli/alert_event.go index 19d0dbe..7abb232 100644 --- a/internal/cli/alert_event.go +++ b/internal/cli/alert_event.go @@ -99,7 +99,7 @@ func newAlertEventListCmd() *cobra.Command { } bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { - return err + return explainProjectionOverflow(cmd, err) } proj = bounded.([]map[string]any) noteProjectionBound(cmd, bound) @@ -125,6 +125,7 @@ func newAlertEventListCmd() *cobra.Command { cmd.Flags().IntVar(&limit, "limit", 20, "Max results (max 100)") cmd.Flags().IntVar(&page, "page", 1, "Page number") cmd.Flags().StringVar(&fields, "fields", "", "Comma-separated fields to project in json/toon output (e.g. event_id,alert_id,event_severity,event_status,event_time,title); ignored in table mode. Defaults to these compact event fields. If the page would exceed 16 KiB, only the leading rows that fit are emitted, with every value intact (announced on stderr).") + declareOutputNarrowing(cmd, "fields", "limit") return cmd } diff --git a/internal/cli/channel.go b/internal/cli/channel.go index 3f25869..0355d15 100644 --- a/internal/cli/channel.go +++ b/internal/cli/channel.go @@ -197,7 +197,7 @@ func newChannelEscalateRuleListCmd() *cobra.Command { } bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { - return err + return explainProjectionOverflow(cmd, err) } proj = bounded.([]map[string]any) noteProjectionBound(cmd, bound) @@ -218,5 +218,6 @@ func newChannelEscalateRuleListCmd() *cobra.Command { cmd.Flags().Int64Var(&fChannelID, "channel-id", 0, "Channel to list rules for. (required)") cmd.Flags().StringVar(&dataJSON, "data", "", "Full request body as JSON; positional arguments and typed flags override its fields. Accepts inline JSON, or - to read stdin.") cmd.Flags().StringVar(&fields, "fields", "", "Comma-separated fields to project in json/toon output (e.g. rule_id,rule_name,status,priority); ignored in table mode. Use to avoid dumping the full nested record.") + declareOutputNarrowing(cmd, "fields", "") return cmd } diff --git a/internal/cli/fieldproject.go b/internal/cli/fieldproject.go index 3bf3c4e..eef53bf 100644 --- a/internal/cli/fieldproject.go +++ b/internal/cli/fieldproject.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "fmt" "io" "reflect" @@ -101,45 +102,133 @@ type projectionBound struct { maxBytes int } +// Commands declare here which of their flags narrow structured output, so the +// projection note and overflow error can name them. Nothing infers this from a +// flag's name: a verb may spell a request-body write selector --fields, or +// accept a --limit the server ignores. Only a command whose own definition +// states what its flags do sets these, via declareOutputNarrowing. +const ( + narrowsByProjection = "narrows-output-projection" + narrowsByRows = "narrows-output-rows" +) + +// declareOutputNarrowing records, on cmd, the flags its own definition declares +// as narrowing its structured output. projectionFlag reduces how much each row +// carries (e.g. --fields); rowsFlag reduces how many rows are requested (e.g. +// --limit). Pass "" for a control the command does not expose. +func declareOutputNarrowing(cmd *cobra.Command, projectionFlag, rowsFlag string) { + if cmd.Annotations == nil { + cmd.Annotations = map[string]string{} + } + if projectionFlag != "" { + cmd.Annotations[narrowsByProjection] = projectionFlag + } + if rowsFlag != "" { + cmd.Annotations[narrowsByRows] = rowsFlag + } +} + +// declaredNarrowing returns the flag cmd declared under annotation, or "" when +// it declared none. A declaration whose flag the command no longer carries (a +// renamed flag) is dropped, so the note can never name a flag that is not there. +func declaredNarrowing(cmd *cobra.Command, annotation string) string { + name := cmd.Annotations[annotation] + if name == "" || cmd.Flags().Lookup(name) == nil { + return "" + } + return name +} + +// projectionRemedy names the flags cmd declared as able to shrink an over-budget +// projection, suffixed with what they buy (purpose). rowsHelp is false for a +// single-row shortening, where a smaller page cannot shrink the one row that +// overflows, so a rows flag is not offered there. A command that declared no +// usable flag is told so rather than sent to one it may reject or misuse. +func projectionRemedy(cmd *cobra.Command, rowsHelp bool, purpose string) string { + projection := declaredNarrowing(cmd, narrowsByProjection) + rows := declaredNarrowing(cmd, narrowsByRows) + if !rowsHelp { + rows = "" + } + var offered []string + if projection != "" { + offered = append(offered, "narrow --"+projection) + } + if rows != "" { + offered = append(offered, "lower --"+rows) + } + if len(offered) == 0 { + return "this command declares no flag that narrows the output" + } + remedy := strings.Join(offered, " or ") + if purpose != "" { + remedy += " " + purpose + } + return remedy +} + // noteProjectionBound announces on stderr how an over-budget projection was -// reduced. Without it a reduced page or a shortened value is only visible to -// a reader, not to the jq filter or exact match a --json consumer runs over -// it, so a query that silently matches nothing looks like an empty result -// rather than a bounded one. +// reduced. Without it a reduced page or a shortened value is only visible to a +// reader, not to the jq filter or exact match a --json consumer runs over it, +// so a query that silently matches nothing looks like an empty result rather +// than a bounded one. // -// The advice is composed HERE, against cmd's real flag set, because --fields -// and --limit are registered per verb: a byte-bounding helper composing the -// sentence in ignorance of the command would tell a verb that has neither to -// "lower --limit", costing the caller a round trip on a flag it rejects -// (unknown flag: --limit). +// The remedy comes from cmd's own narrowing declaration, not from the +// byte-bounding helper: the helper knows bytes and nothing about the command, +// so a sentence composed there could name a flag the command rejects +// (unknown flag), ignores, or means for something else entirely. func noteProjectionBound(cmd *cobra.Command, bound projectionBound) { w := cmd.ErrOrStderr() switch { case bound.rowsTotal > 0: _, _ = fmt.Fprintf(w, "note: emitted %d of %d projected rows (every value intact) to stay below the %d-byte structured-output limit; %s — the rows past the first %d were not emitted\n", - bound.rowsEmitted, bound.rowsTotal, bound.maxBytes, narrowingAdvice(cmd, "to fit more rows per page"), bound.rowsEmitted) + bound.rowsEmitted, bound.rowsTotal, bound.maxBytes, projectionRemedy(cmd, true, "to fit more rows per page"), bound.rowsEmitted) case bound.shortened > 0: _, _ = fmt.Fprintf(w, "note: %d of %d string values were shortened to fit the %d-byte limit and now end with \"...\" (fields: %s); matching or filtering on those fields will miss — %s\n", - bound.shortened, bound.valuesTotal, bound.maxBytes, strings.Join(bound.fields, ", "), narrowingAdvice(cmd, "for untruncated values")) + bound.shortened, bound.valuesTotal, bound.maxBytes, strings.Join(bound.fields, ", "), projectionRemedy(cmd, false, "for untruncated values")) } } -// narrowingAdvice names the flags cmd actually declares that can shrink an -// over-budget projection, suffixed with what they buy (purpose). A command -// that declares neither says so instead of naming a flag it would reject. -func narrowingAdvice(cmd *cobra.Command, purpose string) string { - fields := cmd.Flags().Lookup("fields") != nil - limit := cmd.Flags().Lookup("limit") != nil - switch { - case fields && limit: - return "narrow --fields or lower --limit " + purpose - case fields: - return "narrow --fields " + purpose - case limit: - return "lower --limit " + purpose - default: - return "this command declares no --fields or --limit flag to narrow the output" +// projectionOverflow is the byte-bounding failure for a projection that no +// reduction can bring under the limit: no leading prefix fits and no row can be +// shortened (detail marks the single-object projection, which is refused rather +// than truncated). It carries the facts; its own message is flag-neutral, and +// callers turn it into the command-appropriate failure with +// explainProjectionOverflow. +type projectionOverflow struct { + detail bool + bytes int + rows int + 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", + 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) +} + +// explainProjectionOverflow returns err with the remedy for cmd appended when +// err is a projectionOverflow, so the failure names only flags cmd declared as +// output-narrowing; any other error passes through unchanged. Callers wrap each +// error returned from the bounding path with it. +func explainProjectionOverflow(cmd *cobra.Command, err error) error { + var overflow *projectionOverflow + if !errors.As(err, &overflow) { + return err + } + if overflow.detail { + projection := declaredNarrowing(cmd, narrowsByProjection) + if projection == "" { + return fmt.Errorf("%w; this command declares no flag that narrows the output", overflow) + } + return fmt.Errorf("%w; request fewer --%s, or omit --%s for the full, unbounded detail", overflow, projection, projection) } + return fmt.Errorf("%w; %s", overflow, projectionRemedy(cmd, true, "")) } // boundProjectedOutput keeps the new agent-oriented projections below their @@ -212,10 +301,10 @@ func largestProjectedFields(rows []map[string]any) (string, error) { return strings.Join(largest, ", "), nil } -// 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. +// boundProjectedDetail rejects an oversized single-object projection instead of +// truncating it, naming the largest fields so the request can be narrowed in one +// pass. Its message is flag-neutral; the caller appends the command-appropriate +// remedy via explainProjectionOverflow. func boundProjectedDetail(row map[string]any, maxBytes int) error { encoded, err := marshalStructured(row) if err != nil { @@ -229,8 +318,7 @@ func boundProjectedDetail(row map[string]any, maxBytes int) error { if err != nil { return err } - 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, largest) + return &projectionOverflow{detail: true, bytes: len(encoded), maxBytes: maxBytes, largest: largest} } // isIdentifierField reports whether a projected field is an identifier: @@ -277,8 +365,7 @@ func boundProjectedList(rows []map[string]any, maxBytes int) ([]map[string]any, if err != nil { return nil, projectionBound{}, err } - return nil, projectionBound{}, fmt.Errorf("projected list is %d bytes across %d rows, exceeds the %d-byte limit; largest fields: %s; request fewer rows (--limit) or fewer --fields", - len(encoded), len(rows), maxBytes, largest) + return nil, projectionBound{}, &projectionOverflow{bytes: len(encoded), rows: len(rows), maxBytes: maxBytes, largest: largest} } kept, err := largestFittingPrefix(rows, maxBytes) diff --git a/internal/cli/fieldproject_test.go b/internal/cli/fieldproject_test.go index 14ec03f..0cd1a67 100644 --- a/internal/cli/fieldproject_test.go +++ b/internal/cli/fieldproject_test.go @@ -13,22 +13,6 @@ import ( toon "github.com/toon-format/toon-go" ) -// projectionNote renders noteProjectionBound against a synthetic command that -// declares exactly the named flags, so a unit test can assert the composed note -// text without standing up a whole verb. The flag sets real verbs register are -// covered by the end-to-end tests. -func projectionNote(t *testing.T, bound projectionBound, flags ...string) string { - t.Helper() - cmd := &cobra.Command{} - for _, name := range flags { - cmd.Flags().String(name, "", "") - } - var buf bytes.Buffer - cmd.SetErr(&buf) - noteProjectionBound(cmd, bound) - return buf.String() -} - // incidentRow / alertRow are multi-field stub payloads with the nested blobs // (responders/labels/alerts, events/incident/labels) that bloat the full dump. // The SDK structs carry no `omitempty`, so the full toon/json marshal always @@ -86,8 +70,19 @@ func TestBoundProjectedOutputRejectsIrreducibleMetadata(t *testing.T) { rows := []map[string]any{{"counts": make([]int, 500)}} _, _, err := boundProjectedOutput(rows, 512) - if err == nil || !strings.Contains(err.Error(), "request fewer rows") { - t.Fatalf("irreducible output error = %v, want bounded guidance", err) + 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") { + 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 + // bytes, not the command, so a remedy can only be appended by a caller that + // owns the command (explainProjectionOverflow). + for _, flag := range []string{"--limit", "--fields"} { + if strings.Contains(err.Error(), flag) { + t.Fatalf("bounding error must not name a flag itself, got: %v", err) + } } } @@ -932,7 +927,7 @@ func TestAlertEventListFieldsProjectionUnchanged(t *testing.T) { // for the original defect's silent-corruption half: a page that overflows the // budget is reduced to the leading rows that fit, so no value is ever // shortened — every emitted row is byte-identical to the fixture, no "..." -// marker appears anywhere, the note names the emitted count, and the encoded +// marker appears anywhere, the report records the emitted count, and the encoded // output stays under the budget. func TestBoundProjectedListNeverEmitsUnmarkedTruncation(t *testing.T) { saveAndResetGlobals(t) @@ -995,10 +990,11 @@ func TestBoundProjectedListNeverEmitsUnmarkedTruncation(t *testing.T) { if len(encoded)+1 >= compactListOutputLimit { t.Fatalf("bounded output is %d bytes, want <%d", len(encoded)+1, compactListOutputLimit) } - wantNote := fmt.Sprintf("note: emitted %d of %d projected rows (every value intact) to stay below the %d-byte structured-output limit; narrow --fields or lower --limit to fit more rows per page — the rows past the first %d were not emitted\n", - len(kept), len(rows), compactListOutputLimit, len(kept)) - if got := projectionNote(t, bound, "fields", "limit"); got != wantNote { - t.Fatalf("note = %q, want %q", got, wantNote) + if bound.rowsEmitted != len(kept) || bound.rowsTotal != len(rows) || bound.maxBytes != compactListOutputLimit { + t.Fatalf("bound = %+v, want a %d-of-%d prefix reduction against the %d-byte limit", bound, len(kept), len(rows), compactListOutputLimit) + } + if bound.shortened != 0 { + t.Fatalf("bound reported %d shortened values; a page reduction must shorten none", bound.shortened) } } @@ -1029,15 +1025,15 @@ func TestStructuredFieldsEmptyErrors(t *testing.T) { } } -// TestBoundProjectedListAnnouncesShortening pins that a list projection which -// had to clip values says so on the caller's side. The "..." marker alone is -// only visible to something that READS the value; a --json consumer runs a jq -// filter or an exact match over it, where a clipped string produces an empty -// result that is indistinguishable from "nothing matched" — the expensive -// failure this note exists to prevent. The fixture is a single oversized row -// so the run lands on the shortening fallback (a multi-row page would be -// reduced, not shortened). -func TestBoundProjectedListAnnouncesShortening(t *testing.T) { +// TestBoundProjectedListReportsShortening pins that a list projection which had +// to clip values reports the loss as data. The "..." marker alone is only +// visible to something that READS the value; a --json consumer runs a jq filter +// or an exact match over it, where a clipped string produces an empty result +// indistinguishable from "nothing matched" — the expensive failure the caller's +// stderr note exists to prevent, keyed off these facts. The fixture is a single +// oversized row so the run lands on the shortening fallback (a multi-row page +// would be reduced, not shortened). +func TestBoundProjectedListReportsShortening(t *testing.T) { for _, format := range []string{"json", "toon"} { t.Run(format, func(t *testing.T) { saveAndResetGlobals(t) @@ -1051,20 +1047,19 @@ func TestBoundProjectedListAnnouncesShortening(t *testing.T) { if err != nil { t.Fatalf("bound projected output: %v", err) } - note := projectionNote(t, bound, "fields", "limit") - if note == "" { - t.Fatalf("shortened projection returned no note; caller cannot tell values were clipped") + if bound.shortened == 0 || bound.valuesTotal == 0 { + t.Fatalf("shortened projection reported no shortening: %+v", bound) } - if !strings.Contains(note, "title") { - t.Fatalf("note = %q, want it to name the shortened field (title)", note) + if len(bound.fields) != 1 || bound.fields[0] != "title" { + t.Fatalf("bound.fields = %v, want exactly the shortened field (title)", bound.fields) } }) } } -// TestBoundProjectedListNoNoteWhenNothingShortened keeps the note honest: a +// TestBoundProjectedListReportsNothingWhenFits keeps the report honest: a // projection that fits must not claim anything was clipped. -func TestBoundProjectedListNoNoteWhenNothingShortened(t *testing.T) { +func TestBoundProjectedListReportsNothingWhenFits(t *testing.T) { saveAndResetGlobals(t) flagOutputFormat = "json" rows := []map[string]any{{"incident_id": "inc-1", "title": "disk full"}} @@ -1073,8 +1068,8 @@ func TestBoundProjectedListNoNoteWhenNothingShortened(t *testing.T) { if err != nil { t.Fatalf("bound projected output: %v", err) } - if note := projectionNote(t, bound, "fields", "limit"); note != "" { - t.Fatalf("fitting projection returned note %q, want none", note) + if bound.rowsTotal != 0 || bound.shortened != 0 || len(bound.fields) != 0 { + t.Fatalf("fitting projection reported a reduction: %+v", bound) } } @@ -1276,15 +1271,41 @@ func TestChannelEscalateRuleListStructuredProjection(t *testing.T) { }) } -// TestReducedPageNoteNamesOnlyDeclaredFlags pins that the reduced-page note -// advises only the flags the running verb actually carries: --fields/--limit -// are registered per verb, so a note composed without the command's flag set -// sends the caller to a flag the verb rejects (unknown flag: --limit), which -// costs a wasted round trip for advice that cannot be obeyed. -func TestReducedPageNoteNamesOnlyDeclaredFlags(t *testing.T) { - // channel escalate-rule-list declares --fields but neither --limit nor - // --page: the advice may name --fields alone. - t.Run("fields without limit", func(t *testing.T) { +// TestReductionAdviceNamesOnlyDeclaredFlags pins that the projection note names +// a flag only when the command's own definition declares that flag as +// narrowing its structured output. --fields and --limit are registered per verb +// and their names do not imply their effect: a request-body write selector +// spelled --fields, or a --limit the server ignores, must never be offered as a +// way to shrink the output. +func TestReductionAdviceNamesOnlyDeclaredFlags(t *testing.T) { + // alert-event list declares both a projection --fields and a paging --limit, + // so the note may offer both. + t.Run("declared projection and rows", func(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + stub := newGFStub(t) + items := make([]any, 40) + for i := range items { + items[i] = map[string]any{ + "event_id": fmt.Sprintf("%024x", i), + "title": strings.Repeat(fmt.Sprintf("row %d fat event title 详情 ", i), 20), + } + } + stub.data = map[string]any{"items": items, "total": len(items)} + + _, stderrText, err := execCommandSplit("alert-event", "list", "--limit", "40", + "--fields", "event_id,title", "--output-format", "json") + if err != nil { + t.Fatalf("execCommandSplit: %v", err) + } + if !strings.Contains(stderrText, "narrow --fields or lower --limit to fit more rows per page") { + t.Fatalf("both declared flags should be offered, got:\n%s", stderrText) + } + }) + + // channel escalate-rule-list declares a projection --fields but no paging + // flag, so the note may offer --fields alone. + t.Run("declared projection only", func(t *testing.T) { saveAndResetGlobals(t) flagOutputFormat = "json" stub := newGFStub(t) @@ -1302,20 +1323,46 @@ func TestReducedPageNoteNamesOnlyDeclaredFlags(t *testing.T) { if err != nil { t.Fatalf("execCommandSplit: %v", err) } - if !strings.Contains(stderrText, "note: emitted") { - t.Fatalf("over-budget page should announce the reduction, got:\n%s", stderrText) - } if !strings.Contains(stderrText, "narrow --fields to fit more rows per page") { - t.Errorf("advice should name --fields, the flag this verb declares, got:\n%s", stderrText) + t.Fatalf("advice should name the declared --fields, got:\n%s", stderrText) } if strings.Contains(stderrText, "--limit") { - t.Errorf("advice must not name --limit; this verb declares no such flag, got:\n%s", stderrText) + t.Fatalf("advice must not name --limit; this verb declares no paging flag, got:\n%s", stderrText) } }) - // channel silence-rule-list is a generated verb that declares neither - // --fields nor --limit: the note must not invent one. - t.Run("neither flag", func(t *testing.T) { + // monit rule-update-fields spells a request-body write selector --fields and + // declares no narrowing: the note must not offer --fields, which would + // silently change a write instead of the output. + t.Run("write-selector fields is not a projection control", func(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + stub := newGFStub(t) + rows := make([]any, 60) + for i := range rows { + rows[i] = map[string]any{ + "message": "", + "name": strings.Repeat(fmt.Sprintf("rule %d ", i), 40), + } + } + stub.data = rows + + _, stderrText, err := execCommandSplit("monit", "rule-update-fields", + "--data", `{"ids":[50001],"fields":["enabled"]}`, "--output-format", "json") + if err != nil { + t.Fatalf("execCommandSplit: %v", err) + } + if !strings.Contains(stderrText, "note: emitted") { + t.Fatalf("over-budget response should announce the reduction, got:\n%s", stderrText) + } + if strings.Contains(stderrText, "--fields") { + t.Fatalf("advice must not name --fields; here it selects rule fields to WRITE, got:\n%s", stderrText) + } + }) + + // channel silence-rule-list is a generated verb that declares no narrowing + // flag: the note must not invent one. + t.Run("no narrowing flag", func(t *testing.T) { saveAndResetGlobals(t) flagOutputFormat = "json" stub := newGFStub(t) @@ -1336,24 +1383,156 @@ func TestReducedPageNoteNamesOnlyDeclaredFlags(t *testing.T) { if !strings.Contains(stderrText, "note: emitted") { t.Fatalf("over-budget page should announce the reduction, got:\n%s", stderrText) } - if !strings.Contains(stderrText, "declares no --fields or --limit flag") { - t.Errorf("advice should say this verb has no narrowing flag, got:\n%s", stderrText) + if !strings.Contains(stderrText, "declares no flag that narrows the output") { + t.Fatalf("advice should say no flag narrows this command, got:\n%s", stderrText) } for _, flag := range []string{"--fields", "--limit"} { - if strings.Contains(stderrText, "narrow "+flag) || strings.Contains(stderrText, "lower "+flag) { - t.Errorf("advice must not tell the caller to %s on a verb that rejects it, got:\n%s", flag, stderrText) + if strings.Contains(stderrText, flag) { + t.Fatalf("advice must not name %s on a verb that does not narrow with it, got:\n%s", flag, stderrText) } } }) } +// hugeTimeFilters returns a recurring-window list large enough that a single row +// carrying it cannot fit the compact budget even after its string values are +// shortened — the shape that lands the run on the irreducible-overflow path. +func hugeTimeFilters() []any { + out := make([]any, 400) + for i := range out { + out[i] = map[string]any{"start": "00:00", "end": "23:59", "repeat": []any{1, 2, 3, 4, 5, 6, 7}} + } + return out +} + +// TestShorteningAdviceOmitsRowsFlag pins that the single-row shortening branch +// offers only the projection control: requesting fewer rows cannot shrink the +// one row that already overflows, so a paging flag declared by the same command +// must not appear in that remedy. +func TestShorteningAdviceOmitsRowsFlag(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + stub := newGFStub(t) + stub.data = map[string]any{ + "items": []any{map[string]any{ + "event_id": fmt.Sprintf("%024x", 1), + "title": strings.Repeat("payment-gateway timeout 详情 ", 800), + }}, + "total": 1, + } + + _, stderrText, err := execCommandSplit("alert-event", "list", "--fields", "event_id,title", "--output-format", "json") + if err != nil { + t.Fatalf("execCommandSplit: %v", err) + } + if !strings.Contains(stderrText, "were shortened to fit") { + t.Fatalf("a clipped value should be announced, got:\n%s", stderrText) + } + if !strings.Contains(stderrText, "narrow --fields for untruncated values") { + t.Fatalf("the shortening remedy should name the declared projection, got:\n%s", stderrText) + } + if strings.Contains(stderrText, "--limit") { + t.Fatalf("a smaller page cannot shrink the overflowing row, so --limit must not be offered, got:\n%s", stderrText) + } +} + +// TestOverflowErrorNamesOnlyDeclaredFlags pins that the irreducible-overflow +// failure carries the same command-scoped remedy as the note: it is reachable by +// the same generated verbs, so it must not name a flag the command does not +// declare as narrowing. +func TestOverflowErrorNamesOnlyDeclaredFlags(t *testing.T) { + // A row whose bulk is a non-string field cannot be shortened, so the command + // fails. channel silence-rule-list declares no narrowing flag. + t.Run("no narrowing flag", func(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + stub := newGFStub(t) + stub.data = map[string]any{"items": []any{map[string]any{ + "rule_id": fmt.Sprintf("%024x", 1), + "rule_name": "silence everything", + "time_filters": hugeTimeFilters(), + }}} + + _, _, err := execCommandSplit("channel", "silence-rule-list", "4201", "--output-format", "json") + if err == nil { + t.Fatal("irreducible single row should fail, got nil") + } + if !strings.Contains(err.Error(), "largest fields:") { + t.Fatalf("error should name the largest fields, got: %v", err) + } + if !strings.Contains(err.Error(), "declares no flag that narrows the output") { + t.Fatalf("error should state no flag narrows this command, got: %v", err) + } + for _, flag := range []string{"--fields", "--limit"} { + if strings.Contains(err.Error(), flag) { + t.Fatalf("error must not name %s on a verb that does not narrow with it, got: %v", flag, err) + } + } + }) + + // channel escalate-rule-list declares a projection --fields, so the failure + // may name it and must not name the paging flag the verb does not have. + t.Run("declared projection", func(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + stub := newGFStub(t) + stub.data = map[string]any{"items": []any{map[string]any{ + "rule_id": "6621b23f4a2c5e0012ab34d0", + "rule_name": "P1 on-call", + "time_filters": hugeTimeFilters(), + }}} + + _, _, err := execCommandSplit("channel", "escalate-rule-list", "4201", + "--fields", "rule_id,rule_name,time_filters", "--output-format", "json") + if err == nil { + t.Fatal("irreducible single row should fail, got nil") + } + if !strings.Contains(err.Error(), "narrow --fields") { + t.Fatalf("error should name the declared --fields, got: %v", err) + } + if strings.Contains(err.Error(), "--limit") { + t.Fatalf("error must not name --limit; this verb declares no paging flag, got: %v", err) + } + }) +} + +// TestIgnoredLimitIsNeverOffered pins the other half of the declaration rule: a +// verb whose --limit the server ignores gets no paging advice. safari +// knowledge-file-list documents its --limit as accepted-but-ignored, and it +// declares no narrowing flag, so no reduction note can offer --limit. +func TestIgnoredLimitIsNeverOffered(t *testing.T) { + saveAndResetGlobals(t) + flagOutputFormat = "json" + stub := newGFStub(t) + files := make([]any, 200) + for i := range files { + files[i] = map[string]any{ + "file_id": fmt.Sprintf("kfl_%06d", i), + "rel_path": strings.Repeat(fmt.Sprintf("runbooks/restart-%d.md ", i), 10), + "checksum": strings.Repeat("a", 64), + "pack_id": "kp_1", + "size_bytes": 1024, + "updated_by": 101, + } + } + stub.data = map[string]any{"files": files, "total": len(files)} + + _, stderrText, err := execCommandSplit("safari", "knowledge-file-list", "--limit", "50", "--output-format", "json") + if err != nil { + t.Fatalf("execCommandSplit: %v", err) + } + if strings.Contains(stderrText, "--limit") { + t.Fatalf("a --limit the server ignores must never be offered, got:\n%s", stderrText) + } +} + // TestBoundProjectedListNeverShortensIdentifierFields pins the identifier // exemption: keys ending in _id/_key carry values a consumer matches, // filters, or passes back verbatim (a jq exact-match over --json output, a // follow-up detail call), so shortening one silently defeats that consumer. // The fixture is a single row whose oversized title overflows the budget on // its own, landing the run on the shortening fallback: the identifiers must -// come back byte-identical, only the free-text title shortens, and the note +// come back byte-identical, only the free-text title shortens, and the report // must name only the clipped field. func TestBoundProjectedListNeverShortensIdentifierFields(t *testing.T) { for _, format := range []string{"json", "toon"} { @@ -1389,15 +1568,11 @@ func TestBoundProjectedListNeverShortensIdentifierFields(t *testing.T) { t.Errorf("title should be shortened with the \"...\" marker, got %q", title) } - note := projectionNote(t, bound, "fields", "limit") - if note == "" { - t.Fatal("shortened projection returned no note; caller cannot tell values were clipped") - } - if !strings.Contains(note, "title") { - t.Errorf("note = %q, want it to name the shortened field (title)", note) + if len(bound.fields) != 1 || bound.fields[0] != "title" { + t.Fatalf("bound.fields = %v, want only the shortened non-identifier field (title)", bound.fields) } - if strings.Contains(note, "event_id") || strings.Contains(note, "alert_key") { - t.Errorf("note = %q, want it to name only shortened fields, never exempt identifiers", note) + if bound.shortened != 1 { + t.Fatalf("bound.shortened = %d, want exactly the one clipped value", bound.shortened) } encoded, err := marshalStructured(kept) @@ -1414,7 +1589,7 @@ func TestBoundProjectedListNeverShortensIdentifierFields(t *testing.T) { // TestBoundProjectedListIdentifierOnlyOverflowReducesPage pins the other half // of the identifier exemption: a page carrying nothing shortenable (only // identifier content) that overflows the budget is reduced to the leading -// rows that fit — identifiers stay byte-identical and the note names the +// rows that fit — identifiers stay byte-identical and the report records the // emitted count — instead of clipping identifiers or erroring out. func TestBoundProjectedListIdentifierOnlyOverflowReducesPage(t *testing.T) { saveAndResetGlobals(t) @@ -1445,9 +1620,8 @@ func TestBoundProjectedListIdentifierOnlyOverflowReducesPage(t *testing.T) { t.Errorf("row %d incident_id was mutated: got %q, want byte-identical %q", i, got, originals[i]) } } - wantNote := fmt.Sprintf("emitted %d of %d", len(kept), len(rows)) - if note := projectionNote(t, bound, "fields", "limit"); !strings.Contains(note, wantNote) { - t.Fatalf("note = %q, want it to name the emitted count (%q)", note, wantNote) + if bound.rowsEmitted != len(kept) || bound.rowsTotal != len(rows) { + t.Fatalf("bound = %+v, want it to report emitting %d of %d rows", bound, len(kept), len(rows)) } encoded, err := marshalStructured(kept) if err != nil { diff --git a/internal/cli/gen_support.go b/internal/cli/gen_support.go index 5eeda82..7a72408 100644 --- a/internal/cli/gen_support.go +++ b/internal/cli/gen_support.go @@ -380,7 +380,7 @@ func printBoundedGenericResult(ctx *RunContext, data any) error { } bounded, bound, err := boundProjectedList(rows, compactListOutputLimit) if err != nil { - return err + return explainProjectionOverflow(ctx.Cmd, err) } noteProjectionBound(ctx.Cmd, bound) return ctx.Printer.Print(bounded, nil) @@ -404,7 +404,7 @@ func printBoundedGenericResult(ctx *RunContext, data any) error { for { bounded, bound, err := boundProjectedList(rows, budget) if err != nil { - return err + return explainProjectionOverflow(ctx.Cmd, err) } value[key] = bounded out, err := marshalStructured(value) diff --git a/internal/cli/gen_support_test.go b/internal/cli/gen_support_test.go index e344d58..e5fc36a 100644 --- a/internal/cli/gen_support_test.go +++ b/internal/cli/gen_support_test.go @@ -76,13 +76,15 @@ func TestPrintGenericResultBoundsListEnvelope(t *testing.T) { if !strings.Contains(stderrText, "note: emitted") { t.Errorf("reduced %s page should announce itself on stderr, got:\n%s", format, stderrText) } - // insight incident-list declares --limit but no --fields: the advice - // may name only the flag the verb actually carries. - if !strings.Contains(stderrText, "lower --limit") { - t.Errorf("advice should name --limit, the flag this verb declares, got:\n%s", stderrText) + // insight incident-list is a generated verb: it declares no narrowing + // flag, so even though it carries a --limit, the note names none. + if !strings.Contains(stderrText, "declares no flag that narrows the output") { + t.Errorf("a generated verb should get the flagless note, got:\n%s", stderrText) } - if strings.Contains(stderrText, "--fields") { - t.Errorf("advice must not name --fields; this verb declares no such flag, got:\n%s", stderrText) + for _, flag := range []string{"--limit", "--fields"} { + if strings.Contains(stderrText, flag) { + t.Errorf("advice must not name %s on a verb that declares no narrowing flag, got:\n%s", flag, stderrText) + } } // The first row survives intact; the last was dropped by the // prefix reduction. @@ -140,13 +142,14 @@ func TestPrintGenericResultBoundsTopLevelArray(t *testing.T) { if !strings.Contains(stderrText, "note: emitted") { t.Errorf("reduced page should announce itself on stderr, got:\n%s", stderrText) } - // monit rule-list-basic declares --limit but no --fields: the advice may - // name only the flag the verb actually carries. - if !strings.Contains(stderrText, "lower --limit") { - t.Errorf("advice should name --limit, the flag this verb declares, got:\n%s", stderrText) + // monit rule-list-basic is a generated verb whose --limit sizes the response + // only alongside --include-descendants: it declares no narrowing flag, so the + // note must not offer --limit. + if !strings.Contains(stderrText, "declares no flag that narrows the output") { + t.Errorf("a generated verb should get the flagless note, got:\n%s", stderrText) } - if strings.Contains(stderrText, "--fields") { - t.Errorf("advice must not name --fields; this verb declares no such flag, got:\n%s", stderrText) + if strings.Contains(stderrText, "--limit") { + t.Errorf("advice must not name --limit; this verb declares no narrowing flag, got:\n%s", stderrText) } var decoded []map[string]any if err := json.Unmarshal([]byte(strings.TrimSpace(out)), &decoded); err != nil { diff --git a/internal/cli/incident.go b/internal/cli/incident.go index 298ce6e..f4833d5 100644 --- a/internal/cli/incident.go +++ b/internal/cli/incident.go @@ -139,7 +139,7 @@ func newIncidentListCmd() *cobra.Command { } bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { - return err + return explainProjectionOverflow(cmd, err) } proj = bounded.([]map[string]any) noteProjectionBound(cmd, bound) @@ -176,6 +176,7 @@ func newIncidentListCmd() *cobra.Command { cmd.Flags().IntVar(&limit, "limit", 20, "Max results (max 100)") cmd.Flags().IntVar(&page, "page", 1, "Page number") cmd.Flags().StringVar(&fields, "fields", "", "Comma-separated fields to project in json/toon output (e.g. incident_id,title,incident_severity,progress,start_time); ignored in table mode. Use to avoid dumping the full nested record.") + declareOutputNarrowing(cmd, "fields", "limit") return cmd } @@ -640,7 +641,7 @@ func newIncidentSimilarCmd() *cobra.Command { } bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { - return err + return explainProjectionOverflow(cmd, err) } proj = bounded.([]map[string]any) noteProjectionBound(cmd, bound) @@ -654,6 +655,7 @@ func newIncidentSimilarCmd() *cobra.Command { cmd.Flags().IntVar(&limit, "limit", 5, "Max results") cmd.Flags().StringVar(&fields, "fields", "", "Comma-separated fields to project in json/toon output (e.g. incident_id,title,incident_severity,progress,start_time); ignored in table mode. Defaults to a compact incident summary. If the page would exceed 16 KiB, only the leading rows that fit are emitted, with every value intact (announced on stderr).") + declareOutputNarrowing(cmd, "fields", "limit") return cmd } @@ -1615,7 +1617,7 @@ func newIncidentDetailCmd() *cobra.Command { } _, bound, err := boundProjectedOutput(proj[0], compactDetailOutputLimit) if err != nil { - return err + return explainProjectionOverflow(cmd, err) } noteProjectionBound(cmd, bound) return ctx.Printer.Print(proj[0], nil) @@ -1629,6 +1631,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. The projection must fit within 8 KiB or the command fails and names the largest fields; omit --fields for the full, unbounded detail.") + declareOutputNarrowing(cmd, "fields", "") return cmd } diff --git a/internal/cli/insight.go b/internal/cli/insight.go index a4c5a1a..97a3c99 100644 --- a/internal/cli/insight.go +++ b/internal/cli/insight.go @@ -131,7 +131,7 @@ func newInsightIncidentsCmd() *cobra.Command { } bounded, bound, err := boundProjectedOutput(proj, compactListOutputLimit) if err != nil { - return err + return explainProjectionOverflow(cmd, err) } proj = bounded.([]map[string]any) noteProjectionBound(cmd, bound) @@ -176,6 +176,7 @@ func newInsightIncidentsCmd() *cobra.Command { cmd.Flags().IntVar(&limit, "limit", 20, "Max results (max 100)") cmd.Flags().IntVar(&page, "page", 1, "Page number") cmd.Flags().StringVar(&fields, "fields", "", "Comma-separated fields to project in json/toon output (e.g. incident_id,title,severity); ignored in table mode. Use to avoid dumping the full nested record.") + declareOutputNarrowing(cmd, "fields", "limit") return cmd } diff --git a/internal/cmd/cligen/main.go b/internal/cmd/cligen/main.go index a3e0b48..a219808 100644 --- a/internal/cmd/cligen/main.go +++ b/internal/cmd/cligen/main.go @@ -1317,6 +1317,13 @@ func emitCmd(fn string, s service, o specOp, mi methodInfo) string { call += ")" } + // Generated commands emit no declareOutputNarrowing call. The spec gives no + // structured contract for whether a --limit bounds the emitted rows (the + // knowledge file-list documents its --limit as ignored; rule-list-basic + // honors its own only alongside --include-descendants), and a flag's name is + // not evidence of its effect. With no declaration the projection note names + // no flag, which is the honest default; a hand-written command that knows + // what its flags do declares them itself. switch { case mi.HasData: fmt.Fprintf(&b, "\t\t\t\tout, _, err := %s\n", call) From 57c633e41c36fd2b2a79137d297d2f0998be10bd Mon Sep 17 00:00:00 2001 From: ysyneu Date: Fri, 11 Sep 2026 00:36:52 -0700 Subject: [PATCH 3/3] test(cli): drop the ignored-limit test that never reached the note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestIgnoredLimitIsNeverOffered drove safari knowledge-file-list, whose response shape (a files array beside total) printBoundedGenericResult does not treat as a list envelope, so the payload was never bounded and no note was emitted — the sole assertion could not fail. The same proposition is already pinned non-vacuously by the monit rule-list-basic case, which asserts the note is emitted and names no flag. --- internal/cli/fieldproject_test.go | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/internal/cli/fieldproject_test.go b/internal/cli/fieldproject_test.go index 0cd1a67..907a0ad 100644 --- a/internal/cli/fieldproject_test.go +++ b/internal/cli/fieldproject_test.go @@ -1496,36 +1496,6 @@ func TestOverflowErrorNamesOnlyDeclaredFlags(t *testing.T) { }) } -// TestIgnoredLimitIsNeverOffered pins the other half of the declaration rule: a -// verb whose --limit the server ignores gets no paging advice. safari -// knowledge-file-list documents its --limit as accepted-but-ignored, and it -// declares no narrowing flag, so no reduction note can offer --limit. -func TestIgnoredLimitIsNeverOffered(t *testing.T) { - saveAndResetGlobals(t) - flagOutputFormat = "json" - stub := newGFStub(t) - files := make([]any, 200) - for i := range files { - files[i] = map[string]any{ - "file_id": fmt.Sprintf("kfl_%06d", i), - "rel_path": strings.Repeat(fmt.Sprintf("runbooks/restart-%d.md ", i), 10), - "checksum": strings.Repeat("a", 64), - "pack_id": "kp_1", - "size_bytes": 1024, - "updated_by": 101, - } - } - stub.data = map[string]any{"files": files, "total": len(files)} - - _, stderrText, err := execCommandSplit("safari", "knowledge-file-list", "--limit", "50", "--output-format", "json") - if err != nil { - t.Fatalf("execCommandSplit: %v", err) - } - if strings.Contains(stderrText, "--limit") { - t.Fatalf("a --limit the server ignores must never be offered, got:\n%s", stderrText) - } -} - // TestBoundProjectedListNeverShortensIdentifierFields pins the identifier // exemption: keys ending in _id/_key carry values a consumer matches, // filters, or passes back verbatim (a jq exact-match over --json output, a