refactor: decoupling and speeding up#163
Merged
SantiagoDePolonia merged 13 commits intomainfrom Mar 22, 2026
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRefactors unknown JSON-field handling by replacing map-based storage with a compact UnknownJSONFields type, adds allocation-light scanning/merging helpers, switches several JSON-peek code paths to gjson, and implements an async bounded cache-write worker pool with shutdown coordination. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerMiddleware as Middleware
participant Enqueue as EnqueueLogic
participant WorkerPool as Workers
participant Store
Client->>Middleware: HTTP request
Middleware->>Enqueue: requestBodyForCache(req) -> (body, cacheable)
alt cacheable
Enqueue->>WorkerPool: enqueue write job
WorkerPool->>Store: Set(job)
Store-->>WorkerPool: ack
WorkerPool-->>Middleware: in-flight decremented
else not cacheable
Enqueue-->>Middleware: continue without enqueue
end
Middleware-->>Client: respond
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-03-22-chat-unknown-fields-refactor.md`:
- Line 13: Heading levels jump from H1 to H3 in this document; change the task
headings like "### Task 1: Add Raw Unknown-Field Container" (and the other task
headings at the noted spots) to H2 (use "##") or insert an intermediate H2
section above them so the structure goes H1 → H2 → H3 and satisfies markdownlint
MD001.
In `@internal/core/batch_preparation_test.go`:
- Around line 40-43: The test mutates slices returned by
cloned.Requests[0].ExtraFields.Lookup("x_item") and
cloned.ExtraFields.Lookup("x_top") without checking the returned slice is
non-nil and long enough, which can panic; before assigning to itemExtra[9] and
topExtra[9] guard the Lookup results by verifying they are not nil and have
length > 9 (or handle shorter/nil cases appropriately—e.g. skip mutation or
expand/copy into a new slice) so the test won't panic when Lookup returns nil or
a shorter byte slice.
In `@internal/core/json_fields.go`:
- Around line 44-56: The code building JSON in the loop over keys writes raw
bytes verbatim (using CloneRawJSON(fields[key])) which leaves `{"key":}` when
fields[key] is nil/empty; update the loop in the function that constructs buf
(the section iterating over keys and using fields[key] and CloneRawJSON) to
check the raw value first and, if nil or length 0, write the literal "null"
after the colon, otherwise write CloneRawJSON(fields[key]) as before so the
output is valid JSON for empty values.
- Around line 271-347: scanJSONValue, scanJSONObject, and scanJSONArray
currently accept malformed bare tokens and allow missing/Trailing commas; update
them so the scanner enforces JSON syntax: in scanJSONValue's default branch
actually parse/validate bare literals (true,false,null or a JSON number) instead
of accepting any run of non-delimiters — return an error for invalid tokens; in
scanJSONObject (function scanJSONObject) after parsing a value and
skipJSONWhitespace, require either a ',' to continue or a closing '}' (otherwise
return an error) and treat a trailing ',' followed immediately by '}' as an
error; likewise in scanJSONArray (function scanJSONArray) after each element
require either ',' or ']' (reject trailing commas) and return an error if
neither is present; ensure all branches advance the index to avoid infinite
loops and propagate errors from the literal validator back to callers.
- Around line 78-90: The Lookup implementation currently returns on the first
matching member, which loses later duplicates; instead, iterate through the
entire object, and when fieldName == key store CloneRawJSON(value) into a local
result variable (do not return immediately), continue decoding remaining members
(using readJSONObjectKey/dec.Decode as today) to preserve source order, then
after the loop return the last stored result (or nil if none found); update
references in Lookup to use a local "result" (or "found") and ensure existing
error handling paths remain unchanged.
In `@internal/responsecache/simple.go`:
- Around line 72-75: The raw error returned from
requestBodyForCache(c.Request()) must be wrapped as a core.GatewayError before
returning to enforce the repository's client-facing error contract; replace the
direct return of err with constructing and returning a core.GatewayError
(populate the OpenAI-compatible fields: type set to an appropriate client-facing
category, message set to err.Error(), param = nil, code = nil) so middleware
always emits the typed GatewayError at this boundary (locate the call to
requestBodyForCache in the same function in simple.go and wrap its error result
accordingly).
- Around line 173-178: The snapshot handling in the cache path uses
snapshot.CapturedBodyView() before checking snapshot.BodyNotCaptured, causing an
empty stored slice to be treated as an empty body; change the logic in the block
that uses core.GetRequestSnapshot(req.Context()) so it first checks
snapshot.BodyNotCaptured and returns nil,false,nil when true, and only then
calls snapshot.CapturedBodyView() to reuse bytes; reference the
GetRequestSnapshot call and RequestSnapshot.CapturedBodyView/BodyNotCaptured
fields to locate and reorder the checks.
In `@internal/server/model_validation.go`:
- Around line 149-163: The function selectorHintsFromJSONGJSON currently treats
any json.Unmarshal failure as parsed=false, causing syntactically valid bodies
with wrong selector field types to skip validation; change it so parsed reflects
successful top-level object traversal (what core.VisitTopLevelJSONObjectFields
returns) and do not short-circuit on unmarshal errors: in the
VisitTopLevelJSONObjectFields callback for "model" and "provider" attempt
json.Unmarshal into model/provider but ignore/uncheck errors (still return true
to continue), so type mismatches don't set parsed=false; also add a
middleware-level regression test that sends malformed selector fields (e.g.,
{"model":123} and {"provider":123}) and asserts the request goes through normal
invalid-request validation rather than being skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a686a97a-fcb8-49ea-aecc-c432bf889c83
📒 Files selected for processing (36)
.gitignoredocs/superpowers/plans/2026-03-22-chat-unknown-fields-refactor.mdinternal/core/batch.gointernal/core/batch_json_test.gointernal/core/batch_preparation.gointernal/core/batch_preparation_test.gointernal/core/chat_content.gointernal/core/chat_json_test.gointernal/core/embeddings_json_test.gointernal/core/json_codepaths_bench_test.gointernal/core/json_fields.gointernal/core/json_fields_bench_test.gointernal/core/json_fields_test.gointernal/core/request_snapshot.gointernal/core/request_snapshot_test.gointernal/core/responses.gointernal/core/responses_json_test.gointernal/core/semantic.gointernal/core/semantic_test.gointernal/core/types.gointernal/core/types_test.gointernal/guardrails/provider.gointernal/guardrails/provider_test.gointernal/providers/anthropic/anthropic.gointernal/providers/groq/groq_test.gointernal/providers/openai/openai_test.gointernal/providers/responses_adapter.gointernal/providers/responses_adapter_test.gointernal/responsecache/middleware_test.gointernal/responsecache/simple.gointernal/server/handlers_test.gointernal/server/model_validation.gointernal/server/model_validation_test.gointernal/server/request_snapshot.gointernal/server/semantic_requests_test.gotests/perf/hotpath_test.go
docs/superpowers/plans/2026-03-22-chat-unknown-fields-refactor.md
Outdated
Show resolved
Hide resolved
Comment on lines
+78
to
+90
| for dec.More() { | ||
| fieldName, ok := readJSONObjectKey(dec) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| var value json.RawMessage | ||
| if err := dec.Decode(&value); err != nil { | ||
| return nil | ||
| } | ||
| if fieldName == key { | ||
| return CloneRawJSON(value) | ||
| } |
Contributor
There was a problem hiding this comment.
Lookup should not stop at the first duplicate member.
Line 88 returns immediately on the first match, but Lines 248-255 preserve duplicate unknown members in source order. For a payload like {"x":1,"x":2}, Lookup("x") can never see the later value.
💡 Minimal fix
func (fields UnknownJSONFields) Lookup(key string) json.RawMessage {
if len(fields.raw) == 0 {
return nil
}
@@
+ var match json.RawMessage
for dec.More() {
fieldName, ok := readJSONObjectKey(dec)
if !ok {
return nil
}
@@
if err := dec.Decode(&value); err != nil {
return nil
}
if fieldName == key {
- return CloneRawJSON(value)
+ match = CloneRawJSON(value)
}
}
- return nil
+ return match
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/json_fields.go` around lines 78 - 90, The Lookup implementation
currently returns on the first matching member, which loses later duplicates;
instead, iterate through the entire object, and when fieldName == key store
CloneRawJSON(value) into a local result variable (do not return immediately),
continue decoding remaining members (using readJSONObjectKey/dec.Decode as
today) to preserve source order, then after the loop return the last stored
result (or nil if none found); update references in Lookup to use a local
"result" (or "found") and ensure existing error handling paths remain unchanged.
Comment on lines
+149
to
+163
| func selectorHintsFromJSONGJSON(body []byte) (model, provider string, parsed bool) { | ||
| parsed = core.VisitTopLevelJSONObjectFields(body, func(key string, raw []byte) bool { | ||
| switch key { | ||
| case "model": | ||
| return json.Unmarshal(raw, &model) == nil | ||
| case "provider": | ||
| return json.Unmarshal(raw, &provider) == nil | ||
| default: | ||
| return true | ||
| } | ||
| }) | ||
| if !parsed { | ||
| return "", "", false | ||
| } | ||
| return model, provider, true |
Contributor
There was a problem hiding this comment.
Do not treat selector type mismatches as “unparsed.”
Returning parsed=false here short-circuits ensureRequestModelResolution; internal/server/request_model_resolution.go Lines 93-94 bail out on !parsed. That means syntactically valid bodies like {"model":123} or {"model":"gpt-4o-mini","provider":123} now skip model validation entirely instead of entering the normal invalid-request path.
🛠️ Suggested fix
func selectorHintsFromJSONGJSON(body []byte) (model, provider string, parsed bool) {
parsed = core.VisitTopLevelJSONObjectFields(body, func(key string, raw []byte) bool {
switch key {
case "model":
- return json.Unmarshal(raw, &model) == nil
+ _ = json.Unmarshal(raw, &model)
+ return true
case "provider":
- return json.Unmarshal(raw, &provider) == nil
+ _ = json.Unmarshal(raw, &provider)
+ return true
default:
return true
}
})
if !parsed {
return "", "", false
}
return model, provider, true
}Please add a middleware-level regression test for malformed selector field types as well; the new stdlib-parity helper does not cover this control-flow contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/model_validation.go` around lines 149 - 163, The function
selectorHintsFromJSONGJSON currently treats any json.Unmarshal failure as
parsed=false, causing syntactically valid bodies with wrong selector field types
to skip validation; change it so parsed reflects successful top-level object
traversal (what core.VisitTopLevelJSONObjectFields returns) and do not
short-circuit on unmarshal errors: in the VisitTopLevelJSONObjectFields callback
for "model" and "provider" attempt json.Unmarshal into model/provider but
ignore/uncheck errors (still return true to continue), so type mismatches don't
set parsed=false; also add a middleware-level regression test that sends
malformed selector fields (e.g., {"model":123} and {"provider":123}) and asserts
the request goes through normal invalid-request validation rather than being
skipped.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/server/model_validation.go (1)
149-179: 🧹 Nitpick | 🔵 TrivialType mismatches return
parsed=false, skipping early model validation.As noted in a previous review, bodies like
{"model":123}returnparsed=false, which causesderiveExecutionPlan(line 99) to skip model resolution and return the plan without validation. This means type-invalid selectors proceed to downstream handlers instead of getting an early 400 response.This appears to be an intentional tradeoff for the gjson performance optimization. The handler will still reject the request when it attempts to use the invalid model. Consider adding a brief comment documenting this behavior choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/model_validation.go` around lines 149 - 179, Update selectorHintsFromJSONGJSON to include a brief comment explaining that non-string or type-mismatched selector fields (e.g., "model": 123) cause the function to return parsed=false and therefore bypass early model validation in deriveExecutionPlan; state this is an intentional tradeoff for gjson performance (first-match behavior and fast path) and note that downstream handlers will still reject invalid selectors when they are used. Reference the selectorHintsFromJSONGJSON function and the interaction with deriveExecutionPlan in the comment so future readers understand why parsed can be false for type-mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/json_fields.go`:
- Around line 27-31: Update the stale doc comment to match the renamed function:
replace the incorrect `CloneRawJSONMap` description with a concise comment for
`CloneUnknownJSONFields` that explains it returns a detached copy of an
UnknownJSONFields value (and mention it uses `CloneRawJSON` of the underlying
`raw` field); ensure the comment sits immediately above the
`CloneUnknownJSONFields` function and references `UnknownJSONFields` and
`CloneRawJSON` to make intent clear.
---
Duplicate comments:
In `@internal/server/model_validation.go`:
- Around line 149-179: Update selectorHintsFromJSONGJSON to include a brief
comment explaining that non-string or type-mismatched selector fields (e.g.,
"model": 123) cause the function to return parsed=false and therefore bypass
early model validation in deriveExecutionPlan; state this is an intentional
tradeoff for gjson performance (first-match behavior and fast path) and note
that downstream handlers will still reject invalid selectors when they are used.
Reference the selectorHintsFromJSONGJSON function and the interaction with
deriveExecutionPlan in the comment so future readers understand why parsed can
be false for type-mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aee2c738-1c42-4fc3-8bac-251584582ea4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modinternal/core/batch_preparation_test.gointernal/core/json_fields.gointernal/core/json_fields_test.gointernal/core/semantic.gointernal/core/semantic_test.gointernal/responsecache/middleware_test.gointernal/responsecache/simple.gointernal/server/model_validation.gointernal/server/model_validation_test.go
This was referenced Mar 26, 2026
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.
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Tests