chore: housekeeping April 2026#134
Merged
Merged
Conversation
WalkthroughThe changes refactor context key handling across the proxy pipeline to use a typed constant ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/adapter/proxy/olla/service.go (1)
382-385: Minor: preferconstants.HeaderXModelover the literal"X-Model".
internal/adapter/proxy/sherpa/service_retry.go:90usesproxyReq.Header.Set(constants.HeaderXModel, model)for the same purpose. The literal here is pre-existing, but while you're touching this block to switch to the typed context key, aligning the header constant too would remove the last string-literal in this path.Proposed tweak
if model, ok := ctx.Value(constants.ContextModelKey).(string); ok && model != "" { - proxyReq.Header.Set("X-Model", model) + proxyReq.Header.Set(constants.HeaderXModel, model) stats.Model = model }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/proxy/olla/service.go` around lines 382 - 385, Replace the hard-coded header string in the block that reads from ctx.Value(constants.ContextModelKey) and sets the header using proxyReq.Header.Set("X-Model", model) with the shared constant by calling proxyReq.Header.Set(constants.HeaderXModel, model); leave stats.Model = model and the context lookup unchanged so behavior is identical but the header name is centralized.internal/core/constants/context.go (1)
14-28: Optional:ContextModelAliasMapKeystill uses a plain string.The rationale in the new comment (avoiding collisions with third-party middleware using a plain
"model"key) applies equally toContextModelAliasMapKey = "model_alias_map"on line 28, which remains an untyped string. Consider migrating it tocontextKey("model_alias_map")in a follow-up for consistency with the rest of the sticky-session keys in this block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/constants/context.go` around lines 14 - 28, ContextModelAliasMapKey is defined as a plain string ("model_alias_map") while other context keys use the typed contextKey to avoid collisions; change ContextModelAliasMapKey to use contextKey("model_alias_map") so it matches the pattern used by ContextModelKey, ContextStickyKeyKey, etc.; update the declaration of ContextModelAliasMapKey to call contextKey with the same identifier and ensure any consumers that compare key types still use the typed key symbol ContextModelAliasMapKey.internal/adapter/proxy/olla/service_leak_test.go (1)
370-391: Covers the double-close regression; minor suggestion: assert goroutine exit.The test validates that
sync.Onceprevents the secondclose(cleanupStop)from panicking. SinceCleanup()closescleanupStopand stops the ticker, the backgroundcleanupLoopgoroutine returns — but the test exits before verifying it. A short sleep + goroutine count check (as inTestCleanupLoop_ExitsCleanly) would additionally guarantee the guard doesn't accidentally skip channel closure on the first call. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/proxy/olla/service_leak_test.go` around lines 370 - 391, TestCleanup_DoubleInvoke currently only calls s.Cleanup() twice; enhance it to assert the background cleanupLoop goroutine actually exits after the first Cleanup by waiting briefly and verifying goroutine exit (follow the pattern used in TestCleanupLoop_ExitsCleanly): after starting cleanupLoop and calling s.Cleanup(), sleep a short duration, then check that the goroutine count (or other exit signal) has decreased/indicates the cleanupLoop returned before calling s.Cleanup() the second time; reference symbols: TestCleanup_DoubleInvoke, Service, cleanupLoop, Cleanup, cleanupStop, cleanupTicker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 3: The go.mod Go version directive is 1.25.0 but CI uses GO_VERSION:
1.24.x, causing a mismatch; either update the CI workflow GO_VERSION entries (in
the CI and release workflows) to 1.25.x to match the go.mod directive, or change
the go.mod "go" directive back to 1.24 (e.g., go 1.24.0) so both targets align;
locate the go.mod "go" line and the CI workflow environment entries named
GO_VERSION and make them identical across the repo before merging.
---
Nitpick comments:
In `@internal/adapter/proxy/olla/service_leak_test.go`:
- Around line 370-391: TestCleanup_DoubleInvoke currently only calls s.Cleanup()
twice; enhance it to assert the background cleanupLoop goroutine actually exits
after the first Cleanup by waiting briefly and verifying goroutine exit (follow
the pattern used in TestCleanupLoop_ExitsCleanly): after starting cleanupLoop
and calling s.Cleanup(), sleep a short duration, then check that the goroutine
count (or other exit signal) has decreased/indicates the cleanupLoop returned
before calling s.Cleanup() the second time; reference symbols:
TestCleanup_DoubleInvoke, Service, cleanupLoop, Cleanup, cleanupStop,
cleanupTicker.
In `@internal/adapter/proxy/olla/service.go`:
- Around line 382-385: Replace the hard-coded header string in the block that
reads from ctx.Value(constants.ContextModelKey) and sets the header using
proxyReq.Header.Set("X-Model", model) with the shared constant by calling
proxyReq.Header.Set(constants.HeaderXModel, model); leave stats.Model = model
and the context lookup unchanged so behavior is identical but the header name is
centralized.
In `@internal/core/constants/context.go`:
- Around line 14-28: ContextModelAliasMapKey is defined as a plain string
("model_alias_map") while other context keys use the typed contextKey to avoid
collisions; change ContextModelAliasMapKey to use contextKey("model_alias_map")
so it matches the pattern used by ContextModelKey, ContextStickyKeyKey, etc.;
update the declaration of ContextModelAliasMapKey to call contextKey with the
same identifier and ensure any consumers that compare key types still use the
typed key symbol ContextModelAliasMapKey.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d0f9115-475e-4ea7-8be3-8bc53a4f81bc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.modinternal/adapter/health/checker.gointernal/adapter/health/checker_test.gointernal/adapter/proxy/core/retry.gointernal/adapter/proxy/olla/service.gointernal/adapter/proxy/olla/service_leak_test.gointernal/adapter/proxy/proxy_headers_test.gointernal/adapter/proxy/sherpa/service_retry.gointernal/app/handlers/handler_translation.gointernal/app/handlers/handler_translation_alias_test.gointernal/core/constants/context.go
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.
Various changes for dependabot and minor tweaks from internal review before next major release.
Summary by CodeRabbit
Release Notes
Chores
Bug Fixes
Tests