fix(cli): never truncate incident detail --fields values - #141
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
boundProjectedOutputreceived two different shapes and applied one budget strategy to both:[]map[string]any, 16 KiB) — many small rowsincident detail --fields(map[string]any, 8 KiB)On overflow it shortened every string in place, then halved the per-field limit and retried.
For a list that is defensible. For a single object it silently corrupts data, in three compounding ways:
truncateUTF8Bytesstops appending the...marker once the limit is<= 3, and returns""at<= 0. So a 24-character id can arrive as"6"and"Warning"as"W"— indistinguishable from a genuinely short value.The loop's
fieldLimit == 0error exit is unreachable: by the time the limit reaches 0 every string is empty, so the payload always fits and the command exits 0 with hollowed-out fields. Nothing in the output says anything was dropped.incident detail --fields(internal/cli/incident.go:1544) is the only call site that passes a single object; the other three pass lists.Change
Split the two shapes behind the existing entry point.
boundProjectedList).boundProjectedDetail). If the projection does not fit, the command fails and names the largest fields with their sizes, so the caller can fix the request in one pass.Ties in that ranking break on field name, so the same oversized request never names different fields between runs (Go's map iteration order is randomized).
Why error rather than drop keys or shrink
Omitting a key is indistinguishable from a null one to a
jqconsumer — that reintroduces exactly the class of bug being removed. And there is already a strictly better escape hatch: omitting--fieldsreturns the full, unbounded detail, i.e. more data, not less. So the failure is always actionable, which is what the error text says.Tests
reflect.DeepEqualagainst an independently built expectation — the regression guard)incident detail --fieldsoversized case errors, naming the budget, a largest field, and--fieldsas the remedyTestIncidentDetailFieldsProjectionpreviously asserted the truncate-and-succeed path; it now uses realistic in-budget values and asserts a retained value is unaltered. Its structural assertions (key set, excluded fields, size bound) are unchanged.Verified red before green: reverting only the implementation fails both oversized guards; reverting only the tie-break fails the determinism test.
make check(gofmt/gci, golangci-lint 0 issues,go test -race ./..., build) andmake check-cardsboth pass.Docs
--fieldsflag help and the hand-written note inskills/flashduty/reference/incident.mdnow distinguishsimilar(list — still shortens, marked with...) fromdetail --fields(never shortens; fails and names the largest fields). The<!-- GENERATED -->fence is untouched.