Repository navigation
Accept -o json on kernel search commands and extend search contents - #297
Conversation
Search commands always print JSON, but they rejected the -o flag that other commands accept, so scripts piping through jq got an error on stderr instead of JSON. Register the shared output flag on search, get, providers, and contents, and validate it before any request is sent.
- Add --request to `search contents`, taking the full request body as a JSON object and sending it unchanged. It cannot be combined with the other contents flags. - Clamp --content-max-chars to the API's 100–100000 range instead of passing out-of-range values through to a server error. Negative values still error.
chruffins
changed the title
masnwilliams
left a comment
masnwilliams
left a comment
There was a problem hiding this comment.
Approve with comments.
-o json on the search commands looks good. It brings the code in line with what the README already documents, and checking the value before the request means a bad value can't run a billable search. go test ./cmd/... and go vet pass at fd0c6df.
Two things I'd like fixed before merge:
- The README change is on the wrong command (inline).
kernel search <query>has no--content-max-charsflag; the clamp is onsearch contents, and that README line is unchanged. - Make the
--requestdocs agree.contentsnow accepts--request, but its Long help still says "Provide exactly one of --result-ids or --limit", the example doesn't show it, and the READMEsearch contentssection doesn't list--requestor-o. The agent instructions that currently say--requestisn't valid oncontentsalso need to be reverted before or alongside this.
The rest are optional (inline):
- Let cobra enforce the
--requestconflicts so the list can't go stale when a flag is added. --content-max-charsis now the only bounded flag that clamps; the others error.- Register
-oonce as a persistent flag onsearch.
| - `--content-source <source>` - Retrieval source: `auto`, `provider`, or `browser` | ||
| - `--content-format <format>` - Extracted content format: `markdown` or `text` | ||
| - `--content-max-chars <n>` - Per-result Unicode character limit after extraction | ||
| - `--content-max-chars <n>` - Per-result Unicode character limit after extraction (clamped to 100–100000; default 10000) |
There was a problem hiding this comment.
This line is under kernel search <query>, which doesn't register --content-max-chars (only --provider, --max-results, --request). The clamp applies to kernel search contents --content-max-chars, which is around line 1547 and still says "Per-result character limit after extraction". Can you move the change there? That section could also list --request and -o json.
| } | ||
| flags := cmd.Flags() | ||
| if flags.Changed("request") { | ||
| for _, name := range []string{"result-ids", "limit", "timeout-ms", "content-source", "content-format", "content-max-chars", "content-max-age-hours", "content-timeout-ms", "content-browser-id", "content-browser-mode"} { |
There was a problem hiding this comment.
optional: this list has to be kept in sync by hand. If a contents flag is added later and not added here, it gets silently ignored whenever --request is set. kernel search already lets cobra do this with MarkFlagsMutuallyExclusive, so you could do the same here: register --request first, then after the other flags
contents.Flags().VisitAll(func(f *pflag.Flag) { contents.MarkFlagsMutuallyExclusive("request", f.Name) })The JSON-object check right below also duplicates the one in runSearch (which keeps the parse error with %w; this one drops it). A shared jsonObjectFlag(cmd, name) helper would cover both. I tried both changes locally and the existing tests still pass.
| if value < 0 { | ||
| return fmt.Errorf("--content-max-chars must not be negative") | ||
| } | ||
| content.MaxChars = kernel.Int(int64(min(max(value, 100), 100000))) |
There was a problem hiding this comment.
optional: this is now the only bounded flag that clamps instead of erroring (--limit and --timeout-ms error on out-of-range values). Clamping is also skipped for --request bodies, and --content-max-chars 0 becomes 100 even though the help says the default is 10000. Clamping is a reasonable choice for agents, but I'd lean toward --content-max-chars must be between 100 and 100000. It's checked before the API call either way, and it matches the other flags. If you keep the clamp, maybe print a warning on stderr when the value is changed.
| contents.MarkFlagsMutuallyExclusive("result-ids", "limit") | ||
| contents.MarkFlagsOneRequired("result-ids", "limit") | ||
| contents.MarkFlagsOneRequired("result-ids", "limit", "request") | ||
| addJSONOutputFlag(cmd) |
There was a problem hiding this comment.
nit: cmd.PersistentFlags().StringP("output", "o", "", util.JSONOutputFlagDescription) once on search covers all four subcommands, and a new search subcommand can't miss it and bring back unknown shorthand flag 'o'. executeSearchRequest still reads it the same way.
- Register -o/--output once on the search command as a persistent flag. - Let cobra enforce that --request cannot be combined with the other contents flags, so new flags are covered automatically. - Share JSON object parsing between search and contents --request. - Warn on stderr when --content-max-chars is clamped. - Move the clamp note to the search contents section of the README.
Summary
kernel search,search get,search providers, andsearch contentsrejected-o jsonwithUnknown shorthand flag. Other commands accept it, and the search output is already JSON. The shared-o/--outputflag is now registered on the four search commands. It acceptsjsonor nothing, and is validated before the request, so a bad value does not run a billable search.search contentsaccepts--requestwith a complete JSON request body (result_idsorlimit, plus optionaltimeout_msandcontent). The body is sent unchanged, and the server validates it.--requestcannot be combined with the other contents flags. This matcheskernel search --request.--content-max-charsis clamped to the API range of 100–100000 before the request, so out-of-range values no longer fail on the server. Negative values still error. The flag help and the README document the range and the clamp.Testing
go test ./cmd/...passes. New cases cover-o jsonon get, providers, and contents; invalid-o yamlon search, get, and contents with no API call expected; a--requestbody sent verbatim;--requestcombined with other contents flags; a non-object--request; and clamping above and below the range.Notes
contents --requestis now a supported flag. The previous revision of this PR said it was never a CLI flag. The agent instructions that document it were corrected separately in kernel/hypeship#1437.--requestbodies are not clamped. Amax_charsvalue outside the range inside the JSON is still rejected by the server.