Enrich truncated OpenRouter descriptions#10
Conversation
📝 WalkthroughWalkthroughAdds bounded OpenRouter model description enrichment: a new ChangesOpenRouter description enrichment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adminapi/handlers.go`:
- Around line 672-677: The lookupCapsFor function can return a zero-value struct
when a lookup misses or encounters an error, and the current code proceeds to
modify and persist this empty struct via PutCapabilities, which overwrites
existing capability data (such as pricing, flags, and context length) with
zeros. After the lookupCapsFor call on line 672, add a check to validate that
the returned capabilities struct is valid and not a zero-value before proceeding
with the modification and PutCapabilities call on line 677. If the lookup fails
or misses, skip the persistence operation entirely to prevent data loss.
In `@internal/llm/capabilities.go`:
- Around line 191-193: The prefix variable is being sliced at the byte level
with prefix[:120], which can split multi-byte UTF-8 characters and cause
subsequent strings.HasPrefix comparisons to fail. Convert the prefix string to
runes using []rune(), slice to 120 runes instead of 120 bytes, then convert back
to a string using string() to ensure proper UTF-8 character boundaries are
respected.
- Around line 160-168: Replace the simple defer statement for resp.Body.Close()
with explicit error handling that captures and checks for close errors, rather
than silently ignoring them. Additionally, ensure the error variable used in the
fmt.Errorf call for the io.ReadAll operation matches the actual variable name
being assigned (if you're using readErr for the read operation error, update the
error message to reference readErr instead of err). This applies to the HTTP
request handling block where openRouterHTTPClient.Do(req) is called and
resp.Body is read with io.LimitReader.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63200344-4167-4d6e-8400-bd07da78a8c2
📒 Files selected for processing (3)
internal/adminapi/handlers.gointernal/llm/capabilities.gointernal/llm/capabilities_test.go
| caps := s.lookupCapsFor(ctx, provider, models[i].ID) | ||
| caps.Description = full | ||
| if caps.Name == "" { | ||
| caps.Name = models[i].Name | ||
| } | ||
| if err := s.capStore.PutCapabilities(ctx, provider, models[i].ID, caps); err != nil { |
There was a problem hiding this comment.
Prevent cache row clobbering when capability lookup misses.
At Line 672, lookupCapsFor can return a zero-value struct on miss/error; then Line 677 upserts that struct, which can overwrite persisted capability fields (pricing, flags, context length) with zeros.
Proposed fix
- caps := s.lookupCapsFor(ctx, provider, models[i].ID)
+ caps, found, err := s.capStore.GetCapabilities(ctx, provider, models[i].ID)
+ if err != nil {
+ s.logger.Debug("openrouter description cache lookup failed", "model", models[i].ID, "err", err)
+ continue
+ }
+ if !found {
+ continue
+ }
caps.Description = full
if caps.Name == "" {
caps.Name = models[i].Name
}
if err := s.capStore.PutCapabilities(ctx, provider, models[i].ID, caps); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/adminapi/handlers.go` around lines 672 - 677, The lookupCapsFor
function can return a zero-value struct when a lookup misses or encounters an
error, and the current code proceeds to modify and persist this empty struct via
PutCapabilities, which overwrites existing capability data (such as pricing,
flags, and context length) with zeros. After the lookupCapsFor call on line 672,
add a check to validate that the returned capabilities struct is valid and not a
zero-value before proceeding with the modification and PutCapabilities call on
line 677. If the lookup fails or misses, skip the persistence operation entirely
to prevent data loss.
| resp, err := openRouterHTTPClient.Do(req) | ||
| if err != nil { | ||
| return "", fmt.Errorf("openrouter model page: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(io.LimitReader(resp.Body, 4*1024*1024)) | ||
| if err != nil { | ||
| return "", fmt.Errorf("openrouter model page: read: %w", err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file around the specified lines
cat -n internal/llm/capabilities.go | sed -n '155,175p'Repository: Dzarlax-AI/personal_assistant
Length of output: 1061
🏁 Script executed:
# Check for other similar patterns in the file to understand error handling patterns
rg -n "defer.*Close\(\)" internal/llm/capabilities.go -A 5 | head -40Repository: Dzarlax-AI/personal_assistant
Length of output: 467
🏁 Script executed:
# Search for proper error handling of resp.Body.Close() in the codebase
rg -n "closeErr\s*:=\s*resp\.Body\.Close\(\)" --type go -A 2Repository: Dzarlax-AI/personal_assistant
Length of output: 55
🏁 Script executed:
# Search for patterns where Close() errors are explicitly handled
rg "\.Close\(\)" --type go -A 2 | grep -E "(closeErr|errClose|err.*Close)" | head -20Repository: Dzarlax-AI/personal_assistant
Length of output: 55
🏁 Script executed:
# Look for other HTTP response body reading patterns in the codebase
rg "io\.ReadAll.*resp\.Body" --type go -B 2 -A 3 | head -60Repository: Dzarlax-AI/personal_assistant
Length of output: 3010
🏁 Script executed:
# Check if there's any linter configuration for errcheck in the repo
find . -name ".golangci.yml" -o -name ".golangci.yaml" -o -name "golangci.yml" | head -5Repository: Dzarlax-AI/personal_assistant
Length of output: 55
🏁 Script executed:
# Look for linting configuration
fd -e "golangci*" -o -e "*.lint*"Repository: Dzarlax-AI/personal_assistant
Length of output: 190
🏁 Script executed:
# Look for linting configuration files
find . -maxdepth 2 -type f \( -name "*.golangci*" -o -name ".lint*" \) 2>/dev/nullRepository: Dzarlax-AI/personal_assistant
Length of output: 55
🏁 Script executed:
# Check the exact proposed fix in the review - verify the error message issue
# Let me see if there are any other close error patterns in the repo
rg "resp\.Body" --type go -A 5 | grep -E "(Close|err)" | head -30Repository: Dzarlax-AI/personal_assistant
Length of output: 2158
🏁 Script executed:
# Check if there's any pattern of handling close errors elsewhere in Go files
rg "defer.*Close" --type go -B 2 -A 8 | grep -E "(defer|closeErr|Close|\.Close)" | head -40Repository: Dzarlax-AI/personal_assistant
Length of output: 2000
Handle resp.Body.Close() errors in the new fetch path.
Line 164 currently ignores close errors via defer, which can mask transport/resource issues. The proposed fix has a variable name mismatch—the error message still references err instead of readErr.
Proposed fix
- defer resp.Body.Close()
-
- body, err := io.ReadAll(io.LimitReader(resp.Body, 4*1024*1024))
- if err != nil {
- return "", fmt.Errorf("openrouter model page: read: %w", err)
+ body, readErr := io.ReadAll(io.LimitReader(resp.Body, 4*1024*1024))
+ closeErr := resp.Body.Close()
+ if readErr != nil {
+ return "", fmt.Errorf("openrouter model page: read: %w", readErr)
+ }
+ if closeErr != nil {
+ return "", fmt.Errorf("openrouter model page: close: %w", closeErr)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resp, err := openRouterHTTPClient.Do(req) | |
| if err != nil { | |
| return "", fmt.Errorf("openrouter model page: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(io.LimitReader(resp.Body, 4*1024*1024)) | |
| if err != nil { | |
| return "", fmt.Errorf("openrouter model page: read: %w", err) | |
| resp, err := openRouterHTTPClient.Do(req) | |
| if err != nil { | |
| return "", fmt.Errorf("openrouter model page: %w", err) | |
| } | |
| body, readErr := io.ReadAll(io.LimitReader(resp.Body, 4*1024*1024)) | |
| closeErr := resp.Body.Close() | |
| if readErr != nil { | |
| return "", fmt.Errorf("openrouter model page: read: %w", readErr) | |
| } | |
| if closeErr != nil { | |
| return "", fmt.Errorf("openrouter model page: close: %w", closeErr) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 164-164: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/llm/capabilities.go` around lines 160 - 168, Replace the simple
defer statement for resp.Body.Close() with explicit error handling that captures
and checks for close errors, rather than silently ignoring them. Additionally,
ensure the error variable used in the fmt.Errorf call for the io.ReadAll
operation matches the actual variable name being assigned (if you're using
readErr for the read operation error, update the error message to reference
readErr instead of err). This applies to the HTTP request handling block where
openRouterHTTPClient.Do(req) is called and resp.Body is read with
io.LimitReader.
Source: Linters/SAST tools
| if len(prefix) > 120 { | ||
| prefix = prefix[:120] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /dev/null || true
# Search for the file and examine the context
fd -t f capabilities.go internal/llmRepository: Dzarlax-AI/personal_assistant
Length of output: 170
🏁 Script executed:
# Read the relevant section around lines 191-193
cat -n internal/llm/capabilities.go | sed -n '170,210p'Repository: Dzarlax-AI/personal_assistant
Length of output: 1706
🏁 Script executed:
# Get more context to understand what prefix is and how it's used
cat -n internal/llm/capabilities.go | sed -n '145,220p'Repository: Dzarlax-AI/personal_assistant
Length of output: 3206
Avoid byte-slicing UTF-8 when capping the prefix.
At line 192, prefix = prefix[:120] slices at the byte level, not the rune level. For any multi-byte UTF-8 characters in the description, this can split a rune and cause the subsequent strings.HasPrefix comparison at line 208 to fail, preventing valid description enrichment.
Proposed fix
if len(prefix) > 120 {
- prefix = prefix[:120]
+ r := []rune(prefix)
+ prefix = string(r[:120])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(prefix) > 120 { | |
| prefix = prefix[:120] | |
| } | |
| if len(prefix) > 120 { | |
| r := []rune(prefix) | |
| prefix = string(r[:120]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/llm/capabilities.go` around lines 191 - 193, The prefix variable is
being sliced at the byte level with prefix[:120], which can split multi-byte
UTF-8 characters and cause subsequent strings.HasPrefix comparisons to fail.
Convert the prefix string to runes using []rune(), slice to 120 runes instead of
120 bytes, then convert back to a string using string() to ensure proper UTF-8
character boundaries are respected.
1 participant
Summary
Tests
Summary by CodeRabbit
Release Notes