Skip to content

feat(openapi): enforce required query params, add --query escape hatch - #76

Open
dspangen wants to merge 2 commits into
mainfrom
feat/required-query-params
Open

feat(openapi): enforce required query params, add --query escape hatch#76
dspangen wants to merge 2 commits into
mainfrom
feat/required-query-params

Conversation

@dspangen

@dspangen dspangen commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Two query-param gaps fixed on generated commands:

  • Spec-required query params are now enforced client-side (required flag(s) "q" not set, (required) in help, and an empty value like --q= is also rejected) — previously a missing one cost a full round trip to a server 400.
  • New repeatable --query key=value escape hatch sends params the spec doesn't declare — previously such operations were simply uncallable (one audited incident burned 45k tokens before giving up). Keys go out verbatim; conflicts with an explicitly set declared flag error instead of silently picking one.

omni <cmd> --schema still works with zero flags — required-flag enforcement is toggled off for schema runs.

Details: semantics, the --schema design note, and verification

What changed

  • Spec-required query params are MarkFlagRequired and labelled (required) in their flag usage. Missing one fails locally with cobra's standard required flag(s) "q" not set. A checkRequiredQueryParams pass in RunE also rejects explicitly empty values (--q=, --q "") against the assembled query string, so the escape-hatch spellings are covered too.
  • Every generated command gets a repeatable --query key=value (StringArray) escape hatch. Values merge into the query string alongside declared flags.
    • Keys are sent verbatim — no case or separator normalization, the server sees exactly what was typed.
    • Repeating a key sends every value (--query tag=a --query tag=btag=a&tag=b), for array-shaped params.
    • A --query key that duplicates an explicitly set declared flag is ambiguous and errors rather than silently picking one. Both spellings are caught (--query connectionId= and --query connectionid= both conflict with --connectionid). If the declared flag is unset, --query may supply its value.
    • Malformed values get a clear error: invalid --query value "nope": expected key=value.
  • Collision guard: --query is registered after spec params and body-shorthand flags, behind a Lookup(...) == nil check, so an operation that declares a param slugifying to query keeps its own flag instead of panicking pflag (same defensive pattern as --field/--depth).
  • omni agent-help documents both behaviors.

The --schema interaction (design note)

Cobra validates required flags in execute() after PreRunE but before RunE, so the existing --schema short-circuit (which wraps Args and RunE) could not cover them — omni <cmd> --schema would have started failing on operations with required query params, breaking zero-friction discovery.

Approach chosen: a PreRunE that rewrites the BashCompOneRequiredFlag annotation for this invocation — "false" when --schema is present, "true" otherwise. It's registered only when the operation actually has required query params.

Rejected alternatives:

  • Mark required only when --schema is absent from os.Args — untestable in-process (the test binary's argv is not the command's) and couples flag wiring to process argv.
  • Clear the annotation in PreRunE — would leave the requirement relaxed for any later invocation on the same command object. Rewriting it every run is self-healing.

Verification

  • make build — clean; make test — all packages pass.
  • New tests in internal/openapi/generate_test.go: required param missing → client-side error + (required) in usage; required param present → in query string; explicitly empty required values rejected end-to-end (--connectionid=, --connectionid "") and via a table test over the assembled query (both spellings, multi-value, optional-empty-is-fine); --query merges extras incl. repeated and empty values; malformed --query errors; --query conflicting with a set declared flag errors (both spellings); no conflict when the declared flag is unset; a spec param named query keeps the flag; --schema works with no flags while the same operation still enforces the required param without --schema.
  • Manual smoke against the built binary (5 spec operations have required query params today: content search, ai-eval runs-list, models dbt-sync, models yaml-delete, query wait):
$ ./bin/omni content search
Error: required flag(s) "q" not set

$ ./bin/omni content search --q=
Error: required flag(s) "q" cannot be empty

$ ./bin/omni content search --q foo --query q=bar
Error: --query q=... conflicts with --q; set that parameter one way or the other

$ ./bin/omni content search --help
      --q string            Free-text keywords ... (required)
      --query stringArray   send an extra query parameter not declared in the spec (repeatable, key=value)

🤖 Generated with Claude Code

https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s

@dspangen
dspangen force-pushed the feat/required-query-params branch from 05b370b to ee9ddc1 Compare August 24, 2026 16:24
@dspangen
dspangen requested a review from n8agrin August 25, 2026 13:37
dspangen and others added 2 commits August 25, 2026 12:00
Spec-required query params were registered as ordinary optional flags, so a
missing one only surfaced as a server 400 after a round trip. They are now
marked required on the cobra flag (and labelled "(required)" in help), so the
failure is local and immediate.

The reverse gap was worse: when the server requires a query param the spec
omits entirely, the operation was uncallable — no flag existed to send it. A
repeatable --query key=value flag is now registered on every generated command
and merged into the query string alongside the declared flags. Keys are sent
verbatim so the server sees exactly what was typed, repeating a key sends every
value (array params), and a key that duplicates an explicitly-set declared flag
errors instead of silently picking one.

Constraint: `omni <cmd> --schema` must stay zero-friction (no args, no token, no
API call), but cobra validates required flags before RunE, so the existing
RunE/Args short-circuit could not cover them
Constraint: --query must never panic flag registration on a spec that declares a
param slugifying to "query"
Rejected: mark required only when "--schema" is absent from os.Args | untestable
in-process and couples flag wiring to process argv
Rejected: clear the required annotation in PreRunE | leaves the requirement
relaxed for any later invocation on the same command object; PreRunE now
rewrites the annotation to the correct value every run instead
Rejected: last-one-wins when --query duplicates a declared flag | silently drops
a value the user explicitly asked for
Confidence: high
Scope-risk: moderate
Directive: --query is registered after spec params and body-shorthand flags and
guarded by a Lookup; keep that ordering so a future flag named "query" degrades
rather than panics
Not-tested: real server behavior for repeated --query keys on a param the API
does not treat as an array

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s
MarkFlagRequired only proves a flag was supplied, so `omni content search --q=`
passed local validation and then had the empty value dropped when the query
string was built — the request went out with no ?q= at all and earned exactly
the server 400 this feature was meant to eliminate.

Required query params are now validated against the assembled query string at
the start of RunE, after --query extras are merged, so every path into the
parameter is covered. A value under either the spec spelling or the flag
spelling satisfies it, mirroring how --query conflicts are detected.

TestSpecCoverage calls RunE directly and so bypasses cobra's own required-flag
check; it now sets a dummy value for each required query param, which is what
surfaced the five real spec operations affected (content search, ai-eval
runs-list, models dbt-sync, models yaml-delete, query wait).

Constraint: the check must run after --query merging, or the escape hatch could
supply a value the validator can't see
Rejected: reject empty strings at flag-parse time via a custom pflag Value |
would also block legitimately empty optional params, which the API accepts
Rejected: send required params even when empty | trades a clear local error for
an opaque server 400, the exact failure being fixed
Confidence: high
Scope-risk: narrow
Directive: keep checkRequiredQueryParams downstream of applyExtraQueryParams;
ordering is load-bearing
Not-tested: servers that treat an explicitly empty required param as valid

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s
@dspangen
dspangen force-pushed the feat/required-query-params branch from ee9ddc1 to f26bdd3 Compare August 25, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant