fix(explore): avoid panic when printing an empty result set - #68
fix(explore): avoid panic when printing an empty result set#68thegoodengineer wants to merge 1 commit into
Conversation
`getSelectedContent` indexed `rowData` at the table cursor without checking
that a row exists. An empty array or object builds a TableView with no rows,
so pressing "p" on one crashed the CLI with an index out of range panic.
This is reachable whenever a list endpoint returns no results, since
`ExploreJSONStream` marshals zero items into `[]` and builds the table from
that, e.g. `openai files list --format explore` on an account with no files.
`navigateForward` already guards the same empty `rowData` case; this applies
the equivalent check to the print path and falls back to the container that
the view is displaying, so "p" prints `[]` or `{}` instead of panicking.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
2 participants
Summary
Pressing
p("print and exit") in the--format exploreviewer panics when the result set is empty.getSelectedContentindexesrowDataat the table cursor without checking that a row exists:https://github.com/openai/openai-cli/blob/7d87ee2/internal/jsonview/explorer.go#L423-L434
An empty array or object builds a
TableViewwith no rows,table.Cursor()returns0, and the index panics.navigateForwardalready guards this exact case (added inTestNavigateForward_EmptyRowData), so this looks like the same oversight in the sibling path rather than an intended difference.How it is reached
Any list endpoint that returns no results.
ExploreJSONStreamcollects zero items,marshalItemsToJSONArrayreturns[], andnewTableViewbuilds a table with no rows:https://github.com/openai/openai-cli/blob/7d87ee2/internal/jsonview/explorer.go#L331-L349
So on an account with no files:
then press
p, and the CLI exits with a runtime panic and a Go stack trace instead of printing anything. The same applies to a top level empty object viaExploreJSON.Every other key binding (
↑,↓,←,→,r,q) already handles the empty view fine.pis the only one that crashes.Reproduction
Reverting just the one line change and running the test added here:
With the fix applied, both cases pass.
Fix
Bounds check the cursor before indexing, and fall back to the container the view is already displaying, so
pprints[]or{}. That matches the existing fallback for non table views a few lines above, which returnsGetData().Raw.The test drives
Updatewith the actualpkey message rather than calling the unexported helper directly, so it covers the real key binding path and asserts the printed output, not just the absence of a panic.Notes
go build ./...andgo vet ./...are clean;go test ./internal/...passes apart frominternal/autocomplete, which fails identically on an unmodified checkout in my environment (it shells out to/bin/bashand I am on Windows), so it is unrelated to this change.