Repository navigation
Fix/include default branch for session category switch - #25
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📝 WalkthroughWalkthroughThe PR enables exhaustive linting, replaces intermediate formatted strings with direct builder writes, and adds explicit unknown-category handling across session storage and expiration logic. ChangesLint configuration
Response formatting
Session category handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 2
🤖 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 `@handlers/common/session_store.go`:
- Around line 319-320: Validate sessionType before entering the capacity and
eviction logic in evictLRUSessions, reusing the existing category validation and
unknown-sessionCategory error behavior. Ensure invalid categories return without
deleting valid sessions, and add a regression test covering an invalid category
when the store is at capacity.
In `@Makefile`:
- Line 38: Before enabling exhaustive in the Makefile lint target, resolve or
explicitly exclude the known findings in handlers/common/errors.go and
client/onebusaway.go, then update the golangci-lint invocation in
.github/workflows/ci.yml to enforce the same exhaustive check as make lint.
🪄 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 Plus
Run ID: df53e1ef-5863-4f57-8121-955f03d62acb
📒 Files selected for processing (3)
Makefileformatters/response.gohandlers/common/session_store.go
| default: | ||
| return fmt.Errorf("unknown sessionCategory(%d)", int(sessionType)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate sessionType before evicting sessions.
When the store is at capacity, evictLRUSessions runs before this error branch. An invalid category can therefore return an error while still deleting valid sessions. Validate the category before the capacity/eviction logic and add a regression test for this failure path.
Proposed fix
func (s *ImprovedSessionStore) setSession(phoneNumber string, data interface{}, sessionType sessionCategory) error {
if !phoneRegex.MatchString(phoneNumber) {
return fmt.Errorf("invalid phone number format: %s", phoneNumber)
}
+ if sessionType != disambiguation && sessionType != voice && sessionType != sms {
+ return fmt.Errorf("unknown sessionCategory(%d)", int(sessionType))
+ }
s.mutex.Lock()📝 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.
| default: | |
| return fmt.Errorf("unknown sessionCategory(%d)", int(sessionType)) | |
| func (s *ImprovedSessionStore) setSession(phoneNumber string, data interface{}, sessionType sessionCategory) error { | |
| if !phoneRegex.MatchString(phoneNumber) { | |
| return fmt.Errorf("invalid phone number format: %s", phoneNumber) | |
| } | |
| if sessionType != disambiguation && sessionType != voice && sessionType != sms { | |
| return fmt.Errorf("unknown sessionCategory(%d)", int(sessionType)) | |
| } | |
| s.mutex.Lock() |
🤖 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 `@handlers/common/session_store.go` around lines 319 - 320, Validate
sessionType before entering the capacity and eviction logic in evictLRUSessions,
reusing the existing category validation and unknown-sessionCategory error
behavior. Ensure invalid categories return without deleting valid sessions, and
add a regression test covering an invalid category when the store is at
capacity.
| # Lint the code (requires golangci-lint) | ||
| lint: | ||
| golangci-lint run | ||
| golangci-lint run -E exhaustive |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not enable a failing lint gate without aligning CI and existing violations.
make lint now fails on the known non-exhaustive switches in handlers/common/errors.go and client/onebusaway.go. Fix or explicitly exclude those findings before enabling this flag; additionally, .github/workflows/ci.yml runs golangci-lint without -E exhaustive, so CI currently does not enforce the same check.
🤖 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 `@Makefile` at line 38, Before enabling exhaustive in the Makefile lint target,
resolve or explicitly exclude the known findings in handlers/common/errors.go
and client/onebusaway.go, then update the golangci-lint invocation in
.github/workflows/ci.yml to enforce the same exhaustive check as make lint.
Code reviewFound 1 issue:
Lines 36 to 39 in 33ced29 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Thanks for this — the Go changes are exactly what #19 asked for, and I like the choices you made. Returning an error from setSession rather than storing a half-initialized entry is right, the sessionCategory(%d) fallback in String() is more diagnosable than "unknown", and panicking in ExpireSession is fine by me: it's a test-only helper, and #19 explicitly sanctioned panicking there. The fmt.Fprintf conversions in formatters/response.go also clear three real staticcheck QF1012 warnings, which is a nice bonus.
One blocker, and it's the Makefile line.
golangci-lint run -E exhaustive leaves make lint failing. Running it against the tree:
client/onebusaway.go:152:2: missing cases in switch of type client.CircuitState: client.CircuitClosed (exhaustive)
handlers/common/errors.go:267:3: missing cases in switch of type models.ErrorCode: models.ErrorCodeInvalidStopID, models.ErrorCodeStopNotFound, models.ErrorCodeInvalidResponse, models.ErrorCodeValidationFailed, models.ErrorCodeInternalError (exhaustive)
You listed both in your PR notes, so this isn't news — but CLAUDE.md requires make lint to pass, so we can't land it red. Two details make this easy to miss: there's no .golangci.yml in the repo, so default-signifies-exhaustive stays false and the default: arms already present in those two switches don't silence the analyzer; and CI's lint job runs golangci-lint-action directly rather than make lint, so CI stays green while the required command is red.
Either fix works for me:
- Drop the
-E exhaustiveline and land thedefault:arms on their own. #19 offered the analyzer and the explicit defaults as alternatives, so this still fully closes the issue. - Keep the analyzer and fix the two switches — add
case CircuitClosed:to the circuit-breaker switch and the five missingmodels.ErrorCodecases toIsRetryableError. Both are a few lines. If you go this way, it's worth pointing CI atmake lintin the same PR so the two can't drift apart again.
I'd lean toward (1) here and (2) as a follow-up, but I'm happy with either. Ping me and I'll re-review as soon as it's updated.
|
@aaronbrethorst this is ready for re-review. I went with option 1 and removed the This keeps the defensive |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. Dropping -E exhaustive was the right call; make lint is clean again and the default: arms stand on their own, which fully closes #19. Approving and merging.
2 participants
Summary
Closes #19
sessionCategoryto provide clear fallback behavior and better debugging information and as general best practice.exhaustivecheck forgolangci-lintin the Makefile.sessionCategorymaps to"disambiguation".exhaustivelinter revealed unrelated non-exhaustive switches elsewhere (listed below).Reasoning for default case logic
setSession()Returning an error for unknown
sessionCategoryprevents silent behavior and forces the caller to handle unexpected input instead of continuing with storing an entry with no timestamps. The error formatting is consistent with that inString()to allow for easy debugging.estimateMemoryUsage()Decided to add a conservative fallback memory estimate (
baseSize += 150) for unknown types. Only a rough estimate and added for completeness sakeExpireSession()Decide to opt for a fail-fast approach and panic when an unknown sessionCategory type is supplied. Error formatting is also consistent with that in
String()for easier debugging.Other notes
The
fmt.Fprintfchanges informatters/response.goare small performance/clarity improvements (avoid temp string allocation).Testing
make fmt,make vet,make lint,make testNotes
make lint(with-E exhaustive) reported remaining non-exhaustive switches in other files. These are unrelated to thesessionCategorychanges, but surfaced after enabling the exhaustive check:handlers/common/errors.go:267:3: missing cases in switch of typemodels.ErrorCode:ErrorCodeInvalidStopID,ErrorCodeStopNotFound,ErrorCodeInvalidResponse,ErrorCodeValidationFailed,ErrorCodeInternalError(exhaustive)client/onebusaway.go:152:2: missing cases in switch of typeclient.CircuitState:CircuitClosed(exhaustive)Summary by CodeRabbit