Conversation
added 30 commits
April 1, 2026 06:41
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Adds a token-bucket WithRateLimit option that rejects excess requests with 429 Too Many Requests and a standard error envelope. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Add a bearerAuth security scheme to the generated OpenAPI document and mark non-public operations as secured, while keeping /health explicitly public. Co-Authored-By: Virgil <virgil@lethean.io>
Add an optional maxEntries cap to WithCache so the in-memory cache can evict old entries instead of growing without bound.\n\nCo-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Keep OpenAPI requestBody generation aligned with the RouteDescription contract by allowing non-GET operations, including DELETE, to declare JSON bodies. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Add stable registry helpers for enumerating provider OpenAPI spec files, plus iterator coverage. This gives discovery consumers a direct way to aggregate provider docs without changing routing behaviour. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds bounded, LRU response caching, transport path configurability (WS/SSE/GraphQL), enriched OpenAPI/Swagger spec builder and runtime client, authentik and i18n snapshotting, response-meta injection middleware, token-bucket rate limiting, tool-bridge JSON Schema validation, iterator/snapshot protections, CLI/spec/sdk improvements, provider/proxy hardening, many tests, and numerous PHP API features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenAPIClient
participant SpecLoader
participant HTTPClient
participant SchemaValidator
Client->>OpenAPIClient: Call(operationID, params)
OpenAPIClient->>SpecLoader: Load spec (lazy/memoised)
SpecLoader-->>OpenAPIClient: Operations metadata
OpenAPIClient->>SchemaValidator: Validate params (if schema present)
alt Params invalid
SchemaValidator-->>OpenAPIClient: Error
OpenAPIClient-->>Client: Return validation error
else Params valid
OpenAPIClient->>HTTPClient: Build request (path/query/body/headers)
HTTPClient-->>OpenAPIClient: Response + body
OpenAPIClient->>SchemaValidator: Validate response (if schema present)
alt Response invalid
SchemaValidator-->>OpenAPIClient: Error
OpenAPIClient-->>Client: Return response error
else Response valid
OpenAPIClient-->>Client: Return decoded result
end
end
sequenceDiagram
participant Request as Client Request
participant Middleware as ResponseMetaMiddleware
participant Recorder as ResponseMetaRecorder
participant Handler as App Handler
participant JSONProc as JSON injector
Request->>Middleware: Incoming request
Middleware->>Recorder: Wrap ResponseWriter
Recorder->>Handler: Call next handler
Handler->>Recorder: Write status, headers, body (buffered)
Handler-->>Middleware: Handler done
Middleware->>Recorder: Inspect buffered body
alt Body is JSON envelope and meta available
Middleware->>JSONProc: Decode and inject request_id/duration
JSONProc-->>Middleware: Re-encoded body
Middleware->>Recorder: Replace body, update Content-Length
end
Recorder->>Client: Commit headers, status, body
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
codegen.go (1)
71-96:⚠️ Potential issue | 🟠 MajorPass the trimmed spec path into
buildArgs.Lines 71-79 normalise
g.SpecPath, but Line 112 still passes the raw field toopenapi-generator-cli. A value like" ./openapi.yaml "now passes validation and then fails at execution time.Suggested fix
- args := g.buildArgs(generator, outputDir) + args := g.buildArgs(specPath, generator, outputDir)-func (g *SDKGenerator) buildArgs(generator, outputDir string) []string { +func (g *SDKGenerator) buildArgs(specPath, generator, outputDir string) []string { args := []string{ "generate", - "-i", g.SpecPath, + "-i", specPath, "-g", generator, "-o", outputDir, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codegen.go` around lines 71 - 96, The trimmed specPath variable (from strings.TrimSpace(g.SpecPath)) must be used when building CLI args: change the call sites that invoke g.buildArgs(generator, outputDir) to pass specPath instead of relying on g.SpecPath, and update the buildArgs function signature/implementation to accept and use this specPath parameter (replace any uses of g.SpecPath inside buildArgs) so the normalized path is what's passed to openapi-generator-cli.cache.go (1)
50-77:⚠️ Potential issue | 🔴 CriticalKeep expiry removal in one critical section.
getreadsentry, unlocks, and later deletes by key. If another goroutine refreshes the same key in between, this path can delete the fresh entry and subtract the stale size fromcurrentBytes. Recheck thats.entries[key] == entryunder the second lock, or perform the expiry check and removal while still holding the mutex.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache.go` around lines 50 - 77, The get method on cacheStore currently unlocks before checking and removing expired entries which can race with concurrent updates; fix by performing the expiry check and any removal while still holding s.mu (or, if you prefer minimal change, re-acquire s.mu and verify that s.entries[key] == entry before removing and adjusting s.currentBytes). Specifically, update the get function to keep the mutex locked across the time.Now().After(entry.expires) check and any calls that modify s.index, s.order, s.currentBytes, and s.entries (or, if re-locking, compare the stored pointer to the original entry to avoid deleting a refreshed entry).pkg/provider/proxy.go (1)
61-72:⚠️ Potential issue | 🟠 MajorStrip
basePathbefore running the default director.
httputil.NewSingleHostReverseProxyjoinstarget.Pathwith the incoming request path. Ifcfg.Upstreamcontains its own path prefix, stripping afterdefaultDirector(req)no longer matchesbasePath, and the upstream receives the provider prefix as well. Rewritereq.URL.Pathandreq.URL.RawPathfirst, then let the default director join against the target. A regression test with an upstream path prefix would catch this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provider/proxy.go` around lines 61 - 72, The current proxy.Director strips cfg.BasePath after calling defaultDirector(req), which is wrong because httputil.NewSingleHostReverseProxy's defaultDirector may join the target.Path with the incoming path; instead, rewrite req.URL.Path and req.URL.RawPath by calling stripBasePath(req.URL.Path, basePath) and stripBasePath(req.URL.RawPath, basePath) first, then call defaultDirector(req) so the director joins against the stripped path; update the proxy.Director closure accordingly (references: proxy.Director, defaultDirector, stripBasePath, cfg.BasePath, req.URL.Path, req.URL.RawPath).sse.go (1)
130-150:⚠️ Potential issue | 🟠 MajorDrain can still admit late clients and hang shutdown.
Drain()only signals the clients present under the lock. AHandler()racing just after Line 217 can stillwg.Add(1)and stay connected indefinitely, sowg.Wait()may never return. Set a broker-level draining flag before iterating and reject or immediately close late arrivals.Suggested shape of the fix
type SSEBroker struct { mu sync.RWMutex wg sync.WaitGroup clients map[*sseClient]struct{} + draining bool } @@ func (b *SSEBroker) Handler() gin.HandlerFunc { return func(c *gin.Context) { channel := c.Query("channel") client := &sseClient{ channel: channel, events: make(chan sseEvent, 64), done: make(chan struct{}), } b.mu.Lock() + if b.draining { + b.mu.Unlock() + c.Status(http.StatusServiceUnavailable) + return + } b.clients[client] = struct{}{} b.wg.Add(1) b.mu.Unlock() @@ func (b *SSEBroker) Drain() { b.mu.Lock() + b.draining = true for client := range b.clients { client.signalDone() client.closeEvents() } b.mu.Unlock() b.wg.Wait() }Also applies to: 211-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sse.go` around lines 130 - 150, The Drain logic in SSEBroker can miss races where Handler adds a client after Drain starts, causing wg.Wait() to hang; add a broker-level draining flag (e.g., b.draining bool or an atomic) set by Drain under b.mu before iterating clients, and have Handler check that flag (under b.mu or atomically) and refuse/close late arrivals instead of doing b.wg.Add(1) and registering them; ensure Handler returns a closed response (or 503) and does not add the client when b.draining is true, and keep existing cleanup (client.signalDone/delete) for normal flows so wg counts remain consistent.pkg/provider/registry_test.go (1)
1-1:⚠️ Potential issue | 🟡 MinorFix the SPDX header token typo.
The file header uses
SPDX-Licence-Identifier; the required token isSPDX-License-Identifier.Suggested patch
-// SPDX-Licence-Identifier: EUPL-1.2 +// SPDX-License-Identifier: EUPL-1.2As per coding guidelines,
**/*.go: Include SPDX license header// SPDX-License-Identifier: EUPL-1.2at the top of Go files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provider/registry_test.go` at line 1, The SPDX header token is misspelled as "SPDX-Licence-Identifier"; update the top-of-file header to use the correct token "SPDX-License-Identifier" (i.e., replace "SPDX-Licence-Identifier: EUPL-1.2" with "SPDX-License-Identifier: EUPL-1.2") so the file header conforms to the required SPDX header format for Go files.
🟡 Minor comments (12)
pkg/provider/proxy_internal_test.go-7-25 (1)
7-25:⚠️ Potential issue | 🟡 MinorMove the
_Goodsuffix to the end of each test name.These names end with the scenario label instead of
_Good, so they miss the repository convention.TestStripBasePath_ExactBoundary_Good,TestStripBasePath_RootPath_Good, andTestStripBasePath_DoesNotTrimPartialPrefix_Goodwould fit it. As per coding guidelines,**/*_test.go: Use_Good,_Bad, and_Uglysuffixes for Go test function names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provider/proxy_internal_test.go` around lines 7 - 25, Rename the three test functions so the scenario label comes before the _Good suffix: change TestStripBasePath_Good_ExactBoundary to TestStripBasePath_ExactBoundary_Good, TestStripBasePath_Good_RootPath to TestStripBasePath_RootPath_Good, and TestStripBasePath_Good_DoesNotTrimPartialPrefix to TestStripBasePath_DoesNotTrimPartialPrefix_Good; update any references or test invocations accordingly so the function names (TestStripBasePath_Good_ExactBoundary, TestStripBasePath_Good_RootPath, TestStripBasePath_Good_DoesNotTrimPartialPrefix) are replaced with the new names.src/php/src/Api/Controllers/Api/UnifiedPixelController.php-46-63 (1)
46-63:⚠️ Potential issue | 🟡 MinorRemove unused
$pixelKeyparameter or implement tracking logic.The
$pixelKeyparameter is captured from the route but never used in the method body. The controller currently only returns HTTP responses without recording or processing the pixel key—no event dispatch, no logging, no tracking state.Either this parameter should be removed if tracking is deferred to other infrastructure, or the implementation should use it to record the tracking event.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Controllers/Api/UnifiedPixelController.php` around lines 46 - 63, The track(Request $request, string $pixelKey) method currently ignores the route parameter $pixelKey; either remove the unused parameter from the method signature and corresponding route, or implement tracking: inside track() (using $pixelKey and $request) validate the key, build a tracking payload (IP, user-agent, query params, timestamp), persist or dispatch it (e.g., via an event, repository call, or logger) before returning the same GIF/no-content responses; update any tests or route definitions accordingly so $pixelKey is no longer unused.swagger_internal_test.go-25-25 (1)
25-25:⚠️ Potential issue | 🟡 MinorAdjust the test name to end with
_Good,_Bad, or_Ugly.
TestSwaggerSpec_ReadDoc_Good_SnapshotsGroupsdoes not end with the required suffix format.As per coding guidelines,
**/*_test.go: Use_Good,_Bad, and_Uglysuffixes for Go test function names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swagger_internal_test.go` at line 25, Rename the test function TestSwaggerSpec_ReadDoc_Good_SnapshotsGroups so its name ends with one of the approved suffixes; e.g., change the identifier to TestSwaggerSpec_ReadDoc_SnapshotsGroups_Good (or any variant that places _Good at the end) and update any references/calls accordingly; ensure the new function name is used in swagger_internal_test.go and in test runners/imports so the test still compiles and runs.pkg/provider/registry_test.go-136-136 (1)
136-136:⚠️ Potential issue | 🟡 MinorRename these tests to end with
_Good,_Bad, or_Uglyexactly.These function names append extra suffixes after
_Good, which does not match the required test naming convention for Go test files.As per coding guidelines,
**/*_test.go: Use_Good,_Bad, and_Uglysuffixes for Go test function names.Also applies to: 176-176, 216-216, 247-247, 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provider/registry_test.go` at line 136, The test function names must end exactly with _Good, _Bad, or _Ugly; rename TestRegistry_StreamableIter_Good_SnapshotCurrentProviders to remove the extra trailing text so it ends with _Good (e.g., TestRegistry_StreamableIter_Good) and similarly rename the other test functions in the same file that currently append extra suffixes so each ends exactly with one of the permitted suffixes (_Good/_Bad/_Ugly); update any references (calls or t.Run names) to those functions accordingly to keep tests runnable.graphql_config_test.go-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorRename test functions to match the required suffix convention.
Both test names include trailing text after
_Good; they should end with_Good,_Bad, or_Ugly.As per coding guidelines,
**/*_test.go: Use_Good,_Bad, and_Uglysuffixes for Go test function names.Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graphql_config_test.go` at line 13, Rename test functions so they end exactly with the required suffixes (_Good, _Bad, or _Ugly): change TestEngine_GraphQLConfig_Good_SnapshotsCurrentSettings to TestEngine_GraphQLConfig_Good and similarly trim any other test names noted (the function referenced at the other location) so they end with only the suffix; update any references/imports/benchmarks or t.Run strings that refer to the old names to the new names to keep builds and test runs passing.cache_test.go-50-57 (1)
50-57:⚠️ Potential issue | 🟡 MinorMake the “large” payload materially larger than the “small” one.
Right now both routes serialise the same payload size (
strings.Repeat(..., 96)), so the size-eviction test depends on the current response envelope staying above the 250-byte threshold rather than on the intended large-entry boundary.Proposed fix
rg.GET("/small", func(c *gin.Context) { n := g.counter.Add(1) - c.JSON(http.StatusOK, api.OK(fmt.Sprintf("small-%d-%s", n, strings.Repeat("a", 96)))) + c.JSON(http.StatusOK, api.OK(fmt.Sprintf("small-%d-%s", n, strings.Repeat("a", 32)))) }) rg.GET("/large", func(c *gin.Context) { n := g.counter.Add(1) - c.JSON(http.StatusOK, api.OK(fmt.Sprintf("large-%d-%s", n, strings.Repeat("b", 96)))) + c.JSON(http.StatusOK, api.OK(fmt.Sprintf("large-%d-%s", n, strings.Repeat("b", 256)))) })Also applies to: 518-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache_test.go` around lines 50 - 57, The test routes for "/small" and "/large" both serialize payloads with strings.Repeat(..., 96), so make the "/large" response materially bigger by increasing its repeat count (e.g., use a much larger repeat value than 96) in the rg.GET handler for "/large" (the handler that calls g.counter.Add and c.JSON with api.OK(fmt.Sprintf(..., strings.Repeat(...)))) so the cache size-eviction test exercises large-entry behavior; apply the same change to the other duplicate test block that defines the same "/small" and "/large" handlers.src/php/src/Front/Api/Middleware/ApiSunset.php-36-60 (1)
36-60:⚠️ Potential issue | 🟡 MinorTrim
$sunsetDatebefore branching on it.A whitespace-only middleware argument still satisfies
!== '', so this path emits an emptySunsetheader and anX-API-Warnmessage with a dangling date. Normalise once inhandle()and reuse the trimmed value for the header and warning decisions.Proposed fix
public function handle(Request $request, Closure $next, string $sunsetDate = '', ?string $replacement = null): Response { + $sunsetDate = trim($sunsetDate); + /** `@var` Response $response */ $response = $next($request);Also applies to: 68-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Front/Api/Middleware/ApiSunset.php` around lines 36 - 60, In handle(), normalize the middleware args by trimming $sunsetDate (and $replacement) at the start so whitespace-only values don't pass the !== '' checks; use the trimmed $sunsetDate when calling formatSunsetDate(), when setting the 'Sunset' header, and when building the $warning string, and use the trimmed $replacement when setting the 'Link' header — update references to $sunsetDate/$replacement throughout handle() (including the later branch that sets the warning/header) to use the trimmed variables.cache_test.go-389-420 (1)
389-420:⚠️ Potential issue | 🟡 MinorProve the second request is a cache HIT.
This test only checks that the same
Linkvalues come back on the second request, butcacheHeaderGroupemits those headers on every execution. As written, it can still pass if caching regresses completely.Proposed fix
w2 := httptest.NewRecorder() req2, _ := http.NewRequest(http.MethodGet, "/cache/multi", nil) h.ServeHTTP(w2, req2) if w2.Code != http.StatusOK { t.Fatalf("expected 200 on cache hit, got %d", w2.Code) } + if got := w2.Header().Get("X-Cache"); got != "HIT" { + t.Fatalf("expected X-Cache=HIT, got %q", got) + } linkHeaders := w2.Header().Values("Link")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache_test.go` around lines 389 - 420, The test doesn't prove the second request was a cache hit because cacheHeaderGroup always emits the Link headers; update the test to assert the handler was executed only once by adding a side-effect counter or marker and checking it after the two requests: e.g., add a package-level counter increment in cacheHeaderGroup's handler (or wrap e.Handler() with a middleware that increments a counter on actual handler execution), call the endpoint twice in TestWithCache_Good_PreservesMultiValueHeadersOnHit, then assert the counter equals 1 (and keep the existing assertions about the Link header values) to prove the second request came from the cache.i18n.go-199-219 (1)
199-219:⚠️ Potential issue | 🟡 MinorUse
golang.org/x/text/languageto handle BCP 47 fallback chain semantics correctly.The string-based approach splits on
-and rejoin, which produces invalid intermediate tags when the locale includes BCP 47 extensions or private-use subtags. For example,de-DE-u-cowould generate intermediates likede-DE-uandde-u, which are malformed. Thelanguage.Parse()andtag.Parent()methods handle BCP 47 semantics properly and are already available via the imported package.Proposed fix
func localeFallbacks(locale string) []string { locale = strings.TrimSpace(strings.ReplaceAll(locale, "_", "-")) if locale == "" { return nil } - parts := strings.Split(locale, "-") - if len(parts) == 0 { + tag, err := language.Parse(locale) + if err != nil { return []string{locale} } - fallbacks := make([]string, 0, len(parts)) - for i := len(parts); i >= 1; i-- { - fallbacks = append(fallbacks, strings.Join(parts[:i], "-")) + var fallbacks []string + for tag != language.Und { + fallbacks = append(fallbacks, tag.String()) + next := tag.Parent() + if next == language.Und { + break + } + tag = next } return fallbacks }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@i18n.go` around lines 199 - 219, The current localeFallbacks function builds fallbacks by naive string splitting which produces invalid BCP47 intermediates (e.g., "de-DE-u-co" -> "de-DE-u"); replace the body of localeFallbacks to parse the input with language.Parse, then iterate using tag.Parent() to collect each parent tag's String() (in most-specific to least-specific order) until Parent returns language.Und or repeats; if language.Parse returns an error, fall back to returning the trimmed input as a single-element slice to preserve behavior. Use golang.org/x/text/language's Parse and Parent methods to ensure correct BCP47 semantics.cmd/api/spec_builder.go-92-95 (1)
92-95:⚠️ Potential issue | 🟡 MinorKeep the default locale inside the supported set.
If
i18nSupportedLocalesis something likefr,deand no default is supplied, this emitsI18nDefaultLocale = "en", which makes the generated spec internally inconsistent. Default to the first supported locale, or only choose"en"when it is actually present.Suggested fix
builder.I18nSupportedLocales = parseLocales(cfg.i18nSupportedLocales) if builder.I18nDefaultLocale == "" && len(builder.I18nSupportedLocales) > 0 { - builder.I18nDefaultLocale = "en" + builder.I18nDefaultLocale = builder.I18nSupportedLocales[0] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/spec_builder.go` around lines 92 - 95, The current logic sets builder.I18nDefaultLocale to "en" even when "en" isn't in builder.I18nSupportedLocales, causing inconsistency; update the block that assigns builder.I18nDefaultLocale (after builder.I18nSupportedLocales = parseLocales(cfg.i18nSupportedLocales)) so that if I18nDefaultLocale is empty and I18nSupportedLocales has entries you set the default to the first element of builder.I18nSupportedLocales, and only fall back to "en" if I18nSupportedLocales is empty or explicitly contains "en"; reference builder.I18nSupportedLocales, builder.I18nDefaultLocale, parseLocales and cfg.i18nSupportedLocales when making the change.src/php/src/Api/Services/SeoReportService.php-195-211 (1)
195-211:⚠️ Potential issue | 🟡 MinorExtract the charset token from legacy
http-equivmeta tags.For
<meta http-equiv="content-type" content="text/html; charset=UTF-8">, the fallback currently returns the whole content type string incharset. Parse out thecharset=value instead, otherwise this field is wrong on older pages.💡 Proposed fix
- return $this->extractMetaContent($xpath, 'content-type', 'http-equiv'); + $contentType = $this->extractMetaContent($xpath, 'content-type', 'http-equiv'); + if ($contentType !== null && preg_match('/charset\s*=\s*["\']?([^;"\']+)/i', $contentType, $matches) === 1) { + return $matches[1]; + } + + return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Services/SeoReportService.php` around lines 195 - 211, The extractCharset method currently falls back to returning the entire content-type string from extractMetaContent; update extractCharset to parse that returned string for a charset token (case-insensitive search for "charset=") and return only the charset value (trimmed, stripped of quotes) or null if none found. Locate extractCharset and after calling extractMetaContent('content-type','http-equiv') inspect the result, split on ';' or search for the "charset=" segment, extract and normalize the token, and return it instead of the full content-type string.src/php/src/Api/Documentation/Extensions/VersionExtension.php-60-63 (1)
60-63:⚠️ Potential issue | 🟡 MinorAdd handling for
defaultand HTTP status range keys.OpenAPI operations can declare responses as
defaultand1XX–5XXranges. The current filter only processes numeric status codes200–599, so version and deprecation headers are silently omitted from valid response definitions that use those forms.💡 Proposed fix
foreach ($operation['responses'] as $status => &$response) { - if (! is_numeric($status) || (int) $status < 200 || (int) $status >= 600) { + $status = (string) $status; + $isExact = is_numeric($status) && (int) $status >= 200 && (int) $status < 600; + $isRange = preg_match('/^[1-5]XX$/', $status) === 1; + if (! $isExact && $status !== 'default' && ! $isRange) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Documentation/Extensions/VersionExtension.php` around lines 60 - 63, The loop in VersionExtension:: (iterating $operation['responses']) currently skips non-numeric keys and thus ignores 'default' and range keys like '2XX'–'5XX'; change the filter so it treats numeric status codes 200–599, the literal 'default' key, and range keys matching /^[1-5]XX$/i as valid response entries and applies the version/deprecation header logic to them (i.e., replace the is_numeric check with a condition that allows 'default' and X'XX' ranges and still excludes invalid keys, then operate on the &$response as before).
🧹 Nitpick comments (23)
src/php/src/Api/Documentation/config.php (1)
271-277: Configuration not utilised in the Website Stoplight view.These configuration options are defined but the Website view at
src/php/src/Website/Api/View/Blade/stoplight.blade.phphardcodestheme="dark"andlayout="sidebar"instead of reading from this config. Consider whether the Website view should also use these configurable values for consistency with the API Documentation view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Documentation/config.php` around lines 271 - 277, The Stoplight configuration array ('stoplight' => ['theme','layout','hide_try_it']) is defined but the Blade view stoplight.blade.php currently hardcodes theme="dark" and layout="sidebar"; update the view to read from the config (e.g., config('Api.Documentation.stoplight.theme', 'dark'), config('Api.Documentation.stoplight.layout', 'sidebar'), and config('Api.Documentation.stoplight.hide_try_it', false)) and use those values for the Stoplight component attributes so the Website view honors the same configurable options as the API Documentation configuration.src/php/src/Website/Api/View/Blade/stoplight.blade.php (2)
8-13: Configuration values are not applied to the component.The
themeandlayoutattributes are hardcoded, andhideTryItis not included despite being configured inapi-docs.ui.stoplight. Consider using the config values for consistency with the API Documentation controller's Stoplight view.♻️ Proposed fix to use config values
<div class="min-h-[calc(100vh-4rem)]"> <elements-api apiDescriptionUrl="{{ route('api.openapi.json') }}" router="hash" - layout="sidebar" - theme="dark" + layout="{{ config('api-docs.ui.stoplight.layout', 'sidebar') }}" + `@if`(config('api-docs.ui.stoplight.hide_try_it', false)) hideTryIt="true" `@endif` ></elements-api> </div>Note: Stoplight Elements doesn't support a
themeattribute on the web component; theming is typically done via CSS variables or by choosing the appropriate stylesheet variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Website/Api/View/Blade/stoplight.blade.php` around lines 8 - 13, The elements-api component in stoplight.blade.php uses hardcoded attributes (layout="sidebar", theme="dark") and omits the configured hideTryIt value; update the component to read the UI config (api-docs.ui.stoplight) instead of hardcoding: replace the fixed layout and router/theme values with the corresponding config entries from api-docs.ui.stoplight, remove the unsupported theme attribute (Stoplight theming should be handled via CSS), and add the hideTryIt (or hide-try-it) attribute bound to the configured boolean so the component honors the controller's Stoplight view settings.
17-22: Pin CDN dependency versions to prevent unexpected breaking changes.Loading Stoplight Elements from unpkg without version pinning means the library could update at any time, potentially causing breaking changes or introducing vulnerabilities.
📦 Proposed fix to pin versions
`@push`('head') - <link rel="stylesheet" href="https://unpkg.com/@stoplight/elements/styles.min.css"> + <link rel="stylesheet" href="https://unpkg.com/@stoplight/elements@9.0.6/styles.min.css"> `@endpush` `@push`('scripts') - <script src="https://unpkg.com/@stoplight/elements/web-components.min.js"></script> + <script src="https://unpkg.com/@stoplight/elements@9.0.6/web-components.min.js"></script> `@endpush`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Website/Api/View/Blade/stoplight.blade.php` around lines 17 - 22, The CDN includes in the Blade template (`@push`('head') link rel="stylesheet" and `@push`('scripts') script src for `@stoplight/elements`) are unpinned and should be fixed by referencing a specific released package version (e.g. `@stoplight/elements`@<version>) instead of the floating unpkg URL; update both the stylesheet and web-components script URLs to include the exact version, and optionally add SRI (integrity) and crossorigin attributes for additional security and immutability.bridge_test.go (1)
309-344: Assert that invalid requests never reach the tool handler.This still passes if the bridge calls the handler and then overwrites the response with
400. A small flag here makes the short-circuit guarantee explicit.Suggested test tightening
bridge := api.NewToolBridge("/tools") + handlerCalled := false bridge.Add(api.ToolDescriptor{ Name: "file_read", Description: "Read a file from disk", Group: "files", @@ - }, func(c *gin.Context) { + }, func(c *gin.Context) { + handlerCalled = true c.JSON(http.StatusOK, api.OK("should not run")) }) @@ if resp.Error == nil || resp.Error.Code != "invalid_request_body" { t.Fatalf("expected invalid_request_body error, got %#v", resp.Error) } + if handlerCalled { + t.Fatal("handler should not be called for an invalid request body") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridge_test.go` around lines 309 - 344, The test currently only checks the final 400 response but not whether the tool handler was invoked; add an explicit short-circuit assertion by introducing a boolean flag (e.g., invoked := false) in the test and have the bridge.Add handler set invoked = true (or write a distinct header like "X-Tool-Invoked") if it runs; after ServeHTTP assert that invoked is false (and/or the header is absent) in addition to the 400 and invalid_request_body checks to guarantee the bridge validation short-circuits and never calls the handler.src/php/src/Api/Tests/Feature/OpenApiDocumentationTest.php (1)
22-23: Consider migrating this test file to Pest syntax.As per coding guidelines, files matching
src/php/src/Api/Tests/Feature/**/*.phpshould use Pest syntax rather than PHPUnit. This is a pre-existing pattern in the file, so it could be addressed as a separate refactoring task.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Tests/Feature/OpenApiDocumentationTest.php` around lines 22 - 23, The file contains a PHPUnit-style test class OpenApiDocumentationTest extending TestCase; migrate it to Pest by replacing the class and method-based tests with Pest test() functions (or it() as preferred), moving any setUp logic into beforeEach/afterEach closures, converting assertions to Pest's expectation helpers where appropriate, and removing the TestCase subclass import; locate symbols OpenApiDocumentationTest and any setUp/tearDown or test* methods and transform them into top-level Pest tests and lifecycle hooks.client.go (1)
166-177: Redundant nil check forhttpClient.The
httpClientis set tohttp.DefaultClientat line 168, so the nil check at lines 173–175 can never trigger. Similarly, the check at lines 287–289 inCallis also redundant since construction guarantees a non-nil client.🧹 Proposed simplification
func NewOpenAPIClient(opts ...OpenAPIClientOption) *OpenAPIClient { c := &OpenAPIClient{ httpClient: http.DefaultClient, } for _, opt := range opts { opt(c) } - if c.httpClient == nil { - c.httpClient = http.DefaultClient - } return c }And in
Call:func (c *OpenAPIClient) Call(operationID string, params any) (any, error) { if err := c.load(); err != nil { return nil, err } - if c.httpClient == nil { - c.httpClient = http.DefaultClient - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 166 - 177, The nil-checks for httpClient are redundant because NewOpenAPIClient already initializes httpClient to http.DefaultClient; remove the unreachable guard in NewOpenAPIClient (the if c.httpClient == nil block) and the similar nil-check in Call so the code relies on constructor-initialized httpClient; locate and update the NewOpenAPIClient function and the Call method (references: NewOpenAPIClient, Call) to delete those redundant checks and ensure no other code paths can set httpClient to nil.src/php/src/Api/Documentation/Attributes/ApiResponse.php (1)
84-89: Minor: Status codes out of numerical order.Status code
410appears before409in the match expression, which slightly reduces readability.🔧 Suggested reordering for consistency
404 => 'Not found', 405 => 'Method not allowed', - 410 => 'Gone', 409 => 'Conflict', + 410 => 'Gone', 422 => 'Validation error',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Documentation/Attributes/ApiResponse.php` around lines 84 - 89, The match expression in ApiResponse (class ApiResponse) lists HTTP status codes out of numeric order (410 appears before 409); reorder the entries in the response mapping inside the match in ApiResponse.php so status codes are sorted numerically (e.g., 401, 403, 404, 405, 409, 410) to improve readability and consistency.src/php/src/Api/Middleware/AuthenticateApiKey.php (1)
133-136: Minor: Redundant status parameter.The
forbiddenResponse()method definesint $status = 403as a default parameter value. Passingstatus: 403explicitly is unnecessary.🔧 Optional simplification
protected function forbidden(string $message): Response { - return $this->forbiddenResponse($message, status: 403); + return $this->forbiddenResponse($message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Middleware/AuthenticateApiKey.php` around lines 133 - 136, The forbidden() method in AuthenticateApiKey currently calls forbiddenResponse($message, status: 403) redundantly since forbiddenResponse() already defaults $status to 403; update AuthenticateApiKey::forbidden to call forbiddenResponse($message) (remove the explicit status: 403 argument) to simplify the code and avoid redundancy while preserving behavior.src/php/src/Front/Api/VersionedRoutes.php (1)
256-257: Missing curly braces in string interpolation.The variable interpolation at line 257 is missing curly braces around
$this->replacement. While PHP can parse$this->replacementin this context, the inconsistency with line 252 (which uses{$this->replacement}) could cause issues if the property name changes and makes the code less maintainable.♻️ Suggested fix
- $middleware[] = "api.sunset:,$this->replacement"; + $middleware[] = "api.sunset:,{$this->replacement}";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Front/Api/VersionedRoutes.php` around lines 256 - 257, In VersionedRoutes.php update the string interpolation for the middleware entry so it uses explicit braces around the property; replace the current "api.sunset:,$this->replacement" usage with a braced interpolation form (e.g., "api.sunset:,{$this->replacement}") to match the style used elsewhere (see other occurrences of {$this->replacement}) and avoid future ambiguities when the property name changes.src/php/src/Api/Controllers/Api/SeoReportController.php (1)
48-59: Consider logging the exception for observability.The exception is caught but discarded without logging. For production debugging, capturing the underlying error (e.g. connection timeout, DNS failure, SSL error) would help diagnose issues without exposing details to the client.
♻️ Suggested improvement
+use Illuminate\Support\Facades\Log; ... try { $report = $this->seoReportService->analyse($validated['url']); - } catch (RuntimeException) { + } catch (RuntimeException $e) { + Log::warning('SEO report fetch failed', [ + 'url' => $validated['url'], + 'error' => $e->getMessage(), + ]); return $this->errorResponse(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Controllers/Api/SeoReportController.php` around lines 48 - 59, Catch the RuntimeException as a variable and log the exception before returning the sanitized error response: change the catch from catch (RuntimeException) to catch (RuntimeException $e), call your logger (e.g. $this->logger->error(...) or \Log::error(...)) with a brief message referencing SeoReportController::analyse or seoReportService->analyse and include $e->getMessage() and contextual meta (e.g. 'error_code'=>'seo_unavailable','provider'=>'seo') for observability, then return the existing errorResponse unchanged so sensitive details are not exposed to the client.src/php/tests/Feature/ApiVersionParsingTest.php (1)
10-45: Type the Pest closures and the inline$nextcallbacks.This file still has untyped test closures, and the middleware callbacks are missing the expected
Request/Responsesignature.As per coding guidelines, "Use full type hints on all PHP function parameters and return types".Proposed fix
-beforeEach(function () { +beforeEach(function (): void { @@ -it('resolves the api version from an accept-version header with parameters', function () { +it('resolves the api version from an accept-version header with parameters', function (): void { @@ - $response = $middleware->handle($request, fn () => new Response('OK')); + $response = $middleware->handle($request, fn (Request $_): Response => new Response('OK')); @@ -it('resolves the api version from a vendor accept header inside a list', function () { +it('resolves the api version from a vendor accept header inside a list', function (): void { @@ - $response = $middleware->handle($request, fn () => new Response('OK')); + $response = $middleware->handle($request, fn (Request $_): Response => new Response('OK'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/tests/Feature/ApiVersionParsingTest.php` around lines 10 - 45, The Pest test closures and the inline middleware callbacks lack type hints; update the beforeEach and each it(...) closures to declare return type void (e.g., beforeEach(function (): void { ... }) and it(..., function (): void { ... }) and change the middleware next callbacks passed to ApiVersion::handle to typed Request/Response signatures (use fn(Request $request): Response => new Response('OK') or an equivalent function(Request $request): Response) so the Request and Response types (used in the tests around ApiVersion->handle and the $request variable) are explicit and satisfy the coding guidelines.src/php/tests/Feature/ApiVersionServiceTest.php (1)
8-40: Add explicit return types to the Pest callbacks.The new test closures are still untyped. Please add
: voidso this file matches the repo-wide PHP typing rule.As per coding guidelines, "Use full type hints on all PHP function parameters and return types".Proposed fix
-beforeEach(function () { +beforeEach(function (): void { @@ -it('normalises configured versions before reading them', function () { +it('normalises configured versions before reading them', function (): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/tests/Feature/ApiVersionServiceTest.php` around lines 8 - 40, Update the untyped Pest callbacks by adding explicit void return types: change the beforeEach(function () { ... }) closure to beforeEach(function (): void { ... }) and the it('normalises configured versions before reading them', function () { ... }) closure to it('normalises configured versions before reading them', function (): void { ... }); ensure both callback signatures use ": void" so they conform to the repo-wide PHP typing rule.spec_registry.go (1)
48-56: Consider using a map for deduplication to improve performance.The current implementation calls
specRegistryContainsfor each group, resulting in O(n²) complexity when registering multiple groups. For typical usage with a small number of route groups this is acceptable, but if the registry grows, a supplementarymap[string]struct{}for seen keys would improve this.♻️ Optional optimisation using a map for deduplication
var specRegistry struct { mu sync.RWMutex groups []RouteGroup + seen map[string]struct{} } func RegisterSpecGroupsIter(groups iter.Seq[RouteGroup]) { if groups == nil { return } specRegistry.mu.Lock() defer specRegistry.mu.Unlock() + if specRegistry.seen == nil { + specRegistry.seen = make(map[string]struct{}) + } + for group := range groups { if group == nil { continue } - if specRegistryContains(group) { + key := specGroupKey(group) + if _, ok := specRegistry.seen[key]; ok { continue } + specRegistry.seen[key] = struct{}{} specRegistry.groups = append(specRegistry.groups, group) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec_registry.go` around lines 48 - 56, The loop uses specRegistryContains for each group causing O(n²); replace it with a local deduplication set: before iterating `groups` create a `seen` map (e.g. map[interface{}]struct{}) and for each `group` skip if nil or already in `seen`, otherwise add to `seen` and append to `specRegistry.groups`; remove the call to `specRegistryContains` and keep references to the existing symbols `groups`, `group`, and `specRegistry.groups` so the change is localized and avoids quadratic behavior.sunset_test.go (1)
50-50: These test names still miss the required_Goodsuffix.The current names place
_Goodin the middle of the identifier rather than at the end.Based on learnings, "Applies to **/*_test.go : Use
_Good,_Bad, and_Uglysuffixes for Go test function names".Also applies to: 80-80, 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sunset_test.go` at line 50, The test function names place the "_Good" suffix in the middle instead of at the end; rename TestWithSunset_Good_AddsDeprecationHeaders to TestWithSunset_AddsDeprecationHeaders_Good and similarly rename the other affected test functions in this file so their names end with the appropriate suffixes (_Good, _Bad, or _Ugly) to follow the required naming convention; update any references or test invocations to use the new function names (e.g., adjust TestWithSunset_* identifiers and any calls from helper functions).ratelimit_test.go (1)
27-27: Rename these tests to end with the required suffix.
_Goodand_Uglyare currently infixes, not suffixes, so these names miss the repository convention.As per coding guidelines, "Use
_Good,_Bad, and_Uglysuffixes for Go test function names".Also applies to: 92-92, 119-119, 154-154, 189-189, 224-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratelimit_test.go` at line 27, Rename the test functions so the quality marker is a suffix (e.g., move `_Good`, `_Bad`, `_Ugly` to the end); for example change TestWithRateLimit_Good_AllowsBurstThenRejects to TestWithRateLimit_AllowsBurstThenRejects_Good and apply the same transformation to the other test functions that currently contain `_Good`/`_Bad`/`_Ugly` as infixes so they end with those suffixes instead.pkg/provider/registry.go (1)
192-194: CloneChannels()when buildingProviderInfo.
Info()andInfoIter()store the slice returned byChannels()directly. If a provider returns its internal slice, callers can mutate provider state throughProviderInfo, which breaks the snapshot behaviour these helpers are meant to give.slices.Clone(s.Channels())keeps the summary detached.Also applies to: 224-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provider/registry.go` around lines 192 - 194, When building ProviderInfo in Info() and InfoIter(), the code stores the raw slice returned by Streamable.Channels(), allowing callers to mutate provider internals; change those assignments to use slices.Clone(s.Channels()) so ProviderInfo contains a detached copy. Update both places where p.(Streamable) is asserted (the block using s.Channels() at the current diff and the similar block around lines 224-226) to assign info.Channels = slices.Clone(s.Channels()).src/php/src/Api/Documentation/OpenApiBuilder.php (3)
802-814: Unused$valueparameter ininferStringSchema.The
$valueparameter is never used in this method. The static analysis correctly identifies this. The method only uses$keyto infer format hints, making$valueredundant.♻️ Proposed fix to remove unused parameter
- protected function inferStringSchema(string $value, ?string $key): array + protected function inferStringSchema(?string $key): array {Then update the call site at line 734:
- return $this->inferStringSchema($value, $key); + return $this->inferStringSchema($key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Documentation/OpenApiBuilder.php` around lines 802 - 814, The method inferStringSchema has an unused $value parameter; remove that parameter from the method signature and any docblock for inferStringSchema, then update all call sites that pass a value to it (e.g. the place currently calling inferStringSchema(..., $value)) to pass only the $key (or adjust the call to match the new single-parameter signature). Keep the function name inferStringSchema and maintain its return type/behavior (use $key only to infer nullable/format) so nothing else changes.
741-746: Returning an empty array for unrecognised types may produce invalid OpenAPI schema.When
$valueis of an unrecognised type (e.g., a resource handle), this returns[], which is not a valid OpenAPI schema object. Consider returning a generic schema instead.♻️ Proposed fix
- return []; + return ['type' => 'string', 'description' => 'Unknown type'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Documentation/OpenApiBuilder.php` around lines 741 - 746, The method currently returns an empty array for unrecognized types which yields invalid OpenAPI; update the fallback in the function that contains the shown snippet (the branch that calls inferObjectSchema($value) when is_object($value) and otherwise returns []) to return a valid generic schema instead (for example a simple string schema or a permissive object schema, e.g. ['type' => 'string', 'nullable' => true] or ['type' => 'object', 'additionalProperties' => true]); modify the return in that function so callers always receive a valid OpenAPI schema object rather than [].
873-920: Unused$routeparameter in generic request body path.The
$routeparameter is only used for the MCP controller check. In the default case (lines 912-919), it's unused. Consider extracting the MCP-specific logic or documenting that$routeenables future extension points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Documentation/OpenApiBuilder.php` around lines 873 - 920, The buildRequestBody(Route $route, ?object $controller, string $action) method takes a $route parameter that is never used in the default branch; extract the MCP-specific branch into a dedicated helper (e.g. buildMcpRequestBody or isMcpCallTool) and call it from buildRequestBody so the generic path no longer depends on $route, or if you intend to keep the signature for future use, add a short phpdoc note and mark $route as intentionally unused (e.g. /* unused */ $route) to silence reviewers and static analyzers; update references to the controller/action check to use the new helper (preserve the controller instanceof \Core\Api\Controllers\McpApiController && $action === 'callTool' logic).src/php/src/Api/Controllers/McpApiController.php (3)
678-695:uniqid()for JSON-RPC ID is acceptable but consider alternatives.
uniqid()provides reasonable uniqueness for request correlation. For high-concurrency environments, you might consideruniqid('', true)orbin2hex(random_bytes(8))for stronger guarantees.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Controllers/McpApiController.php` around lines 678 - 695, The JSON-RPC id generation in buildToolCallRequest currently uses uniqid() which may collide under high concurrency; replace it with a stronger generator (e.g., use uniqid('', true) for more entropy or use bin2hex(random_bytes(8)) for cryptographic uniqueness) and ensure the returned 'id' value remains a string in the returned array from buildToolCallRequest so request correlation works reliably.
173-207: Duplicated resource mapping logic betweenresources()andenrichResourcesWithContent().Both methods contain nearly identical logic for filtering, mapping resource arrays, and conditionally adding content. Consider extracting a shared helper to reduce duplication.
♻️ Proposed refactor
+ /** + * Map a single resource definition to an API payload. + * + * `@param` array<string, mixed> $resource + * `@param` bool $includeContent + * `@return` array<string, mixed> + */ + protected function mapResourceToPayload(array $resource, bool $includeContent): array + { + $payload = array_filter([ + 'uri' => $resource['uri'] ?? null, + 'path' => $resource['path'] ?? null, + 'name' => $resource['name'] ?? null, + 'description' => $resource['description'] ?? null, + 'mime_type' => $resource['mime_type'] ?? ($resource['mimeType'] ?? null), + ], static fn ($value) => $value !== null); + + if ($includeContent && $this->resourceDefinitionHasContent($resource)) { + $payload['content'] = $this->normaliseResourceContent($resource); + } + + return $payload; + }Then update both
resources()andenrichResourcesWithContent()to use this helper.Also applies to: 243-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Controllers/McpApiController.php` around lines 173 - 207, Extract the duplicated mapping logic in resources() and enrichResourcesWithContent() into a single private helper (e.g. transformResourcePayload(array $resource, bool $includeContent): array) that builds the payload keys ('uri','path','name','description','mime_type') using the same null-filtering, and when $includeContent is true uses $this->resourceDefinitionHasContent($resource) and $this->normaliseResourceContent($resource) to add 'content'; then replace the inline map closures in resources() and enrichResourcesWithContent() to call this helper for each resource and return the resulting collections.
756-768: Server command mapping is hardcoded.The
resolveMcpServerCommand()method uses a hardcoded map. Consider moving this to configuration or a dedicated registry for easier maintenance and extensibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Controllers/McpApiController.php` around lines 756 - 768, The resolveMcpServerCommand method currently hardcodes $commandMap inside the function; replace this by loading the mapping from a configurable source (e.g., a config entry or injected registry) so it can be extended without editing the method. Modify resolveMcpServerCommand(string $server) to obtain the map from a service/config (e.g., $this->mcpCommandMap or config('mcp.server_commands')), then return the mapped value or null as before; ensure the new map is provided via dependency injection or config bootstrap and keep the fallback behavior ($map[$server] ?? null). Include any necessary validation or defaulting when reading the map to avoid runtime errors.src/php/src/Api/Tests/Feature/OpenApiDocumentationComprehensiveTest.php (1)
1195-1218: Test fixtures lack return type hints.Per coding guidelines, PHP functions should have full type hints. The test controller methods return
voidbutTestJsonResource::toArraylacks a return type.♻️ Proposed fix for TestJsonResource
- public function toArray($request): array + public function toArray(mixed $request): arrayAlso applies to: 1270-1279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Tests/Feature/OpenApiDocumentationComprehensiveTest.php` around lines 1195 - 1218, The test fixture class TestJsonResource is missing a return type on its toArray method; update TestJsonResource::toArray to declare and return the appropriate type (e.g., ": array") and ensure its implementation returns an array, and scan other test resource methods referenced by TestOpenApiController (e.g., any response transformers) to add explicit return type hints consistent with their docblocks so all controller methods (index, show, store, update, destroy) tie to typed resource outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f572b61f-101b-497e-b531-5259297dd48d
📒 Files selected for processing (122)
api.goapi_test.goauthentik.goauthentik_test.gobridge.gobridge_test.gocache.gocache_config.gocache_test.goclient.goclient_test.gocmd/api/cmd.gocmd/api/cmd_args.gocmd/api/cmd_sdk.gocmd/api/cmd_spec.gocmd/api/cmd_test.gocmd/api/spec_builder.gocmd/api/spec_groups_iter.gocodegen.gocodegen_test.godocs/architecture.mddocs/history.mddocs/index.mdexport.goexport_test.gogo-io/go.modgo-io/local.gogo-log/error.gogo-log/go.modgo.modgraphql.gographql_config_test.gographql_test.gogroup.goi18n.goi18n_test.gomiddleware.gomiddleware_test.gomodernization_test.goopenapi.goopenapi_test.gooptions.gopkg/provider/provider.gopkg/provider/proxy.gopkg/provider/proxy_internal_test.gopkg/provider/proxy_test.gopkg/provider/registry.gopkg/provider/registry_test.goratelimit.goratelimit_test.goresponse.goresponse_meta.goruntime_config.goservers.gospec_builder_helper.gospec_builder_helper_internal_test.gospec_builder_helper_test.gospec_registry.gospec_registry_test.gosrc/php/src/Api/Concerns/HasApiResponses.phpsrc/php/src/Api/Controllers/Api/EntitlementApiController.phpsrc/php/src/Api/Controllers/Api/SeoReportController.phpsrc/php/src/Api/Controllers/Api/UnifiedPixelController.phpsrc/php/src/Api/Controllers/Api/WebhookSecretController.phpsrc/php/src/Api/Controllers/Api/WebhookTemplateController.phpsrc/php/src/Api/Controllers/McpApiController.phpsrc/php/src/Api/Documentation/Attributes/ApiResponse.phpsrc/php/src/Api/Documentation/DocumentationController.phpsrc/php/src/Api/Documentation/Extensions/ApiKeyAuthExtension.phpsrc/php/src/Api/Documentation/Extensions/SunsetExtension.phpsrc/php/src/Api/Documentation/Extensions/VersionExtension.phpsrc/php/src/Api/Documentation/OpenApiBuilder.phpsrc/php/src/Api/Documentation/Routes/docs.phpsrc/php/src/Api/Documentation/Views/stoplight.blade.phpsrc/php/src/Api/Documentation/config.phpsrc/php/src/Api/Exceptions/RateLimitExceededException.phpsrc/php/src/Api/Middleware/AuthenticateApiKey.phpsrc/php/src/Api/Middleware/CheckApiScope.phpsrc/php/src/Api/Middleware/EnforceApiScope.phpsrc/php/src/Api/Resources/ErrorResource.phpsrc/php/src/Api/Routes/api.phpsrc/php/src/Api/Services/ApiUsageService.phpsrc/php/src/Api/Services/SeoReportService.phpsrc/php/src/Api/Tests/Feature/ApiUsageTest.phpsrc/php/src/Api/Tests/Feature/DocumentationStoplightTest.phpsrc/php/src/Api/Tests/Feature/EntitlementsEndpointTest.phpsrc/php/src/Api/Tests/Feature/McpApiControllerTest.phpsrc/php/src/Api/Tests/Feature/McpResourceTest.phpsrc/php/src/Api/Tests/Feature/McpServerDetailTest.phpsrc/php/src/Api/Tests/Feature/OpenApiDocumentationComprehensiveTest.phpsrc/php/src/Api/Tests/Feature/OpenApiDocumentationTest.phpsrc/php/src/Api/Tests/Feature/OpenApiVersionHeadersTest.phpsrc/php/src/Api/Tests/Feature/PixelEndpointTest.phpsrc/php/src/Api/Tests/Feature/SeoReportEndpointTest.phpsrc/php/src/Api/Tests/Feature/WebhookSecretRoutesTest.phpsrc/php/src/Api/config.phpsrc/php/src/Front/Api/ApiVersionService.phpsrc/php/src/Front/Api/Middleware/ApiSunset.phpsrc/php/src/Front/Api/Middleware/ApiVersion.phpsrc/php/src/Front/Api/README.mdsrc/php/src/Front/Api/VersionedRoutes.phpsrc/php/src/Website/Api/Controllers/DocsController.phpsrc/php/src/Website/Api/Routes/web.phpsrc/php/src/Website/Api/View/Blade/layouts/docs.blade.phpsrc/php/src/Website/Api/View/Blade/stoplight.blade.phpsrc/php/tests/Feature/ApiSunsetTest.phpsrc/php/tests/Feature/ApiVersionHeadersTest.phpsrc/php/tests/Feature/ApiVersionParsingTest.phpsrc/php/tests/Feature/ApiVersionServiceTest.phpsrc/php/tests/Feature/VersionedRoutesTest.phpsse.gosse_test.gosunset.gosunset_test.goswagger.goswagger_internal_test.goswagger_test.gotimeout_test.gotracing.gotransport.gowebsocket.gowebsocket_test.go
Go: - codegen: pass trimmed specPath to buildArgs instead of raw g.SpecPath - cmd/sdk: use local resolvedSpecFile to avoid mutating flag variable per-invocation - export: write to temp file + atomic rename to prevent destination truncation on failure - openapi: gate effectiveGraphQLPath/SwaggerPath/WSPath/SSEPath on enable flags; use effectiveSwaggerPath in effectiveAuthentikPublicPaths - cache: reject oversized replacement before mutating LRU state for existing keys - ratelimit: move setRateLimitHeaders before c.Next() so headers are sent; hash credential headers with SHA-256 to avoid storing raw secrets; prefer validated principal from context - response_meta: track size separately from body buffer so Size() is accurate after body rewrites and in passthrough mode - bridge: limit request body reads with http.MaxBytesReader (10 MiB); allow missing data key in ValidateResponse for nil/zero success responses; update recorder status in writeErrorResponse - pkg/provider/proxy: validate target scheme and host after url.Parse to catch hostless inputs - cmd_test: snapshot/restore global spec registry in TestAPISpecCmd_Good_RegisteredSpecGroups PHP: - HasApiResponses.php, config.php: add declare(strict_types=1) - RateLimitExceededException: validate Origin against cors.allowed_origins before reflecting in CORS header - ApiUsageService: import and use Core\Api\Models\ApiKey instead of fully-qualified Mod\ path - SeoReportService: add SSRF protection (scheme check, private-IP rejection); add .throw() for HTTP error handling; disable automatic redirects Co-Authored-By: Virgil <virgil@lethean.io>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cache.go (1)
63-76:⚠️ Potential issue | 🟠 MajorTOCTOU race when evicting expired entries.
Between releasing the lock (line 58) and re-acquiring it (line 64), another goroutine may replace the entry via
set(). The current code then:
- Decrements
currentBytesby the stale entry's size (line 69), corrupting the byte counter- Deletes the new valid entry from
entries(line 73)Re-verify the entry pointer is unchanged before proceeding with eviction.
🔧 Proposed fix
if time.Now().After(entry.expires) { s.mu.Lock() + currentEntry, stillExists := s.entries[key] + if !stillExists || currentEntry != entry { + // Entry was replaced or removed; nothing to clean up. + s.mu.Unlock() + return nil + } if elem, exists := s.index[key]; exists { s.order.Remove(elem) delete(s.index, key) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache.go` around lines 63 - 76, The eviction path is vulnerable to a TOCTOU race: after the time check but before holding the mutex you must re-verify the cached entry is still the same object to avoid removing a newly-set entry and mis-updating s.currentBytes. Fix: inside the existing mutex-protected block in the expiry branch, read current := s.entries[key] and compare pointer/identifier to the previously-captured entry variable; if they differ, simply unlock and return (do not decrement s.currentBytes or delete), otherwise proceed with s.order.Remove(elem), delete(s.index, key), s.currentBytes -= entry.size (clamped to >=0), delete(s.entries, key) and unlock. Reference symbols: entry, key, s.entries, s.index, s.order, s.currentBytes, and set().
♻️ Duplicate comments (1)
bridge.go (1)
705-721:⚠️ Potential issue | 🟠 MajorKeep the recorder body and headers in sync with the replacement error response.
After
reset(),writeErrorResponse()still writes straight to the underlying writer.Status()andWritten()are now correct, butHeader()stays empty andSize()stays0, so post-handler middleware and metrics still observe the wrong response metadata. Buffer the JSON into the recorder andcommit(), or mirrorw.headersandw.bodybefore returning.Proposed fix
func (w *toolResponseRecorder) writeErrorResponse(status int, resp Response[any]) { data, err := json.Marshal(resp) if err != nil { w.status = http.StatusInternalServerError w.wroteHeader = true http.Error(w.ResponseWriter, "internal server error", http.StatusInternalServerError) return } - // Update recorder state so middleware observing c.Writer.Status() or - // Written() sees the correct values after an error response is emitted. + // Keep recorder state aligned with the replacement response. w.status = status w.wroteHeader = true - - w.ResponseWriter.Header().Set("Content-Type", "application/json") - w.ResponseWriter.WriteHeader(status) - _, _ = w.ResponseWriter.Write(data) + if w.headers == nil { + w.headers = make(http.Header) + } + w.headers.Set("Content-Type", "application/json") + w.body.Reset() + _, _ = w.body.Write(data) + w.commit() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridge.go` around lines 705 - 721, writeErrorResponse in toolResponseRecorder writes directly to the underlying ResponseWriter after reset(), leaving the recorder's headers/body/Size() out of sync; change writeErrorResponse to marshal the JSON into the recorder's buffer (w.body) and update/mirror w.headers from the replacement headers, set w.status and w.wroteHeader, then call commit() (or otherwise copy w.body and w.headers into the underlying ResponseWriter) so Status(), Written(), Header() and Size() reflect the error response; reference toolResponseRecorder.writeErrorResponse, reset(), commit(), w.headers, w.body, Status(), Written(), Header(), Size() in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bridge.go`:
- Around line 293-320: ValidateResponse currently uses json.Unmarshal into
envelope which coerces numbers to float64; replace that with a json.Decoder on
body and call dec.UseNumber() before Decode(&envelope) so numeric precision is
preserved (matching Validate). In other words, in the ValidateResponse function
use json.NewDecoder(bytes.NewReader(body)), call UseNumber(), Decode into the
envelope map[string]any, and keep the same error handling and subsequent logic
(including encoding/decoding of data) to ensure large integers are not lost.
In `@cmd/api/cmd_sdk.go`:
- Around line 99-100: sdkSpecGroupsIter() currently inserts an empty tool group
because goapi.NewToolBridge("/tools") only creates a stub; fix by populating the
bridge with the actual tool routes before adding it to the spec groups: obtain
the registered tool route groups (or tool handlers) from the existing
registry/registry function you use elsewhere, call Add(...) on the instance
returned by goapi.NewToolBridge("/tools") to attach those routes, then include
that populated bridge in sdkSpecGroupsIter(); ensure the same change is applied
to the other occurrence that mirrors this logic.
In `@go.mod`:
- Line 8: Update the import path in the four listed files to match the migrated
module path: replace any import of "forge.lthn.ai/core/cli/pkg/cli" with
"dappco.re/go/core/cli/pkg/cli" in cmd/api/cmd.go, cmd/api/cmd_test.go,
cmd/api/cmd_sdk.go, and cmd/api/cmd_spec.go so the code matches go.mod's
dappco.re/go/core/cli module and builds successfully.
In `@openapi.go`:
- Around line 500-505: operationResponses currently merges cacheSuccessHeaders
into GET responses unconditionally; change the signature to accept a
cacheEnabled boolean (or a context flag) and only merge cacheSuccessHeaders()
when that flag is true. Update call sites to pass the actual cache-enabled state
into operationResponses, and modify other response-builder functions that call
cacheSuccessHeaders (the other response builders noted in the review) to follow
the same pattern: thread a cache-enabled parameter through and conditionally
include cacheSuccessHeaders only when caching is enabled. Ensure you continue to
use documentedResponseHeaders, mergeHeaders, standardResponseHeaders, and
rateLimitSuccessHeaders as before, but guard cacheSuccessHeaders with the
cacheEnabled flag.
- Around line 1555-1567: The loop over groups can panic on typed-nil RouteGroup
values because it only checks "g == nil" before calling g.Name() and
g.BasePath(); replace that check with a call to isNilRouteGroup(g) (alongside
the existing isHiddenRouteGroup check) so typed-nil values are detected safely;
update the loop that builds preparedRouteGroup entries (where you call g.Name(),
g.BasePath(), and collectRouteDescriptions(g)) to skip any group for which
isNilRouteGroup(g) returns true.
In `@ratelimit.go`:
- Around line 193-220: The bucket-key selection currently checks client IP
before credential headers, making the credential branches in the function (use
of c.GetHeader("X-API-Key") and bearerTokenFromHeader(...)) effectively
unreachable; reorder the fallback so that after the validated principal/userID
checks you first check and hash credentials (X-API-Key then Authorization via
bearerTokenFromHeader) and only then fall back to c.ClientIP() and
c.Request.RemoteAddr; preserve trimming, sha256.Sum256 and hex.EncodeToString
usage and keep principal/userID checks first to match the public WithRateLimit
contract.
In `@src/php/src/Api/Exceptions/RateLimitExceededException.php`:
- Around line 53-65: The render() method in RateLimitExceededException contains
ad-hoc CORS handling that does exact-match checks against
config('cors.allowed_origins') and manually sets
Vary/Access-Control-Allow-Origin, which ignores allowed_origins_patterns and
other CORS options; remove that entire conditional ($request !== null) block and
stop trying to compute CORS headers inside RateLimitExceededException::render();
instead let the existing PublicApiCors middleware (or the framework's CORS
service) apply proper CORS headers for error responses (or, if absolutely
necessary, inject and call the app's CORS service/middleware logic rather than
using config() and manual header manipulation).
In `@src/php/src/Api/Services/SeoReportService.php`:
- Around line 203-220: The extractCharset function currently falls back to
returning the whole Content-Type string from extractMetaContent (e.g.,
"text/html; charset=utf-8"); update extractCharset to parse and return only the
charset token when extractMetaContent returns a content-type value: call
extractMetaContent($xpath, 'content-type', 'http-equiv') into a variable, then
run a case-insensitive regex (e.g., /charset\s*=\s*("?)([^\s;"]+)\1/i) or
equivalent string-splitting to extract the second capture (the actual charset),
trim and return it (or null if not found); keep extractMetaContent untouched and
reference extractCharset and extractMetaContent in the change to locate the
code.
- Around line 385-404: The current validation misses when the URL host is an IP
literal because dns_get_record returns empty and gethostbyname returns the same
value; update SeoReportService to explicitly detect and validate IP literals:
normalize $host (trim surrounding brackets for IPv6), use filter_var($host,
FILTER_VALIDATE_IP) to detect an IP literal before or alongside the
dns_get_record/gethostbyname logic, and if it's an IP call isPrivateIp($ip) and
throw the RuntimeException when private/reserved; otherwise continue with the
existing DNS lookup and gethostbyname fallback as before.
- Around line 440-458: The IPv6 branch in SeoReportService is missing handling
for IPv4-mapped IPv6 addresses (e.g. ::ffff:127.0.0.1) so those skip the IPv4
private-range checks, and the IPv4 private list also omits 0.0.0.0/8; update the
IP-check routine (the method using $ip, $packed, $prefix2 and $privateRanges) to
detect IPv4-mapped IPv6 by checking the packed bytes for the ::ffff: prefix
(first 12 bytes zero + next two bytes 0xff 0xff), extract the trailing 4 bytes
as an IPv4 address and run the existing IPv4 private-range logic against it, and
add the 0.0.0.0/8 range to the $privateRanges array so the IPv4 checks block
that reserved network as well.
---
Outside diff comments:
In `@cache.go`:
- Around line 63-76: The eviction path is vulnerable to a TOCTOU race: after the
time check but before holding the mutex you must re-verify the cached entry is
still the same object to avoid removing a newly-set entry and mis-updating
s.currentBytes. Fix: inside the existing mutex-protected block in the expiry
branch, read current := s.entries[key] and compare pointer/identifier to the
previously-captured entry variable; if they differ, simply unlock and return (do
not decrement s.currentBytes or delete), otherwise proceed with
s.order.Remove(elem), delete(s.index, key), s.currentBytes -= entry.size
(clamped to >=0), delete(s.entries, key) and unlock. Reference symbols: entry,
key, s.entries, s.index, s.order, s.currentBytes, and set().
---
Duplicate comments:
In `@bridge.go`:
- Around line 705-721: writeErrorResponse in toolResponseRecorder writes
directly to the underlying ResponseWriter after reset(), leaving the recorder's
headers/body/Size() out of sync; change writeErrorResponse to marshal the JSON
into the recorder's buffer (w.body) and update/mirror w.headers from the
replacement headers, set w.status and w.wroteHeader, then call commit() (or
otherwise copy w.body and w.headers into the underlying ResponseWriter) so
Status(), Written(), Header() and Size() reflect the error response; reference
toolResponseRecorder.writeErrorResponse, reset(), commit(), w.headers, w.body,
Status(), Written(), Header(), Size() in your changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d891442-a7d2-4302-80e8-6a1d88e324e5
📒 Files selected for processing (17)
bridge.gocache.gocmd/api/cmd_sdk.gocmd/api/cmd_test.gocodegen.goexport.gogo.modopenapi.gopkg/provider/proxy.goratelimit.goresponse_meta.gosrc/php/src/Api/Concerns/HasApiResponses.phpsrc/php/src/Api/Documentation/Views/stoplight.blade.phpsrc/php/src/Api/Exceptions/RateLimitExceededException.phpsrc/php/src/Api/Services/ApiUsageService.phpsrc/php/src/Api/Services/SeoReportService.phpsrc/php/src/Api/config.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/php/src/Api/Documentation/Views/stoplight.blade.php
- codegen.go
- src/php/src/Api/Concerns/HasApiResponses.php
Go: - cache: fix TOCTOU race in get() — re-verify entry pointer under lock before evicting to prevent corrupting s.currentBytes and removing a newly-set entry - bridge: fix writeErrorResponse recorder out of sync — buffer into w.body/ w.headers and call commit() so Status(), Header(), Size() reflect error response - bridge: fix ValidateResponse number precision — use json.Decoder+UseNumber for initial envelope decode to preserve large integers (matches Validate path) - ratelimit: fix unreachable credential branches — move X-API-Key and Authorization hashing before IP fallback so NAT'd clients are bucketed by key - openapi: gate cacheSuccessHeaders on sb.CacheEnabled flag, not just method==get - openapi: use isNilRouteGroup in prepareRouteGroups to catch typed-nil RouteGroup PHP: - RateLimitExceededException: remove ad-hoc CORS handling — let framework CORS middleware apply correct headers for all responses including errors - SeoReportService.extractCharset: parse charset token from Content-Type value instead of returning the full "text/html; charset=utf-8" string - SeoReportService: validate IP literals directly with filter_var before DNS lookup so ::ffff:127.0.0.1-style hosts don't bypass private-IP checks - SeoReportService.isPrivateIp: extract isPrivateIpv4 helper; handle IPv4-mapped IPv6 (::ffff::/96) by checking embedded IPv4 against private ranges; add 0.0.0.0/8 to private range list Co-Authored-By: Virgil <virgil@lethean.io>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
bridge.go (2)
498-506: Consider caching compiled regex patterns.The
patternconstraint compiles the regex on every validation call. For high-throughput scenarios with repeated validations against the same schema, pre-compiling and caching patterns would improve performance.This is acceptable for typical use cases but worth noting if validation becomes a hot path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridge.go` around lines 498 - 506, The ValidateSchema block currently recompiles the regex on each call by using regexp.Compile(pattern); to fix, introduce a thread-safe cache (e.g., sync.Map or a map+sync.RWMutex) keyed by the pattern string and store compiled *regexp.Regexp values, then change ToolBridge.ValidateSchema to first look up the compiled regexp from the cache and only compile/store on miss (propagating compile errors as before); ensure cache access is concurrent-safe and use the cached regexp for re.MatchString(value) so repeated validations reuse the precompiled pattern.
510-518: Potential precision loss in numeric constraint comparisons for large integers.The
numericValue()function converts all numeric types tofloat64for comparison innumericLessThan/numericGreaterThan. Forint64values beyond 2^53, this conversion loses precision, potentially causing incorrect constraint validation (e.g., a value of9007199254740993might incorrectly pass amaximum: 9007199254740992constraint).Consider comparing integers directly when both the value and limit are integral, or document this limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridge.go` around lines 510 - 518, The current numeric comparisons lose precision because numericValue/numericLessThan/numericGreaterThan convert everything to float64; update the numeric comparison logic so when both the input value and schema limit are integral you perform an integer-safe comparison (e.g., detect integral values via a new isIntegralNumeric helper, parse/cast them to big.Int or an exact integer type and compare with math/big), and only fall back to float64 comparisons when either side is non-integral; update numericLessThan and numericGreaterThan (and any helpers called by validateNumericConstraints) to implement this integer-path comparison so large int64/json.Number values beyond 2^53 are compared exactly.src/php/src/Api/Services/SeoReportService.php (1)
412-430: Consider failing closed when hostname resolution yields no records.If both
dns_get_record()andgethostbyname()fail to resolve any addresses,$recordsremains empty and validation passes silently. While the HTTP request would subsequently fail, rejecting unresolvable hostnames at validation time is a more defensive approach.Proposed fix
foreach ($records as $record) { $ip = $record['ip'] ?? $record['ipv6'] ?? null; if ($ip === null) { continue; } if ($this->isPrivateIp($ip)) { throw new RuntimeException('The supplied URL resolves to a private or reserved address.'); } } + + // Fail closed: reject URLs whose hostnames cannot be resolved. + if (empty($records)) { + throw new RuntimeException('Unable to resolve the supplied URL hostname.'); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/php/src/Api/Services/SeoReportService.php` around lines 412 - 430, The validation currently allows unresolvable hostnames to pass because $records can remain empty after dns_get_record and gethostbyname; update the SeoReportService validation (in the block that sets $records and calls gethostbyname) to fail closed by throwing a RuntimeException when no addresses were resolved (i.e., $records is empty after both lookups) before entering the foreach loop; keep the existing private/reserved address check via isPrivateIp unchanged and use a clear message like "The supplied URL could not be resolved to any address." to distinguish resolution failures from private IP rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cache.go`:
- Around line 38-45: The constructor newCacheStore currently treats non-positive
maxEntries and maxBytes as "no limit", which allows WithCacheLimits(ttl, 0, 0)
to create an unbounded cache; change the logic to fail closed by validating
inputs before creating the store: in the call site that invokes newCacheStore
and inside newCacheStore itself, detect when both maxEntries <= 0 and maxBytes
<= 0 and either return an error (or nil) to prevent creation or skip registering
the middleware entirely; update callers to handle the error/skip path
accordingly so at least one of maxEntries or maxBytes must be positive before
instantiating a cacheStore (referencing newCacheStore and the caller that
forwards maxEntries/maxBytes).
- Around line 202-227: When refreshCachedResponseMeta(entry.body, meta) is used
(i.e., meta != nil) the cached body may change, so before replaying
entry.headers in the store.get branch you must remove stale validators like
"ETag", "Digest", and "Content-MD5" (or recompute and set new values) so they
don't misrepresent the rewritten body; modify the code around
GetRequestMeta/refreshCachedResponseMeta to drop those header keys from
entry.headers (or replace them with freshly computed values) before calling
c.Writer.Header().Add and before setting Content-Length/WriteHeader/Write.
- Around line 50-83: In cacheStore.get, currently s.order.MoveToFront(elem) is
called before checking expiry which can promote an expired entry and cause
races; change the logic so you only MoveToFront for an entry after verifying it
exists and is not expired, and perform the expiry check and potential eviction
within the same critical section (hold s.mu) to avoid releasing the lock between
read and remove; specifically, in cacheStore.get refer to s.entries[key],
s.index[key], s.order.MoveToFront, s.order.Remove, s.currentBytes and ensure
MoveToFront is executed only after confirming time.Now().Before(entry.expires)
and that evictions (remove from s.order, delete from s.index/entries and adjust
s.currentBytes) occur while s.mu is held.
In `@ratelimit.go`:
- Around line 57-66: The buckets map can grow unbounded; add a hard cap and an
overflow bucket to prevent memory blowups: introduce a maxBuckets constant and
check len(s.buckets) before inserting a new rateLimitBucket in rateLimitStore
(the code around s.buckets, rateLimitBucket, rateLimitStaleAfter, s.limit), and
if the cap is reached either reuse/route new keys to a single overflow bucket
(e.g., an "__overflow__" bucket stored on the store) or evict the
least-recently-seen stale entry before inserting; ensure this check is performed
in the same critical section protected by s.mu and that lastSeen and tokens are
initialized the same way as normal buckets.
- Around line 188-191: Update the top comment for clientRateLimitKey in
ratelimit.go to reflect the actual behavior: state that the function prefers a
validated principal from context first, then falls back to raw credential
headers (which are hashed with SHA-256) and only uses the client IP as the final
fallback; remove or change the phrase that credentials are used “as a last
resort” so the comment matches the code path in clientRateLimitKey.
---
Nitpick comments:
In `@bridge.go`:
- Around line 498-506: The ValidateSchema block currently recompiles the regex
on each call by using regexp.Compile(pattern); to fix, introduce a thread-safe
cache (e.g., sync.Map or a map+sync.RWMutex) keyed by the pattern string and
store compiled *regexp.Regexp values, then change ToolBridge.ValidateSchema to
first look up the compiled regexp from the cache and only compile/store on miss
(propagating compile errors as before); ensure cache access is concurrent-safe
and use the cached regexp for re.MatchString(value) so repeated validations
reuse the precompiled pattern.
- Around line 510-518: The current numeric comparisons lose precision because
numericValue/numericLessThan/numericGreaterThan convert everything to float64;
update the numeric comparison logic so when both the input value and schema
limit are integral you perform an integer-safe comparison (e.g., detect integral
values via a new isIntegralNumeric helper, parse/cast them to big.Int or an
exact integer type and compare with math/big), and only fall back to float64
comparisons when either side is non-integral; update numericLessThan and
numericGreaterThan (and any helpers called by validateNumericConstraints) to
implement this integer-path comparison so large int64/json.Number values beyond
2^53 are compared exactly.
In `@src/php/src/Api/Services/SeoReportService.php`:
- Around line 412-430: The validation currently allows unresolvable hostnames to
pass because $records can remain empty after dns_get_record and gethostbyname;
update the SeoReportService validation (in the block that sets $records and
calls gethostbyname) to fail closed by throwing a RuntimeException when no
addresses were resolved (i.e., $records is empty after both lookups) before
entering the foreach loop; keep the existing private/reserved address check via
isPrivateIp unchanged and use a clear message like "The supplied URL could not
be resolved to any address." to distinguish resolution failures from private IP
rejections.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57870df8-0088-45e2-9252-5308147fb298
📒 Files selected for processing (6)
bridge.gocache.goopenapi.goratelimit.gosrc/php/src/Api/Exceptions/RateLimitExceededException.phpsrc/php/src/Api/Services/SeoReportService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- openapi.go
- cache: fix LRU race — check expiry before MoveToFront within same lock critical section; previously a stale entry could be promoted before expiry was verified - cache: drop stale ETag/Content-Md5/Digest headers when body is rewritten by refreshCachedResponseMeta to avoid misrepresenting the new body to clients - ratelimit: cap buckets map at 100k entries with stale-eviction fallback and shared overflow bucket to prevent unbounded memory growth under high-cardinality traffic - ratelimit: fix clientRateLimitKey comment — credentials are tried second (before IP), not as a "last resort" Co-Authored-By: Virgil <virgil@lethean.io>
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
1 participant
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
Improvements