Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a comprehensive AIP (API Improvement Proposal) request parsing framework and migrates OpenAPI validation from kin-openapi to libopenapi-validator. It expands filter operators in the string filter domain, updates generated API types, and refactors request handling to support pagination, filtering, and sorting through a configurable AIP layer. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span multiple architectural layers (OpenAPI validation, request parsing, handler logic) with substantial logic density in filter/pagination/sort parsing and cursor encryption. While well-tested, the heterogeneity of changes across distinct domains requires careful reasoning for each cohort. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
There was a problem hiding this comment.
CVE-2024-44905 in github.com/go-pg/pg/v10 - medium severity
go-pg pg v10.13.0 was discovered to contain a SQL injection vulnerability via the component /types/append_value.go.
Details
Remediation Aikido suggests bumping this package to version 10.15.0 to resolve this issue
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v3/handlers/llmcost/convert.go (1)
206-225:⚠️ Potential issue | 🟠 Major
oeqandocontainsstop at the API boundary.
api.FilterStringgained those operators in this PR, but this converter still only forwardseq/neq/contains. A request that uses the new operators gets treated like no filter was supplied onceout.IsEmpty()runs.💡 Suggested fix
out := &filters.StringFilter{ - Eq: f.Eq, - Neq: f.Neq, - Contains: f.Contains, + Eq: f.Eq, + Neq: f.Neq, + Contains: f.Contains, + Oeq: f.Oeq, + Ocontains: f.Ocontains, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/llmcost/convert.go` around lines 206 - 225, filterSingleStringToDomain currently only copies Eq, Neq, and Contains from api.FilterString into filters.StringFilter so incoming oeq and ocontains operators are lost and the filter becomes empty; update the function to also copy the oeq and ocontains fields from the input api.FilterString into the resulting filters.StringFilter (preserving the same field names used on the domain type), then let the existing Validate() and IsEmpty() logic run as before.
🧹 Nitpick comments (14)
api/spec/package.json (1)
6-6: Nice simplification; consider adding an explicit “full” compile alias.Since
compilenow skips client generation, it could help to add something likecompile:all(openapi + v3 + client) for discoverability and to avoid accidental drift in local generation flows.Based on learnings: Regenerate OpenAPI spec and SDKs by running
make gen-apiafter editing TypeSpec files inapi/spec/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/package.json` at line 6, Add an explicit full compile alias to package.json to run the OpenAPI spec, v3, and client generation in one command: add a new script "compile:all" (or "compile:full") that chains the existing "compile:openapi", "compile:v3", and the client generation script (e.g., "compile:client") — e.g., "compile:all": "pnpm compile:openapi && pnpm compile:v3 && pnpm compile:client" — so consumers can run a single command instead of the trimmed "compile".api/v3/handlers/llmcost/list_overrides.go (1)
87-91: Please keep this request dump out of the happy path.This turns a diagnostic log into a possible request failure, and it emits an info log on every list call.
💡 Suggested tweak
- j, err := json.Marshal(req) - if err != nil { - return req, err - } - slog.Info("req", "req", string(j)) + if j, err := json.Marshal(req); err == nil { + slog.Debug("list llm cost overrides request", "req", string(j)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/llmcost/list_overrides.go` around lines 87 - 91, The current code marshals the request and returns an error if json.Marshal fails, then logs an info on every call (j, err := json.Marshal(req) ... slog.Info("req", "req", string(j))). Change this so request dumping is off the happy path: make the dump conditional at a debug level (or gated by a diagnostic flag) and do not propagate json.Marshal errors to the caller—if marshaling fails, log a debug/warn about the marshal failure and continue without returning the error; replace the info-level slog.Info("req", ...) with a debug-level log or gated logging so normal list_overrides handling (and function(s) that call it) isn’t impacted by logging failures.api/v3/request/cursor.go (1)
73-81: Usebase64.RawURLEncodinginstead ofStdEncoding+QueryEscapeto keep cursors opaque.The current approach base64-encodes with
StdEncoding, then wraps the result inurl.QueryEscape. This embeds URL-escape characters (%2B,%3D) directly in the string, making the cursor carry encoding state. While this works with standard HTTP libraries (which auto-unescape query params), it's fragile and couples the cursor format to URL mechanics.
base64.RawURLEncodingavoids problematic characters (+,/,=) entirely, so no QueryEscape is needed. The cursor would be truly opaque and wouldn't depend on how clients handle URL encoding.This applies to both
EncodeCursor(lines 73–81) anddecodeCursorAfterQueryUnescape(lines 103–109).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/cursor.go` around lines 73 - 81, Replace the current StdEncoding + url.QueryEscape pattern in EncodeCursor and the corresponding QueryUnescape + StdEncoding decode in decodeCursorAfterQueryUnescape with base64.RawURLEncoding so cursors avoid +/=/ characters and remain opaque; specifically, in EncodeCursor use base64.RawURLEncoding.EncodeToString on the raw bytes and drop url.QueryEscape, and in decodeCursorAfterQueryUnescape stop relying on query-unescape and instead call base64.RawURLEncoding.DecodeString on the incoming cursor string (handle and propagate decode errors as before).api/v3/request/aip_pagination.go (1)
99-104: Minor: redundant variable initialization.
coefis declared and then immediately assigned on the next line. Could be simplified.Suggested simplification
- var coef int - coef = int(p.Number) - 1 + coef := p.Number - 1 if coef < 0 { coef = 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_pagination.go` around lines 99 - 104, The local variable `coef` is declared then immediately assigned; simplify by combining declaration and assignment or eliminate `coef` entirely by computing the offset inline. Update the logic around `coef` in the function handling pagination (the code that reads `p.Number`, `p.Size` and sets `p.Offset`) to either declare `coef := int(p.Number) - 1` and keep the clamp to zero, or replace uses with `p.Offset = max(0, int(p.Number)-1) * p.Size` equivalent so the redundant two-step initialization is removed.api/spec/src/v3/llmcost/operations.tsp (1)
20-20: Clean up commented-out annotation.This commented
@useRefline should either be removed if not needed, or uncommented if it's intended. Leaving it creates confusion about the intended approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/llmcost/operations.tsp` at line 20, The commented-out annotation line //@useRef("../../../../common/definitions/aip_filters.yaml#/components/schemas/StringFieldFilter") should be cleaned up in operations.tsp: either remove the commented line if the reference is not required, or uncomment it (delete the leading //) if you intend to include the `@useRef` to the StringFieldFilter schema; update the file accordingly so the annotation state is explicit and not left commented.api/v3/oasmiddleware/validator.go (2)
76-103: Workaround for libopenapi issue#83is well-documented.The comment explaining why this filter exists is helpful. The string matching is a bit fragile (case-insensitive checks on error messages), but it's a reasonable workaround for a known library limitation.
Consider adding a TODO to remove this when the upstream issue is fixed.
Consider adding tracking comment
// filterQueryParamErrors removes validation errors for multi-level nested deepObject // query params. libopenapi-validator has a known issue: it works for objects with // depth of one but fails for nested objects (e.g. filter[provider][eq]=x). // See https://github.com/pb33f/libopenapi-validator/issues/83 +// TODO: Remove this workaround once the upstream issue is resolved. func filterQueryParamErrors(errs []*validatorerrors.ValidationError) []*validatorerrors.ValidationError {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/oasmiddleware/validator.go` around lines 76 - 103, In filterQueryParamErrors add a one-line TODO comment above the function (or at top of the function) noting this is a temporary workaround for libopenapi-validator issue `#83` with a link to https://github.com/pb33f/libopenapi-validator/issues/83 and a note to remove this filter once the upstream bug is fixed (optionally include the date or current dependency version), so future maintainers can track and remove the workaround when the upstream fix lands.
60-74: Spec patching approach works but has a subtle edge case.If the spec is YAML and contains features that don't round-trip cleanly through JSON (like YAML anchors/aliases), this could cause issues. Probably fine for OpenAPI specs in practice, but worth being aware of.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/oasmiddleware/validator.go` around lines 60 - 74, The patchSpecServers function currently unmarshals specBytes first as JSON and falls back to YAML but always returns json.Marshal(spec), which can lose YAML-specific features (anchors/aliases); update patchSpecServers to detect which format was used (track whether json.Unmarshal succeeded or yaml.Unmarshal was used), modify the spec map as it does now, and then marshal back to the original format (use json.Marshal when JSON was parsed and yaml.Marshal when YAML was parsed) so the function returns the same dialect it received; refer to symbols specBytes, baseURL, spec, json.Unmarshal/json.Marshal and yaml.Unmarshal/yaml.Marshal to locate and change the logic.api/v3/handlers/llmcost/list_prices.go (2)
59-61: Minor inconsistency: sort fields don't match the API spec.The API spec (operations.tsp lines 94-98) documents
provider.idandmodel.idas sort fields, but the authorized sorts here use those exact names. However, the spec also saysmodel.idis the default - you might want to set a default sort inGetAipAttributesto match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/llmcost/list_prices.go` around lines 59 - 61, The authorized sort list in listPricesAuthorizedSorts currently includes "provider.id" and "model.id" but there is no corresponding default sort set to match the API spec; update the GetAipAttributes logic to set the default sort to "model.id" (or ensure it uses model.id when no sort provided) so the default behavior aligns with the operations.tsp spec, and keep listPricesAuthorizedSorts as-is to allow the same fields.
82-85: Consider letting the AIP layer handle this default.This manual check for
pageNumber < 1duplicates logic that could live inextractPagination. If the AIP layer already guarantees a valid page number, this becomes dead code. If not, centralizing it there would reduce handler boilerplate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/llmcost/list_prices.go` around lines 82 - 85, The handler currently guards against invalid page numbers by checking attrs.Pagination.Number and clamping values in the list prices handler (pageNumber := attrs.Pagination.Number; if pageNumber < 1 { pageNumber = 1 }), which duplicates concern that should be centralized; instead, move this defaulting/validation into extractPagination so attrs.Pagination.Number is guaranteed to be >=1 (or have a documented default) and remove the manual clamp in the llmcost list_prices handler, or if you prefer to keep handler-safe behavior, update extractPagination to explicitly validate and set the default and adjust any callers to rely on it (reference extractPagination and attrs.Pagination.Number/list_prices handler).api/v3/request/aip_filter.go (2)
184-190: Consider handling empty values in comma split.If someone passes
filter[field][oeq]=a,,b, this will produce["a", "", "b"]with an empty string in the middle. Depending on your use case, you might want to filter those out.Optional fix
func parseMultipleStringValues(strValue string) []string { var out []string for _, v := range strings.Split(strValue, ",") { - out = append(out, strings.TrimSpace(v)) + trimmed := strings.TrimSpace(v) + if trimmed != "" { + out = append(out, trimmed) + } } return out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_filter.go` around lines 184 - 190, The parseMultipleStringValues function currently returns empty entries when the input contains consecutive commas (e.g., "a,,b"); update parseMultipleStringValues to trim each split element and skip adding any empty strings (after TrimSpace) to the output slice so the result for "a,,b" becomes ["a","b"]; ensure it still returns an empty slice for inputs that yield no non-empty tokens and reference parseMultipleStringValues when locating the change.
110-124: Complex authorization logic could use a comment.The dot-filter authorization has some subtle rules (allowing
known_field.unknown_keypatterns, forbidding whole-field use for dot filters). A brief comment explaining the intent would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_filter.go` around lines 110 - 124, Add a concise inline comment above the authorization block in aip_filter.go explaining the dot-filter rules: when c.authorizedFilters is present and o.Name contains a dot, we allow patterns like known_field.unknown_key by looking up parts[0] in c.authorizedFilters, fall back to the full o.Name for the edge case where only the full field is allowed, and require the AIPFilterOption.DotFilter flag to be true; otherwise (when o.Name has no dot) we require DotFilter to be false to forbid whole-field usage for dot filters. Reference the variables c.authorizedFilters, o.Name, parts (from strings.SplitN), AIPFilterOption and its DotFilter field, and the ok boolean so future readers understand the lookup and boolean composition logic.api/v3/request/aip_sort.go (2)
32-36: Invalid sort order values default to ascending silently.If someone passes
sort=id foo(invalid order), it defaults to ascending without warning. This might be intentional for flexibility, but worth noting if you want stricter validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_sort.go` around lines 32 - 36, The current parsing silently defaults invalid order strings to ascending; update the parsing in the sort parsing routine so that when parts length > 1 you explicitly validate the parsed SortOrder (using the SortOrder type and constants SortOrderAsc and SortOrderDesc) and handle invalid values instead of silently accepting them—either return a validation error to the caller or skip/ignore the order and report it (do not assign to sortBy.Order) so invalid inputs like "sort=id foo" are not treated as ascending without notice.
38-40: Unauthorized sort fields are silently dropped.If a user provides a sort field that isn't authorized, it just gets ignored without any feedback. Depending on how strict you want the API to be, you might want to return an error in strict mode (similar to how
extractFilterhandles unauthorized filters).Possible approach
if isAuthorizedSort(sortBy.Field, c) { out = append(out, sortBy) + } else if c.strictMode { + // Return error for unauthorized sort field }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_sort.go` around lines 38 - 40, The code currently drops unauthorized sort fields silently; change this to return an explicit error like extractFilter does: when iterating sort entries, call isAuthorizedSort(sortBy.Field, c) and if it returns false, return a descriptive error (e.g., "unauthorized sort field: <field>") instead of appending or skipping; update the enclosing function (the sort-parsing function where this snippet appears) to propagate that error to callers so clients receive a 4xx response in strict mode or enable a configurable behavior flag to optionally fall back to ignoring unauthorized sorts.api/v3/request/aip.go (1)
47-80: Consider centralizing the dotted-name remap logic.The filter and sort branches are doing the same remap work right now, so any future tweak has to stay in sync in two places. A tiny helper would make this a lot easier to maintain.
♻️ Small refactor sketch
+func remapField(name string, mappedAttributes map[string]string) string { + if mapped, ok := mappedAttributes[name]; ok { + return mapped + } + + parts := strings.SplitN(name, ".", 2) + if mapped, ok := mappedAttributes[parts[0]]; ok && len(parts) == 2 { + return mapped + "." + parts[1] + } + + return name +} + func RemapAipAttributes(attrs *AipAttributes, mappedAttributes map[string]string) { if attrs.Filters != nil { for k, f := range attrs.Filters { - if _, ok := mappedAttributes[f.Name]; ok { - attrs.Filters[k].Name = mappedAttributes[f.Name] - continue - } - parts := strings.SplitN(f.Name, ".", 2) // allow filters[known_custom_field.unknown_key] - if _, ok := mappedAttributes[parts[0]]; ok { - if len(parts) == 2 { - attrs.Filters[k].Name = mappedAttributes[parts[0]] + "." + parts[1] - } else { - attrs.Filters[k].Name = mappedAttributes[f.Name] - } - } + attrs.Filters[k].Name = remapField(f.Name, mappedAttributes) } } if attrs.Sorts != nil { for k, s := range attrs.Sorts { - if _, ok := mappedAttributes[s.Field]; ok { - attrs.Sorts[k].Field = mappedAttributes[s.Field] - continue - } - parts := strings.SplitN(s.Field, ".", 2) // allow filters[known_custom_field.unknown_key] - if _, ok := mappedAttributes[parts[0]]; ok { - if len(parts) == 2 { - attrs.Sorts[k].Field = mappedAttributes[parts[0]] + "." + parts[1] - } else { - attrs.Sorts[k].Field = mappedAttributes[s.Field] - } - } + attrs.Sorts[k].Field = remapField(s.Field, mappedAttributes) } } }As per coding guidelines,
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip.go` around lines 47 - 80, Remap the duplicated dotted-name logic into a small helper used by RemapAipAttributes: create a function (e.g., remapDottedName(name string, mappedAttributes map[string]string) (string, bool)) that checks direct mapping and then splits on "." once to remap the left-most segment and rejoin when appropriate; replace the inline logic in the Filters loop (attrs.Filters[k].Name) and the Sorts loop (attrs.Sorts[k].Field) to call this helper and only assign when the helper returns changed=true, preserving existing behavior for nil slices and unknown keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/spec/src/v3/llmcost/operations.tsp`:
- Around line 27-30: Replace the placeholder doc comment "aasd" for the oeq
field with a meaningful description; update the JSDoc above the oeq?: string;
property (in operations.tsp) to explain its purpose similar to other fields —
e.g., "The field must match one of the provided values (OR operation)" — so
consumers understand that oeq performs an OR-style match on provided values.
- Around line 42-45: Replace the placeholder doc comment "asd" above the
ocontains?: string; property with a clear, specific description of what
ocontains represents (e.g., its purpose, expected format/values, and that it is
optional), so callers and generated docs understand its semantics; update the
JSDoc for the ocontains property in operations.tsp to explicitly state intent,
examples or allowed values if applicable, and any constraints.
In `@api/v3/api.go`:
- Around line 26-35: The ParseEq method on FilterString currently allocates a
new FilterString when the receiver is nil but only reassigns the local receiver,
so callers like the pointer fields Provider/ModelId never get updated; change
the API so the initializer is propagated back to the caller—for example modify
FilterString.ParseEq to return the (possibly newly allocated) *FilterString and
update call sites (e.g., Provider = Provider.ParseEq(...), ModelId =
ModelId.ParseEq(...)), or alternatively provide a helper function that accepts a
**FilterString and initializes/sets it in-place; update all usages to use the
returned value or the helper so parsed values are not lost.
In `@api/v3/request/aip_filter.go`:
- Around line 208-227: The code that extracts the operation from qsRest (using
start := strings.IndexRune(qsRest, '['); end := strings.IndexRune(qsRest, ']');
op := qsRest[start+1:end]) can panic if a closing ']' is missing; update the
logic in the aip_filter.go block (the qsRest/start/end/op handling inside the
function that builds QueryFilter) to validate both start and end are >= 0 and
that end > start before slicing, and if the check fails return a BadRequest
(similar to the existing apierrors.NewBadRequestError) indicating a malformed
filter operation; ensure you still consult filterMap for valid ops and set
o.Filter when appropriate.
In `@api/v3/request/aip_pagination.go`:
- Around line 80-92: The error-handling block around parsing pageNumber
incorrectly checks pageNumber < 0 inside the err != nil branch (so pageNumber
will be zero), causing incorrect behavior; update the logic in aip_pagination.go
so that parse errors are handled when err != nil (return NewBadRequestError when
c.strictMode or always include parse error) and move the negative-value
validation into the successful-parse path (after converting pageNumber) — adjust
the conditionals around pageNumber, c.strictMode, and the
apierrors.NewBadRequestError call (referencing variables/functions: pageNumber,
c.strictMode, PageNumberQuery, apierrors.NewBadRequestError) so parse failures
and negative-value checks are evaluated correctly and independently.
- Around line 49-62: The parse failure branch incorrectly checks pageSize < 0
(which is zero on parse error); update the logic in the pagination parsing flow
in aip_pagination.go so that on strconv.ParseInt error you only consider
c.strictMode (return apierrors.NewBadRequestError with PageSizeQuery) or fall
back to c.defaultPageSize, and move the negative-value check to the
successful-parse path so that after parsing you validate if pageSize < 0 and
then either return the same bad-request (when c.strictMode) or set pageSize =
int64(c.defaultPageSize); reference variables/functions: pageSize, c.strictMode,
c.defaultPageSize, PageSizeQuery, apierrors.NewBadRequestError,
apierrors.InvalidParameters.
In `@api/v3/request/aip.go`:
- Around line 151-161: Rename the public option function From
WithDefaultPageSizeDefault to a clearer name like WithDefaultPageSize (update
all callers) and add validation to ignore non-positive values: inside the
returned closure for the AipParseOption ensure the incoming value > 0 before
assigning to config.defaultPageSize; do the same validation change for the other
page-size setter referenced (the other setter that writes config.defaultPageSize
/ page size) so neither accepts non-positive sizes and both become harder to
misuse.
- Around line 10-12: Update the exported Go doc comments to match the current
API: revise GetAipAttributes' comment to remove or correct the AipSetStrictMode
reference and describe the actual behavior/error type returned (BaseApiError),
change WithOffsetPagination's comment to no longer start with
WithCursorPagination and describe offset behavior, fix WithAuthorizedSorts so it
doesn't point to itself and instead explains how authorized sorts are
provided/used, and update ValidationFunc's comment to state it returns an error
(not a bool/string contract); ensure each comment mentions the correct
function/signature (GetAipAttributes, AipSetStrictMode, WithOffsetPagination,
WithCursorPagination, WithAuthorizedSorts, ValidationFunc) and accurately
reflects current return types and side effects so GoDoc is correct.
In `@Makefile`:
- Line 32: The Makefile target "gen-api" currently only depends on
update-openapi but its help/description still reads "Generate API and SDKs", so
update the Makefile to make the target and help text consistent: either restore
SDK generation by adding the appropriate dependency (e.g., include the existing
"generate-javascript-sdk" or equivalent target to the "gen-api" prerequisites)
or change the target's description string from "Generate API and SDKs" to
"Generate API" (and update any help output and documentation references). Locate
the "gen-api" target and the associated help text/description in the Makefile
and apply the chosen fix so local workflows accurately reflect whether SDKs are
regenerated.
---
Outside diff comments:
In `@api/v3/handlers/llmcost/convert.go`:
- Around line 206-225: filterSingleStringToDomain currently only copies Eq, Neq,
and Contains from api.FilterString into filters.StringFilter so incoming oeq and
ocontains operators are lost and the filter becomes empty; update the function
to also copy the oeq and ocontains fields from the input api.FilterString into
the resulting filters.StringFilter (preserving the same field names used on the
domain type), then let the existing Validate() and IsEmpty() logic run as
before.
---
Nitpick comments:
In `@api/spec/package.json`:
- Line 6: Add an explicit full compile alias to package.json to run the OpenAPI
spec, v3, and client generation in one command: add a new script "compile:all"
(or "compile:full") that chains the existing "compile:openapi", "compile:v3",
and the client generation script (e.g., "compile:client") — e.g., "compile:all":
"pnpm compile:openapi && pnpm compile:v3 && pnpm compile:client" — so consumers
can run a single command instead of the trimmed "compile".
In `@api/spec/src/v3/llmcost/operations.tsp`:
- Line 20: The commented-out annotation line
//@useRef("../../../../common/definitions/aip_filters.yaml#/components/schemas/StringFieldFilter")
should be cleaned up in operations.tsp: either remove the commented line if the
reference is not required, or uncomment it (delete the leading //) if you intend
to include the `@useRef` to the StringFieldFilter schema; update the file
accordingly so the annotation state is explicit and not left commented.
In `@api/v3/handlers/llmcost/list_overrides.go`:
- Around line 87-91: The current code marshals the request and returns an error
if json.Marshal fails, then logs an info on every call (j, err :=
json.Marshal(req) ... slog.Info("req", "req", string(j))). Change this so
request dumping is off the happy path: make the dump conditional at a debug
level (or gated by a diagnostic flag) and do not propagate json.Marshal errors
to the caller—if marshaling fails, log a debug/warn about the marshal failure
and continue without returning the error; replace the info-level
slog.Info("req", ...) with a debug-level log or gated logging so normal
list_overrides handling (and function(s) that call it) isn’t impacted by logging
failures.
In `@api/v3/handlers/llmcost/list_prices.go`:
- Around line 59-61: The authorized sort list in listPricesAuthorizedSorts
currently includes "provider.id" and "model.id" but there is no corresponding
default sort set to match the API spec; update the GetAipAttributes logic to set
the default sort to "model.id" (or ensure it uses model.id when no sort
provided) so the default behavior aligns with the operations.tsp spec, and keep
listPricesAuthorizedSorts as-is to allow the same fields.
- Around line 82-85: The handler currently guards against invalid page numbers
by checking attrs.Pagination.Number and clamping values in the list prices
handler (pageNumber := attrs.Pagination.Number; if pageNumber < 1 { pageNumber =
1 }), which duplicates concern that should be centralized; instead, move this
defaulting/validation into extractPagination so attrs.Pagination.Number is
guaranteed to be >=1 (or have a documented default) and remove the manual clamp
in the llmcost list_prices handler, or if you prefer to keep handler-safe
behavior, update extractPagination to explicitly validate and set the default
and adjust any callers to rely on it (reference extractPagination and
attrs.Pagination.Number/list_prices handler).
In `@api/v3/oasmiddleware/validator.go`:
- Around line 76-103: In filterQueryParamErrors add a one-line TODO comment
above the function (or at top of the function) noting this is a temporary
workaround for libopenapi-validator issue `#83` with a link to
https://github.com/pb33f/libopenapi-validator/issues/83 and a note to remove
this filter once the upstream bug is fixed (optionally include the date or
current dependency version), so future maintainers can track and remove the
workaround when the upstream fix lands.
- Around line 60-74: The patchSpecServers function currently unmarshals
specBytes first as JSON and falls back to YAML but always returns
json.Marshal(spec), which can lose YAML-specific features (anchors/aliases);
update patchSpecServers to detect which format was used (track whether
json.Unmarshal succeeded or yaml.Unmarshal was used), modify the spec map as it
does now, and then marshal back to the original format (use json.Marshal when
JSON was parsed and yaml.Marshal when YAML was parsed) so the function returns
the same dialect it received; refer to symbols specBytes, baseURL, spec,
json.Unmarshal/json.Marshal and yaml.Unmarshal/yaml.Marshal to locate and change
the logic.
In `@api/v3/request/aip_filter.go`:
- Around line 184-190: The parseMultipleStringValues function currently returns
empty entries when the input contains consecutive commas (e.g., "a,,b"); update
parseMultipleStringValues to trim each split element and skip adding any empty
strings (after TrimSpace) to the output slice so the result for "a,,b" becomes
["a","b"]; ensure it still returns an empty slice for inputs that yield no
non-empty tokens and reference parseMultipleStringValues when locating the
change.
- Around line 110-124: Add a concise inline comment above the authorization
block in aip_filter.go explaining the dot-filter rules: when c.authorizedFilters
is present and o.Name contains a dot, we allow patterns like
known_field.unknown_key by looking up parts[0] in c.authorizedFilters, fall back
to the full o.Name for the edge case where only the full field is allowed, and
require the AIPFilterOption.DotFilter flag to be true; otherwise (when o.Name
has no dot) we require DotFilter to be false to forbid whole-field usage for dot
filters. Reference the variables c.authorizedFilters, o.Name, parts (from
strings.SplitN), AIPFilterOption and its DotFilter field, and the ok boolean so
future readers understand the lookup and boolean composition logic.
In `@api/v3/request/aip_pagination.go`:
- Around line 99-104: The local variable `coef` is declared then immediately
assigned; simplify by combining declaration and assignment or eliminate `coef`
entirely by computing the offset inline. Update the logic around `coef` in the
function handling pagination (the code that reads `p.Number`, `p.Size` and sets
`p.Offset`) to either declare `coef := int(p.Number) - 1` and keep the clamp to
zero, or replace uses with `p.Offset = max(0, int(p.Number)-1) * p.Size`
equivalent so the redundant two-step initialization is removed.
In `@api/v3/request/aip_sort.go`:
- Around line 32-36: The current parsing silently defaults invalid order strings
to ascending; update the parsing in the sort parsing routine so that when parts
length > 1 you explicitly validate the parsed SortOrder (using the SortOrder
type and constants SortOrderAsc and SortOrderDesc) and handle invalid values
instead of silently accepting them—either return a validation error to the
caller or skip/ignore the order and report it (do not assign to sortBy.Order) so
invalid inputs like "sort=id foo" are not treated as ascending without notice.
- Around line 38-40: The code currently drops unauthorized sort fields silently;
change this to return an explicit error like extractFilter does: when iterating
sort entries, call isAuthorizedSort(sortBy.Field, c) and if it returns false,
return a descriptive error (e.g., "unauthorized sort field: <field>") instead of
appending or skipping; update the enclosing function (the sort-parsing function
where this snippet appears) to propagate that error to callers so clients
receive a 4xx response in strict mode or enable a configurable behavior flag to
optionally fall back to ignoring unauthorized sorts.
In `@api/v3/request/aip.go`:
- Around line 47-80: Remap the duplicated dotted-name logic into a small helper
used by RemapAipAttributes: create a function (e.g., remapDottedName(name
string, mappedAttributes map[string]string) (string, bool)) that checks direct
mapping and then splits on "." once to remap the left-most segment and rejoin
when appropriate; replace the inline logic in the Filters loop
(attrs.Filters[k].Name) and the Sorts loop (attrs.Sorts[k].Field) to call this
helper and only assign when the helper returns changed=true, preserving existing
behavior for nil slices and unknown keys.
In `@api/v3/request/cursor.go`:
- Around line 73-81: Replace the current StdEncoding + url.QueryEscape pattern
in EncodeCursor and the corresponding QueryUnescape + StdEncoding decode in
decodeCursorAfterQueryUnescape with base64.RawURLEncoding so cursors avoid +/=/
characters and remain opaque; specifically, in EncodeCursor use
base64.RawURLEncoding.EncodeToString on the raw bytes and drop url.QueryEscape,
and in decodeCursorAfterQueryUnescape stop relying on query-unescape and instead
call base64.RawURLEncoding.DecodeString on the incoming cursor string (handle
and propagate decode errors as before).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 916c275f-8e6d-4250-9438-2f28b308a108
⛔ Files ignored due to path filters (3)
api/v3/openapi.yamlis excluded by!**/openapi.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (21)
Makefileapi/spec/package.jsonapi/spec/src/v3/llmcost/operations.tspapi/v3/api.gen.goapi/v3/api.goapi/v3/filters/filter.goapi/v3/handlers/llmcost/convert.goapi/v3/handlers/llmcost/list_overrides.goapi/v3/handlers/llmcost/list_prices.goapi/v3/oasmiddleware/decoder.goapi/v3/oasmiddleware/error.goapi/v3/oasmiddleware/hook.goapi/v3/oasmiddleware/router.goapi/v3/oasmiddleware/validator.goapi/v3/request/aip.goapi/v3/request/aip_filter.goapi/v3/request/aip_pagination.goapi/v3/request/aip_sort.goapi/v3/request/cursor.goapi/v3/server/server.goapi/v3/spec.go
💤 Files with no reviewable changes (2)
- api/v3/oasmiddleware/decoder.go
- api/v3/oasmiddleware/router.go
Comment on lines
+26
to
+35
| func (f *FilterString) ParseEq(name string, r *http.Request) { | ||
| if f == nil { | ||
| f = &FilterString{} | ||
| } | ||
| query := r.URL.Query() | ||
| eq := query.Get(name) | ||
| if eq != "" { | ||
| f.Eq = &eq | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the helper and the current call sites.
sed -n '20,40p' api/v3/api.go
rg -n --type=go -C2 '\.ParseEq\(' api/v3Repository: openmeterio/openmeter
Length of output: 1989
🏁 Script executed:
# Find the Filter type definition
fd -t f "\.go$" api/v3 -x grep -l "type.*Filter" {} \;
# Look for the structure that contains Provider, ModelId, etc.
rg -A 20 "type.*Filter.*struct" api/v3 --type=go | head -100
# Check how params.Filter is initialized
rg -B 5 -A 5 "params.Filter\s*=" api/v3/handlers/llmcost --type=goRepository: openmeterio/openmeter
Length of output: 5053
🏁 Script executed:
# Find the Filter type that contains Provider, ModelId, ModelName, Currency
rg -B 5 "Provider.*Filter" api/v3/handlers/llmcost/list_overrides.go --type=go | head -30
# Search for the struct definition with these fields
rg "type.*struct" api/v3 --type=go -A 10 | grep -A 15 "Provider"Repository: openmeterio/openmeter
Length of output: 1771
🏁 Script executed:
# Find the handler function signature in list_overrides.go
rg -B 20 "params.Filter.Provider.ParseEq" api/v3/handlers/llmcost/list_overrides.go --type=go | head -50
# Look for the type definition of the params variable
rg "type.*Override.*struct|type.*ListOverrides.*struct" api/v3 --type=go -A 20Repository: openmeterio/openmeter
Length of output: 2697
🏁 Script executed:
# Find the ListLLMCostPricesParamsFilter struct definition
rg "type ListLLMCostPricesParamsFilter struct" api/v3 --type=go -A 20Repository: openmeterio/openmeter
Length of output: 1320
ParseEq doesn't update the caller's field when it starts nil.
These filter fields (Provider, ModelId, etc.) are pointers to FilterString, so they can be nil. When you call f.ParseEq(...) on a nil pointer, the method rebinds the local f variable but doesn't update the original field. The freshly allocated FilterString gets discarded, and any parsed value is lost. Flat filter[provider]={value} params get dropped unless the field was already initialized elsewhere.
Consider initializing these filter fields upfront, returning the initialized struct from ParseEq, or using a helper that directly updates the field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/api.go` around lines 26 - 35, The ParseEq method on FilterString
currently allocates a new FilterString when the receiver is nil but only
reassigns the local receiver, so callers like the pointer fields
Provider/ModelId never get updated; change the API so the initializer is
propagated back to the caller—for example modify FilterString.ParseEq to return
the (possibly newly allocated) *FilterString and update call sites (e.g.,
Provider = Provider.ParseEq(...), ModelId = ModelId.ParseEq(...)), or
alternatively provide a helper function that accepts a **FilterString and
initializes/sets it in-place; update all usages to use the returned value or the
helper so parsed values are not lost.
api/v3/request/aip_filter.go
Outdated
Comment on lines
+208
to
+227
| if len(qsRest) > 0 { | ||
| start := strings.IndexRune(qsRest, '[') | ||
| end := strings.IndexRune(qsRest, ']') | ||
| op := qsRest[start+1 : end] | ||
| if len(op) > 0 { | ||
| if queryOp, ok := filterMap[op]; ok { | ||
| o.Filter = queryOp | ||
| } else { | ||
| return QueryFilter{}, apierrors.NewBadRequestError(ctx, ErrUnallowedFilterColumn, | ||
| apierrors.InvalidParameters{ | ||
| apierrors.InvalidParameter{ | ||
| Field: filter, | ||
| Reason: fmt.Sprintf("invalid operation '%s' on filter", op), | ||
| Source: apierrors.InvalidParamSourceQuery, | ||
| Rule: "unauthorized filter on column", | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Potential panic: missing bounds check on closing bracket.
If qsRest contains [ but no ] (e.g., malformed query like filter[field][eq), end will be -1 and line 211 will panic with an out-of-bounds slice access: qsRest[start+1:end] becomes qsRest[1:-1].
Suggested fix
if len(qsRest) > 0 {
start := strings.IndexRune(qsRest, '[')
end := strings.IndexRune(qsRest, ']')
+ if start == -1 || end == -1 || end <= start {
+ return o, nil // Malformed filter syntax, skip
+ }
op := qsRest[start+1 : end]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(qsRest) > 0 { | |
| start := strings.IndexRune(qsRest, '[') | |
| end := strings.IndexRune(qsRest, ']') | |
| op := qsRest[start+1 : end] | |
| if len(op) > 0 { | |
| if queryOp, ok := filterMap[op]; ok { | |
| o.Filter = queryOp | |
| } else { | |
| return QueryFilter{}, apierrors.NewBadRequestError(ctx, ErrUnallowedFilterColumn, | |
| apierrors.InvalidParameters{ | |
| apierrors.InvalidParameter{ | |
| Field: filter, | |
| Reason: fmt.Sprintf("invalid operation '%s' on filter", op), | |
| Source: apierrors.InvalidParamSourceQuery, | |
| Rule: "unauthorized filter on column", | |
| }, | |
| }) | |
| } | |
| } | |
| } | |
| if len(qsRest) > 0 { | |
| start := strings.IndexRune(qsRest, '[') | |
| end := strings.IndexRune(qsRest, ']') | |
| if start == -1 || end == -1 || end <= start { | |
| return o, nil // Malformed filter syntax, skip | |
| } | |
| op := qsRest[start+1 : end] | |
| if len(op) > 0 { | |
| if queryOp, ok := filterMap[op]; ok { | |
| o.Filter = queryOp | |
| } else { | |
| return QueryFilter{}, apierrors.NewBadRequestError(ctx, ErrUnallowedFilterColumn, | |
| apierrors.InvalidParameters{ | |
| apierrors.InvalidParameter{ | |
| Field: filter, | |
| Reason: fmt.Sprintf("invalid operation '%s' on filter", op), | |
| Source: apierrors.InvalidParamSourceQuery, | |
| Rule: "unauthorized filter on column", | |
| }, | |
| }) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/request/aip_filter.go` around lines 208 - 227, The code that extracts
the operation from qsRest (using start := strings.IndexRune(qsRest, '['); end :=
strings.IndexRune(qsRest, ']'); op := qsRest[start+1:end]) can panic if a
closing ']' is missing; update the logic in the aip_filter.go block (the
qsRest/start/end/op handling inside the function that builds QueryFilter) to
validate both start and end are >= 0 and that end > start before slicing, and if
the check fails return a BadRequest (similar to the existing
apierrors.NewBadRequestError) indicating a malformed filter operation; ensure
you still consult filterMap for valid ops and set o.Filter when appropriate.
mark-vass-konghq
force-pushed
the
feat/filter-poc
branch
from
7e713a8 to
1edb576
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
api/v3/request/aip_pagination.go (2)
80-95:⚠️ Potential issue | 🟡 MinorSame branch issue for
page[number]parsing.At Line 81,
pageNumber < 0is inside the parse-error path, so strict-mode negative values are never rejected after a successful parse.💡 Suggested fix
pageNumber, err := strconv.ParseInt(strPageNumber, 10, 16) if err != nil { - if c.strictMode || pageNumber < 0 { + if c.strictMode { return p, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ apierrors.InvalidParameter{ Field: PageNumberQuery, Reason: "unable to parse query field", Source: apierrors.InvalidParamSourceQuery, Rule: "page number should be a positive integer", }, }) } + } else if pageNumber < 0 { + if c.strictMode { + return p, apierrors.NewBadRequestError(ctx, fmt.Errorf("negative page number"), + apierrors.InvalidParameters{ + apierrors.InvalidParameter{ + Field: PageNumberQuery, + Reason: "unable to parse query field", + Source: apierrors.InvalidParamSourceQuery, + Rule: "page number should be a positive integer", + }, + }) + } + pageNumber = DefaultPaginationNumber }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_pagination.go` around lines 80 - 95, The parse-error branch for page[number] currently checks pageNumber < 0 only when parsing failed, so negative values that parsed successfully bypass strict-mode rejection; update the logic in the pagination parsing (the code handling pageNumber and c.strictMode) to validate negative values after a successful parse as well: if c.strictMode and pageNumber < 0 return the same apierrors.NewBadRequestError (using PageNumberQuery and the same InvalidParameters/InvalidParameter payload), and keep the existing fallback that sets pageNumber = DefaultPaginationNumber when pageNumber < 1.
49-66:⚠️ Potential issue | 🟡 Minor
page[size]validation is wired to the wrong branch.At Line 50,
pageSize < 0is checked insideerr != nil. Negative values (-1) parse fine, so strict-mode never rejects them there. Also Line 65 falls back toDefaultPaginationSizeinstead ofc.defaultPageSize.💡 Suggested fix
pageSize, err := strconv.ParseInt(strPageSize, 10, 16) if err != nil { - if c.strictMode || pageSize < 0 { + if c.strictMode { return p, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ apierrors.InvalidParameter{ Field: PageSizeQuery, Reason: "unable to parse query field", Source: apierrors.InvalidParamSourceQuery, Rule: "page size should be a positive integer", }, }) - } else { - pageSize = int64(c.defaultPageSize) } + pageSize = int64(c.defaultPageSize) + } else if pageSize < 0 { + if c.strictMode { + return p, apierrors.NewBadRequestError(ctx, fmt.Errorf("negative page size"), + apierrors.InvalidParameters{ + apierrors.InvalidParameter{ + Field: PageSizeQuery, + Reason: "unable to parse query field", + Source: apierrors.InvalidParamSourceQuery, + Rule: "page size should be a positive integer", + }, + }) + } + pageSize = int64(c.defaultPageSize) } if pageSize < 1 { - pageSize = DefaultPaginationSize + pageSize = int64(c.defaultPageSize) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_pagination.go` around lines 49 - 66, The page[size] validation currently checks pageSize < 0 inside the err != nil branch and falls back to DefaultPaginationSize; fix by separating parse-errors from value validation: when parsing fails (err != nil) return a BadRequest if c.strictMode, otherwise set pageSize = c.defaultPageSize; then independently validate the parsed pageSize: if pageSize < 0 then return BadRequest when c.strictMode or set pageSize = c.defaultPageSize when lenient; finally, when pageSize < 1 use c.defaultPageSize (not DefaultPaginationSize). Update references to PageSizeQuery, c.strictMode, c.defaultPageSize, pageSize, and the BadRequest construction accordingly.api/v3/api.go (1)
26-35:⚠️ Potential issue | 🟠 Major
ParseEqloses values when the receiver starts nil.At Line 27-Line 29, a new
FilterStringis created, but only the localfis updated. Caller fields stay nil, so parsed flat eq values are silently dropped.💡 Suggested fix
-func (f *FilterString) ParseEq(name string, r *http.Request) { +func (f *FilterString) ParseEq(name string, r *http.Request) *FilterString { if f == nil { f = &FilterString{} } query := r.URL.Query() eq := query.Get(name) if eq != "" { f.Eq = &eq } + return f }And update call sites to assign back, e.g.:
params.Filter.Provider = params.Filter.Provider.ParseEq("filter[provider]", r)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.go` around lines 26 - 35, The ParseEq method on FilterString mutates a potentially nil receiver and loses parsed values; change its signature to return *FilterString (e.g. func (f *FilterString) ParseEq(name string, r *http.Request) *FilterString), inside the method if f==nil allocate f = &FilterString{}, set f.Eq from r.URL.Query().Get(name) as you already do, and then return f; update all call sites (e.g. params.Filter.Provider = params.Filter.Provider.ParseEq("filter[provider]", r)) to assign the returned *FilterString so parsed values are not discarded.api/spec/packages/aip/src/llmcost/operations.tsp (1)
27-30:⚠️ Potential issue | 🟡 MinorPlease replace the placeholder docs and call out the OR format.
oeqandocontainsstill render asaasd/asd, and these are the operators that accept comma-separated values. Right now the generated docs don't tell callers how to use either field.As per coding guidelines "The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user."
Also applies to: 42-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/llmcost/operations.tsp` around lines 27 - 30, Replace the placeholder JSDoc for the filter operator fields oeq and ocontains with clear documentation that these operators accept comma-separated values interpreted as an OR list (e.g., "a,b,c" matches a OR b OR c); update both occurrences (the oeq JSDoc around the first declaration and the ocontains JSDoc near the later declaration) to explicitly state the CSV format, trimming/whitespace handling, and that matching is performed as OR across the values to match the implementation.api/v3/request/aip_filter.go (1)
199-212:⚠️ Potential issue | 🔴 CriticalMalformed filter keys can still panic here.
If the second bracket pair is missing,
startorendgoes negative andqsRest[start+1:end]panics. Since this is user input, it should return the same BadRequest path as other invalid operators instead of crashing or silently skipping malformed syntax.🐛 Suggested guard
if len(qsRest) > 0 { start := strings.IndexRune(qsRest, '[') end := strings.IndexRune(qsRest, ']') + if start == -1 || end == -1 || end <= start { + return QueryFilter{}, apierrors.NewBadRequestError(ctx, ErrUnallowedFilterMethod, + apierrors.InvalidParameters{ + apierrors.InvalidParameter{ + Field: qs, + Reason: "malformed filter operation", + Source: apierrors.InvalidParamSourceQuery, + }, + }, + ) + } op := qsRest[start+1 : end]Based on learnings "Follow HTTP handler patterns and ValidationIssue conventions documented in the
/apiskill"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_filter.go` around lines 199 - 212, The code in the filter parsing section (variables qsRest, start, end, op and the logic setting o.Filter/o.Name) can panic when the second bracket pair is missing; add guards to validate start and end (ensure start >= 0, end >= 0 and end > start) before slicing qsRest[start+1:end], and if the validation fails return the same BadRequest/ValidationIssue response used elsewhere (do not silently skip or crash). Update the parsing branch that sets op and later uses it to return a ValidationIssue/BadRequest path for malformed operator syntax so behavior matches the existing handler conventions.
🧹 Nitpick comments (1)
api/v3/api.go (1)
19-23: Replace placeholder operator docs before they leak into API docs.
"Ocontains asd"and"Oeq aasd"are placeholder comments and end up in generated OpenAPI output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.go` around lines 19 - 23, The struct field comment placeholders for Ocontains and Oeq are leaking into generated API docs; update the doc comments above the fields Ocontains *string `json:"ocontains,omitempty"` and Oeq *string `json:"oeq,omitempty"` to meaningful descriptions (e.g., "Filter: contains substring" and "Filter: equals") or remove the comments entirely so OpenAPI output no longer contains the placeholder text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/spec/packages/aip/src/llmcost/operations.tsp`:
- Around line 54-68: Update the docs for the filter fields (provider, model_id,
model_name, currency) to show the supported shorthand syntax in addition to the
explicit operator form: explain that these fields accept string |
FilterSingleString and that a bare query like filter[provider]=openai (or
filter[model_id]=gpt-4, filter[model_name]=gpt, filter[currency]=USD) is
accepted as the eq shorthand equivalent to filter[provider][eq]=..., so add
examples and a short note about the shorthand next to each field declaration
(references: provider, model_id, model_name, currency, FilterSingleString).
In `@api/v3/oasmiddleware/error.go`:
- Around line 67-89: The switches on ve.ValidationType and ve.ValidationSubType
misclassify "cookie" by falling through to body; update both switch statements
to handle the "cookie" case and return the correct enum (e.g.,
apierrors.InvalidParamSourceCookie) for the "cookie" string, leaving the
existing fallthrough/default to body otherwise so other behavior is unchanged.
In `@api/v3/oasmiddleware/validator.go`:
- Around line 76-102: In filterQueryParamErrors, replace the broad
string-matching logic with checks on the structured ValidationError fields: only
suppress errors where e.ValidationType == "query" and e.ParameterName ==
"filter" (or other explicit parameter names you intend to exempt) and the error
indicates an object-schema schema-validation failure; keep all other errors. Use
the ValidationType and ParameterName fields from the ValidationError struct
(instead of matching e.Message/e.Reason) and only fall back to the existing
schema/object indicator if needed to confirm it's an object schema validation
error before continuing to skip that error.
In `@api/v3/request/aip_filter.go`:
- Around line 143-149: The InvalidParameter.Field in the BadRequest errors is
being set to the operator or value (via filterName(o.Filter) / o.Filter) instead
of the offending query key/column; update the error construction in the branches
that return apierrors.NewBadRequestError (the ones currently using
filterName(o.Filter) or o.Filter) to use the query string key or column
identifier (qs or o.Name) as the Field, keep Reason/Source/Rule the same, and
apply this change consistently for the other similar branches (the ones around
the other BadRequest returns currently using o.Filter or the value) so clients
can map strict-mode 400s back to the original request field.
In `@api/v3/request/cursor.go`:
- Around line 42-48: The xorText helper will panic when cipherKey is empty due
to division by zero in i%keyLen; change xorText to validate the key up-front (if
len(cipherKey) == 0) and return an error instead of panicking, e.g. update the
signature from xorText(cipherKey, input string) string to xorText(cipherKey,
input string) (string, error), perform the empty-key check and return a
descriptive error, and update all call sites to handle the error (propagate it
up the request path rather than allowing a panic).
- Around line 120-129: The cursor decoding path allows a version-only payload to
pass and produce an empty ID; update Cursor decoding in the block that uses
getVersionIdx and xorText so it rejects too-short or empty-ID cursors: ensure
the decoded payload has the expected minimum length before indexing (guard
against decoded being shorter than 2 when calling getVersionIdx and when reading
c.decoded[versionIdx]), and after removing the version character and running
xorText verify the resulting ID string is non-empty; if either check fails,
return ErrInvalidCursor. Reference symbols: Cursor, getVersionIdx,
cursorVersion, xorText, and ErrInvalidCursor.
---
Duplicate comments:
In `@api/spec/packages/aip/src/llmcost/operations.tsp`:
- Around line 27-30: Replace the placeholder JSDoc for the filter operator
fields oeq and ocontains with clear documentation that these operators accept
comma-separated values interpreted as an OR list (e.g., "a,b,c" matches a OR b
OR c); update both occurrences (the oeq JSDoc around the first declaration and
the ocontains JSDoc near the later declaration) to explicitly state the CSV
format, trimming/whitespace handling, and that matching is performed as OR
across the values to match the implementation.
In `@api/v3/api.go`:
- Around line 26-35: The ParseEq method on FilterString mutates a potentially
nil receiver and loses parsed values; change its signature to return
*FilterString (e.g. func (f *FilterString) ParseEq(name string, r *http.Request)
*FilterString), inside the method if f==nil allocate f = &FilterString{}, set
f.Eq from r.URL.Query().Get(name) as you already do, and then return f; update
all call sites (e.g. params.Filter.Provider =
params.Filter.Provider.ParseEq("filter[provider]", r)) to assign the returned
*FilterString so parsed values are not discarded.
In `@api/v3/request/aip_filter.go`:
- Around line 199-212: The code in the filter parsing section (variables qsRest,
start, end, op and the logic setting o.Filter/o.Name) can panic when the second
bracket pair is missing; add guards to validate start and end (ensure start >=
0, end >= 0 and end > start) before slicing qsRest[start+1:end], and if the
validation fails return the same BadRequest/ValidationIssue response used
elsewhere (do not silently skip or crash). Update the parsing branch that sets
op and later uses it to return a ValidationIssue/BadRequest path for malformed
operator syntax so behavior matches the existing handler conventions.
In `@api/v3/request/aip_pagination.go`:
- Around line 80-95: The parse-error branch for page[number] currently checks
pageNumber < 0 only when parsing failed, so negative values that parsed
successfully bypass strict-mode rejection; update the logic in the pagination
parsing (the code handling pageNumber and c.strictMode) to validate negative
values after a successful parse as well: if c.strictMode and pageNumber < 0
return the same apierrors.NewBadRequestError (using PageNumberQuery and the same
InvalidParameters/InvalidParameter payload), and keep the existing fallback that
sets pageNumber = DefaultPaginationNumber when pageNumber < 1.
- Around line 49-66: The page[size] validation currently checks pageSize < 0
inside the err != nil branch and falls back to DefaultPaginationSize; fix by
separating parse-errors from value validation: when parsing fails (err != nil)
return a BadRequest if c.strictMode, otherwise set pageSize = c.defaultPageSize;
then independently validate the parsed pageSize: if pageSize < 0 then return
BadRequest when c.strictMode or set pageSize = c.defaultPageSize when lenient;
finally, when pageSize < 1 use c.defaultPageSize (not DefaultPaginationSize).
Update references to PageSizeQuery, c.strictMode, c.defaultPageSize, pageSize,
and the BadRequest construction accordingly.
---
Nitpick comments:
In `@api/v3/api.go`:
- Around line 19-23: The struct field comment placeholders for Ocontains and Oeq
are leaking into generated API docs; update the doc comments above the fields
Ocontains *string `json:"ocontains,omitempty"` and Oeq *string
`json:"oeq,omitempty"` to meaningful descriptions (e.g., "Filter: contains
substring" and "Filter: equals") or remove the comments entirely so OpenAPI
output no longer contains the placeholder text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56a35832-1254-44ea-b443-f025e4bc7042
⛔ Files ignored due to path filters (3)
api/v3/openapi.yamlis excluded by!**/openapi.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (20)
Makefileapi/spec/packages/aip/src/llmcost/operations.tspapi/v3/api.gen.goapi/v3/api.goapi/v3/filters/filter.goapi/v3/handlers/llmcost/convert.goapi/v3/handlers/llmcost/list_overrides.goapi/v3/handlers/llmcost/list_prices.goapi/v3/oasmiddleware/decoder.goapi/v3/oasmiddleware/error.goapi/v3/oasmiddleware/hook.goapi/v3/oasmiddleware/router.goapi/v3/oasmiddleware/validator.goapi/v3/request/aip.goapi/v3/request/aip_filter.goapi/v3/request/aip_pagination.goapi/v3/request/aip_sort.goapi/v3/request/cursor.goapi/v3/server/server.goapi/v3/spec.go
💤 Files with no reviewable changes (2)
- api/v3/oasmiddleware/router.go
- api/v3/oasmiddleware/decoder.go
🚧 Files skipped from review as they are similar to previous changes (7)
- api/v3/handlers/llmcost/convert.go
- Makefile
- api/v3/spec.go
- api/v3/filters/filter.go
- api/v3/request/aip_sort.go
- api/v3/handlers/llmcost/list_overrides.go
- api/v3/request/aip.go
Comment on lines
54
to
+68
| /** Filter by provider. e.g. ?filter[provider][eq]=openai */ | ||
| provider?: FilterSingleString; | ||
| @extension("x-go-type", "FilterString") | ||
| provider?: string | FilterSingleString; | ||
|
|
||
| /** Filter by model ID. e.g. ?filter[model_id][eq]=gpt-4 */ | ||
| model_id?: FilterSingleString; | ||
| @extension("x-go-type", "FilterString") | ||
| model_id?: string | FilterSingleString; | ||
|
|
||
| /** Filter by model name. e.g. ?filter[model_name][contains]=gpt */ | ||
| model_name?: FilterSingleString; | ||
| @extension("x-go-type", "FilterString") | ||
| model_name?: string | FilterSingleString; | ||
|
|
||
| /** Filter by currency code. e.g. ?filter[currency][eq]=USD */ | ||
| currency?: FilterSingleString; | ||
| @extension("x-go-type", "FilterString") | ||
| currency?: string | FilterSingleString; |
Contributor
There was a problem hiding this comment.
The new shorthand form should be documented too.
These filters now accept string | FilterSingleString, but the examples still only show filter[field][eq]=.... Since bare filter[field]=value is supported as the eq shorthand, the spec is under-documenting the public syntax.
As per coding guidelines "The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/spec/packages/aip/src/llmcost/operations.tsp` around lines 54 - 68,
Update the docs for the filter fields (provider, model_id, model_name, currency)
to show the supported shorthand syntax in addition to the explicit operator
form: explain that these fields accept string | FilterSingleString and that a
bare query like filter[provider]=openai (or filter[model_id]=gpt-4,
filter[model_name]=gpt, filter[currency]=USD) is accepted as the eq shorthand
equivalent to filter[provider][eq]=..., so add examples and a short note about
the shorthand next to each field declaration (references: provider, model_id,
model_name, currency, FilterSingleString).
Comment on lines
+67
to
89
| // libopenapi uses: path, query, header, cookie for parameter validation | ||
| switch strings.ToLower(ve.ValidationType) { | ||
| case "path": | ||
| return apierrors.InvalidParamSourcePath | ||
| case "query": | ||
| return apierrors.InvalidParamSourceQuery | ||
| case "header": | ||
| return apierrors.InvalidParamSourceHeader | ||
| case "requestbody", "schema": | ||
| return apierrors.InvalidParamSourceBody | ||
| } | ||
| if path := schemaErr.JSONPointer(); len(path) > 0 { | ||
| ip.Field = strings.Join(path, ".") | ||
| switch strings.ToLower(ve.ValidationSubType) { | ||
| case "path": | ||
| return apierrors.InvalidParamSourcePath | ||
| case "query": | ||
| return apierrors.InvalidParamSourceQuery | ||
| case "header": | ||
| return apierrors.InvalidParamSourceHeader | ||
| case "requestbody", "schema": | ||
| return apierrors.InvalidParamSourceBody | ||
| } | ||
| return *ip | ||
| return apierrors.InvalidParamSourceBody | ||
| } |
Contributor
There was a problem hiding this comment.
cookie validation source is currently misclassified.
At Line 67, the comment lists cookie, but the switch doesn’t handle it and falls back to body (Line 88). That makes InvalidParameter source less accurate.
💡 Suggested fix
switch strings.ToLower(ve.ValidationType) {
case "path":
return apierrors.InvalidParamSourcePath
case "query":
return apierrors.InvalidParamSourceQuery
case "header":
return apierrors.InvalidParamSourceHeader
+ case "cookie":
+ return apierrors.InvalidParamSourceHeader
case "requestbody", "schema":
return apierrors.InvalidParamSourceBody
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/oasmiddleware/error.go` around lines 67 - 89, The switches on
ve.ValidationType and ve.ValidationSubType misclassify "cookie" by falling
through to body; update both switch statements to handle the "cookie" case and
return the correct enum (e.g., apierrors.InvalidParamSourceCookie) for the
"cookie" string, leaving the existing fallthrough/default to body otherwise so
other behavior is unchanged.
Comment on lines
+76
to
+102
| // filterQueryParamErrors removes validation errors for multi-level nested deepObject | ||
| // query params. libopenapi-validator has a known issue: it works for objects with | ||
| // depth of one but fails for nested objects (e.g. filter[provider][eq]=x). | ||
| // See https://github.com/pb33f/libopenapi-validator/issues/83 | ||
| func filterQueryParamErrors(errs []*validatorerrors.ValidationError) []*validatorerrors.ValidationError { | ||
| if len(errs) == 0 { | ||
| return errs | ||
| } | ||
| var filtered []*validatorerrors.ValidationError | ||
| for _, e := range errs { | ||
| if e == nil { | ||
| continue | ||
| } | ||
| // Only skip errors for object-type query params that failed schema validation | ||
| // (e.g. "The query parameter 'filter' is defined as an object, however it | ||
| // failed to pass a schema validation"). Simple query params are still validated. | ||
| msg := strings.ToLower(e.Message) | ||
| reason := strings.ToLower(e.Reason) | ||
| isQueryParam := strings.Contains(msg, "query parameter") || strings.Contains(reason, "query parameter") | ||
| isObjectSchema := strings.Contains(msg, "object") && strings.Contains(msg, "schema validation") || | ||
| strings.Contains(reason, "object") && strings.Contains(reason, "schema validation") | ||
| if isQueryParam && isObjectSchema { | ||
| continue | ||
| } | ||
| filtered = append(filtered, e) | ||
| } | ||
| return filtered |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does github.com/pb33f/libopenapi-validator expose structured fields on ValidationError that identify the offending query parameter or schema path, so callers can avoid string-matching Message/Reason text?
💡 Result:
No—errors.ValidationError itself does not expose dedicated structured fields like ParameterName, In (query/header/path), or a JSON-pointer-style InstancePath that would let you reliably identify “the offending query parameter” without looking at text.
What you do get on ValidationError is mostly high-level context plus an untyped Context pointer: Message, Reason, ValidationType, ValidationSubType, spec line/col, plus RequestPath, SpecPath, RequestMethod, and Context interface{}. Callers would need to type-assert Context (when populated) to whatever parameter/schema object was being validated, otherwise they’re left with Message/Reason strings. [1][2]
For schema failures specifically, ValidationError.SchemaValidationErrors[] contains structured location fields (Location, DeepLocation, AbsoluteLocation) that identify where in the schema/instance the JSON Schema validator reported the problem—so you can avoid string matching there. [1][2]
🏁 Script executed:
cd /tmp && curl -s "https://raw.githubusercontent.com/pb33f/libopenapi-validator/main/schema_validator.go" | head -100Repository: openmeterio/openmeter
Length of output: 79
🏁 Script executed:
rg "ValidationError" api/v3/oasmiddleware/ -A 5 -B 2Repository: openmeterio/openmeter
Length of output: 9482
Use structured ValidationError fields instead of string matching to narrow the filter scope.
The filtering is indeed too broad. Currently it drops any object-style query-param schema error matching generic message text, which can hide real validation failures. The good news: ValidationError already exposes structured fields you can leverage—ValidationType and ParameterName are directly accessible (you're already using them in error.go for error conversion).
Instead of matching on Message / Reason strings, check e.ValidationType == "query" paired with e.ParameterName == "filter" (or whatever specific parameters need the workaround). This targets the actual bug rather than suppressing a class of errors wholesale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/oasmiddleware/validator.go` around lines 76 - 102, In
filterQueryParamErrors, replace the broad string-matching logic with checks on
the structured ValidationError fields: only suppress errors where
e.ValidationType == "query" and e.ParameterName == "filter" (or other explicit
parameter names you intend to exempt) and the error indicates an object-schema
schema-validation failure; keep all other errors. Use the ValidationType and
ParameterName fields from the ValidationError struct (instead of matching
e.Message/e.Reason) and only fall back to the existing schema/object indicator
if needed to confirm it's an object schema validation error before continuing to
skip that error.
api/v3/request/aip_filter.go
Outdated
Comment on lines
+143
to
+149
| return nil, apierrors.NewBadRequestError(ctx, ErrUnallowedFilterColumn, | ||
| apierrors.InvalidParameters{ | ||
| apierrors.InvalidParameter{ | ||
| Field: filterName(o.Filter), | ||
| Reason: "unauthorized filter on column", | ||
| Source: apierrors.InvalidParamSourceQuery, | ||
| Rule: "unknown_property", |
Contributor
There was a problem hiding this comment.
These ValidationIssues are pointing at the wrong field.
A few BadRequest branches fill InvalidParameter.Field with the operator (eq) or the filter value instead of the rejected query key / column. Using qs or o.Name here would make strict-mode 400s much easier for clients to map back to the request.
Based on learnings "Follow HTTP handler patterns and ValidationIssue conventions documented in the /api skill"
Also applies to: 161-168, 216-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/request/aip_filter.go` around lines 143 - 149, The
InvalidParameter.Field in the BadRequest errors is being set to the operator or
value (via filterName(o.Filter) / o.Filter) instead of the offending query
key/column; update the error construction in the branches that return
apierrors.NewBadRequestError (the ones currently using filterName(o.Filter) or
o.Filter) to use the query string key or column identifier (qs or o.Name) as the
Field, keep Reason/Source/Rule the same, and apply this change consistently for
the other similar branches (the ones around the other BadRequest returns
currently using o.Filter or the value) so clients can map strict-mode 400s back
to the original request field.
Comment on lines
+42
to
+48
| func xorText(cipherKey, input string) string { | ||
| var output string | ||
| keyLen := len(cipherKey) | ||
| for i := range input { | ||
| output += string(input[i] ^ cipherKey[i%keyLen]) | ||
| } | ||
| return output |
Contributor
There was a problem hiding this comment.
Please fail fast on an empty cipher key.
If cipherKey is empty, i % keyLen panics here. Since the key is configurable and these helpers are public, this should return an error instead of blowing up the request path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/request/cursor.go` around lines 42 - 48, The xorText helper will panic
when cipherKey is empty due to division by zero in i%keyLen; change xorText to
validate the key up-front (if len(cipherKey) == 0) and return an error instead
of panicking, e.g. update the signature from xorText(cipherKey, input string)
string to xorText(cipherKey, input string) (string, error), perform the
empty-key check and return a descriptive error, and update all call sites to
handle the error (propagate it up the request path rather than allowing a
panic).
Comment on lines
+120
to
+129
| c := Cursor{encoded: cursor, decoded: string(v)} | ||
|
|
||
| // We're adding a single-character version ID to the cursor, in case we ever change this | ||
| // implementation. As such, we need to account for it & remove it before XOR'ing the input. | ||
| versionIdx := getVersionIdx(c.decoded[1:]) | ||
| if c.version = string(c.decoded[versionIdx]); c.version != cursorVersion { | ||
| return nil, ErrInvalidCursor | ||
| } | ||
| c.decoded = xorText(cipherKey, c.decoded[:versionIdx]+c.decoded[versionIdx+1:]) | ||
|
|
Contributor
There was a problem hiding this comment.
A version-only cursor currently decodes as a valid empty ID.
MQ== makes it through the version check and returns ID() == "", even though EncodeCursor never emits that shape. A small minimum-length / post-decode non-empty guard would keep this on the ErrInvalidCursor path.
💡 Tight guard
- c := Cursor{encoded: cursor, decoded: string(v)}
+ if len(v) < 2 {
+ return nil, ErrInvalidCursor
+ }
+ c := Cursor{encoded: cursor, decoded: string(v)}
@@
- c.decoded = xorText(cipherKey, c.decoded[:versionIdx]+c.decoded[versionIdx+1:])
+ c.decoded = xorText(cipherKey, c.decoded[:versionIdx]+c.decoded[versionIdx+1:])
+ if c.decoded == "" {
+ return nil, ErrInvalidCursor
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| c := Cursor{encoded: cursor, decoded: string(v)} | |
| // We're adding a single-character version ID to the cursor, in case we ever change this | |
| // implementation. As such, we need to account for it & remove it before XOR'ing the input. | |
| versionIdx := getVersionIdx(c.decoded[1:]) | |
| if c.version = string(c.decoded[versionIdx]); c.version != cursorVersion { | |
| return nil, ErrInvalidCursor | |
| } | |
| c.decoded = xorText(cipherKey, c.decoded[:versionIdx]+c.decoded[versionIdx+1:]) | |
| if len(v) < 2 { | |
| return nil, ErrInvalidCursor | |
| } | |
| c := Cursor{encoded: cursor, decoded: string(v)} | |
| // We're adding a single-character version ID to the cursor, in case we ever change this | |
| // implementation. As such, we need to account for it & remove it before XOR'ing the input. | |
| versionIdx := getVersionIdx(c.decoded[1:]) | |
| if c.version = string(c.decoded[versionIdx]); c.version != cursorVersion { | |
| return nil, ErrInvalidCursor | |
| } | |
| c.decoded = xorText(cipherKey, c.decoded[:versionIdx]+c.decoded[versionIdx+1:]) | |
| if c.decoded == "" { | |
| return nil, ErrInvalidCursor | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/request/cursor.go` around lines 120 - 129, The cursor decoding path
allows a version-only payload to pass and produce an empty ID; update Cursor
decoding in the block that uses getVersionIdx and xorText so it rejects
too-short or empty-ID cursors: ensure the decoded payload has the expected
minimum length before indexing (guard against decoded being shorter than 2 when
calling getVersionIdx and when reading c.decoded[versionIdx]), and after
removing the version character and running xorText verify the resulting ID
string is non-empty; if either check fails, return ErrInvalidCursor. Reference
symbols: Cursor, getVersionIdx, cursorVersion, xorText, and ErrInvalidCursor.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
api/v3/request/cursor.go (2)
121-130:⚠️ Potential issue | 🟠 MajorReject version-only cursor payloads.
A payload like
MQ==decodes to just the version byte, passes this check, and comes back withID() == "".EncodeCursornever emits that shape, so this path should reject too-short / empty-ID cursors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/cursor.go` around lines 121 - 130, The cursor decoding path in Cursor (constructed in Cursor{encoded: cursor, decoded: string(v)}) accepts payloads that contain only the version byte (e.g., MQ==) and yields an empty ID; reject those by validating the payload length and decoded ID before XOR/return. In practice, inside the block using getVersionIdx, cursorVersion and xorText, check that versionIdx > 1 and that the decoded payload (after removing the version byte) is non-empty (or that the length of c.decoded before removing version is greater than versionIdx) and return ErrInvalidCursor for too-short payloads; mirror the shape produced by EncodeCursor and ensure Cursor.ID() cannot be empty for a valid decoded cursor.
43-48:⚠️ Potential issue | 🔴 CriticalGuard
xorTextagainst an empty key.If
cipherKeyis empty,i%keyLenpanics here and takes the request path down. Please fail fast and letEncodeCursor/DecodeCursorreturn an error instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/cursor.go` around lines 43 - 48, xorText currently panics when cipherKey is empty; update it to validate the key and propagate an error instead of panicking: change xorText to return (string, error) and if cipherKey == "" return a clear error; update callers EncodeCursor and DecodeCursor to call the new xorText signature, handle the error, and return it up the call chain so encoding/decoding fails fast with a proper error rather than crashing. Ensure the error message clearly states "empty cipher key" and update any tests / usages of EncodeCursor/DecodeCursor accordingly.api/v3/oasmiddleware/validator.go (1)
89-98:⚠️ Potential issue | 🟠 MajorDon’t blanket-ignore object query validation errors.
This filter is broader than issue
#83: it drops any object-style query param schema error based on message text. That can hide legitimate bad requests too, so please scope the workaround to the affected parameter(s) and prefer structuredValidationErrormetadata overMessage/Reasonmatching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/oasmiddleware/validator.go` around lines 89 - 98, The current logic that sets isQueryParam/isObjectSchema by matching e.Message/e.Reason is too broad; instead check the structured fields on the ValidationError (e.g., use e.InstanceLocation, e.Field, e.Key or whatever concrete metadata the validator exposes) and only bypass errors for the specific parameter names affected (e.g., "filter") and for the specific validation keyword/type indicating an object schema failure; replace the text-based checks around e.Message/e.Reason and the isQueryParam/isObjectSchema guard in validator.go with a predicate that (1) verifies the error relates to a query parameter via a structured location/key field and (2) that the parameter name equals the allowed skip-list (e.g., "filter"), and (3) that the error kind/keyword indicates object schema validation failure before continuing.
🧹 Nitpick comments (2)
api/v3/request/aip_sort.go (2)
44-57: Authorization logic may silently reject dot-fields when onlyauthorizedSortsis configured.If a caller only uses
WithAuthorizedSorts(withoutWithAuthorizedDotSorts) and a user passes?sort=model.id, the flow is:
checkDotSortsis false (noauthorizedDotSortsconfigured)checkSortsis true- Field contains a dot, so case on line 50 doesn't match
- Falls to case on line 53, which checks
authorizedSortsThis works if
model.idis explicitly inauthorizedSorts(which it is inlist_prices.go), but the logic could be clearer. Consider adding a brief comment explaining that dot-fields are matched literally againstauthorizedSortswhenauthorizedDotSortsisn't configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_sort.go` around lines 44 - 57, isAuthorizedSort currently treats dot-fields differently only when authorizedDotSorts is configured; when authorizedDotSorts is empty dot-containing fields are matched literally against authorizedSorts. Update the isAuthorizedSort function to add a brief clarifying comment above the switch (referencing authorizedSorts, authorizedDotSorts, and the field/parts logic) stating that if authorizedDotSorts is not provided, dot-fields like "model.id" must be present verbatim in authorizedSorts, otherwise dot-prefix matching is used when authorizedDotSorts exists.
18-42: Unused error return value.The
extractSortfunction signature returns([]SortBy, error)but always returnsnilfor the error. If this is intentional for future extension, consider adding a comment. Otherwise, you could simplify the signature to just return[]SortBy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/request/aip_sort.go` around lines 18 - 42, The extractSort function currently returns ([]SortBy, error) but never uses the error; change its signature to return just []SortBy and remove the nil error returns: update func extractSort(qs url.Values, c *config) []SortBy, remove the error from all return sites inside the function, and update all callers to handle the new single-return value; alternatively, if you intend to keep the error for future validation, add a clarifying comment above extractSort explaining the unused error is intentional. Ensure references to SortBy, SortQuery, and config.defaultSort are preserved when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/v3/oasmiddleware/validator.go`:
- Around line 89-98: The current logic that sets isQueryParam/isObjectSchema by
matching e.Message/e.Reason is too broad; instead check the structured fields on
the ValidationError (e.g., use e.InstanceLocation, e.Field, e.Key or whatever
concrete metadata the validator exposes) and only bypass errors for the specific
parameter names affected (e.g., "filter") and for the specific validation
keyword/type indicating an object schema failure; replace the text-based checks
around e.Message/e.Reason and the isQueryParam/isObjectSchema guard in
validator.go with a predicate that (1) verifies the error relates to a query
parameter via a structured location/key field and (2) that the parameter name
equals the allowed skip-list (e.g., "filter"), and (3) that the error
kind/keyword indicates object schema validation failure before continuing.
In `@api/v3/request/cursor.go`:
- Around line 121-130: The cursor decoding path in Cursor (constructed in
Cursor{encoded: cursor, decoded: string(v)}) accepts payloads that contain only
the version byte (e.g., MQ==) and yields an empty ID; reject those by validating
the payload length and decoded ID before XOR/return. In practice, inside the
block using getVersionIdx, cursorVersion and xorText, check that versionIdx > 1
and that the decoded payload (after removing the version byte) is non-empty (or
that the length of c.decoded before removing version is greater than versionIdx)
and return ErrInvalidCursor for too-short payloads; mirror the shape produced by
EncodeCursor and ensure Cursor.ID() cannot be empty for a valid decoded cursor.
- Around line 43-48: xorText currently panics when cipherKey is empty; update it
to validate the key and propagate an error instead of panicking: change xorText
to return (string, error) and if cipherKey == "" return a clear error; update
callers EncodeCursor and DecodeCursor to call the new xorText signature, handle
the error, and return it up the call chain so encoding/decoding fails fast with
a proper error rather than crashing. Ensure the error message clearly states
"empty cipher key" and update any tests / usages of EncodeCursor/DecodeCursor
accordingly.
---
Nitpick comments:
In `@api/v3/request/aip_sort.go`:
- Around line 44-57: isAuthorizedSort currently treats dot-fields differently
only when authorizedDotSorts is configured; when authorizedDotSorts is empty
dot-containing fields are matched literally against authorizedSorts. Update the
isAuthorizedSort function to add a brief clarifying comment above the switch
(referencing authorizedSorts, authorizedDotSorts, and the field/parts logic)
stating that if authorizedDotSorts is not provided, dot-fields like "model.id"
must be present verbatim in authorizedSorts, otherwise dot-prefix matching is
used when authorizedDotSorts exists.
- Around line 18-42: The extractSort function currently returns ([]SortBy,
error) but never uses the error; change its signature to return just []SortBy
and remove the nil error returns: update func extractSort(qs url.Values, c
*config) []SortBy, remove the error from all return sites inside the function,
and update all callers to handle the new single-return value; alternatively, if
you intend to keep the error for future validation, add a clarifying comment
above extractSort explaining the unused error is intentional. Ensure references
to SortBy, SortQuery, and config.defaultSort are preserved when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ac576ef-acb1-4f93-b86e-736b6ce9b29d
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (13)
api/spec/packages/aip/src/llmcost/operations.tspapi/v3/api.gen.goapi/v3/handlers/llmcost/list_prices.goapi/v3/oasmiddleware/validator.goapi/v3/request/aip.goapi/v3/request/aip_filter.goapi/v3/request/aip_filter_test.goapi/v3/request/aip_pagination.goapi/v3/request/aip_sort.goapi/v3/request/aip_sort_test.goapi/v3/request/aip_test.goapi/v3/request/cursor.goapi/v3/request/cursor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/spec/packages/aip/src/llmcost/operations.tsp
Comment on lines
+130
to
+142
| if err == nil && pageNumber < 0 { | ||
| return 0, apierrors.NewBadRequestError(ctx, err, | ||
| apierrors.InvalidParameters{ | ||
| apierrors.InvalidParameter{ | ||
| Field: PageNumberQuery, | ||
| Reason: "unable to parse query field", | ||
| Source: apierrors.InvalidParamSourceQuery, | ||
| Rule: "page number should be a positive integer", | ||
| }, | ||
| }) | ||
| } | ||
| if err != nil || pageNumber < 1 { | ||
| return DefaultPaginationNumber, nil |
Contributor
There was a problem hiding this comment.
Report negative page numbers as validation errors, not parse failures.
This branch is handling a successfully parsed negative number, but it still reuses the "unable to parse query field" validation issue and passes a nil err into NewBadRequestError. If lenient mode is meant to fall back here, this also skips that path. Based on learnings, api/**/*.go should follow HTTP handler patterns and ValidationIssue conventions documented in the /api skill.
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
2 participants
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.
First implementation for aip filtering and apply it to llmcost prices
Summary by CodeRabbit
Release Notes
New Features
Tests