17502_env_variables_in_primed_jupyter_sandbox#23
Open
sergei-bronnikov wants to merge 10 commits into
Open
Conversation
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThis PR implements secondary key functionality for BricksLLM, enabling alternative authentication identifiers while maintaining a primary key relationship. Secondary keys use a cache-backed lookup to resolve to primary key hashes, supporting authentication flows that present secondary credentials. The implementation spans data models, PostgreSQL/Redis storage, manager APIs, authentication request handling, admin endpoints, and system initialization. ChangesSecondary Key Management
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthenticateHttpRequest
participant getHashViaSecondary
participant KeyManager
participant SecondaryKeysCache
participant PostgresStore
Client->>AuthenticateHttpRequest: secondary_xyz key
AuthenticateHttpRequest->>getHashViaSecondary: secondary_xyz
getHashViaSecondary->>getHashViaSecondary: hash(secondary_xyz)
alt secondary_ prefix detected
getHashViaSecondary->>KeyManager: GetKeyHashBySecondary(hash)
KeyManager->>SecondaryKeysCache: Get(hash)
alt cache hit
SecondaryKeysCache-->>KeyManager: primary_hash
else cache miss
KeyManager->>PostgresStore: GetKeyHashBySecondary(hash)
PostgresStore-->>KeyManager: primary_hash
KeyManager->>SecondaryKeysCache: Set(hash, primary_hash, 24h)
end
KeyManager-->>getHashViaSecondary: primary_hash
end
getHashViaSecondary-->>AuthenticateHttpRequest: resolved_hash
AuthenticateHttpRequest->>AuthenticateHttpRequest: resolve key via primary hash
AuthenticateHttpRequest-->>Client: authenticated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (3)
internal/manager/key.go (1)
284-307: 💤 Low valueAvoid computing the hash twice.
hasher.Hash(keyUpdate.Key)is called on both line 298 and line 302 with identical input. Compute once and reuse.♻️ Proposed refactor
func (m *Manager) UpdateSecondaryKey(keyUpdate secondarykey.SecondaryKeyUpdate) error { if keyUpdate.Key == "" { return errors.New("key is required for updating secondary key") } if keyUpdate.LinkedKeyId == "" { return errors.New("linkedKeyId is required for updating secondary key") } rKey, err := m.s.GetKey(keyUpdate.LinkedKeyId) if err != nil { return err } if rKey == nil { return errors.New("linked key not found for updating secondary key") } - err = m.s.UpdateSecondaryKey(hasher.Hash(keyUpdate.Key), rKey.Key) + secondaryHash := hasher.Hash(keyUpdate.Key) + err = m.s.UpdateSecondaryKey(secondaryHash, rKey.Key) if err != nil { return err } - err = m.secondaryKC.Set(hasher.Hash(keyUpdate.Key), rKey.Key, 24*time.Hour) + err = m.secondaryKC.Set(secondaryHash, rKey.Key, 24*time.Hour) if err != nil { telemetry.Incr("bricksllm.manager.update_secondary_key.set_cache_error", nil, 1) } return 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/manager/key.go` around lines 284 - 307, In UpdateSecondaryKey, avoid hashing keyUpdate.Key twice: compute hashed := hasher.Hash(keyUpdate.Key) once, pass hashed to m.s.UpdateSecondaryKey and to m.secondaryKC.Set (instead of calling hasher.Hash twice), preserving existing error handling and TTL; update symbols to use the local variable (hashed) referenced in Manager.UpdateSecondaryKey, m.s.UpdateSecondaryKey, and m.secondaryKC.Set.internal/server/web/admin/admin.go (2)
624-643: ⚡ Quick winMissing validation error handling.
Other create handlers (e.g.,
getCreateKeyHandler,getCreateProviderSettingHandler) check forvalidationErrorand return HTTP 400 Bad Request. Without this check, validation failures will return 500 Internal Server Error.♻️ Proposed fix to add validation error handling
err = m.CreateSecondaryKey(*secondaryKeyCreate) if err != nil { errType := "internal" defer func() { telemetry.Incr("bricksllm.admin.get_create_secondary_key_handler.create_secondary_key_error", []string{ "error_type:" + errType, }, 1) }() + if _, ok := err.(validationError); ok { + errType = "validation" + c.JSON(http.StatusBadRequest, &ErrorResponse{ + Type: "/errors/validation", + Title: "secondary key validation failed", + Status: http.StatusBadRequest, + Detail: err.Error(), + Instance: path, + }) + return + } + logError(log, "error when creating a secondary key", prod, err)🤖 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/server/web/admin/admin.go` around lines 624 - 643, The error handling after calling m.CreateSecondaryKey(*secondaryKeyCreate) currently always returns 500; change it to detect a validationError (same pattern used in getCreateKeyHandler / getCreateProviderSettingHandler) and return HTTP 400 with a validation-related ErrorResponse when err is a validationError, otherwise keep the existing telemetry increment, logging via logError, and the 500 response; refer to the CreateSecondaryKey call and the telemetry key "bricksllm.admin.get_create_secondary_key_handler.create_secondary_key_error" to locate the block to modify.
699-718: ⚡ Quick winMissing validation and not-found error handling.
The
getUpdateKeyHandlerhandles bothvalidationError(400) andnotFoundError(404). Without these checks, all errors return 500 Internal Server Error.♻️ Proposed fix to add error type handling
err = m.UpdateSecondaryKey(*secondaryKeyUpdate) if err != nil { errType := "internal" defer func() { telemetry.Incr("bricksllm.admin.get_update_secondary_key_handler.update_secondary_key_error", []string{ "error_type:" + errType, }, 1) }() + if _, ok := err.(validationError); ok { + errType = "validation" + c.JSON(http.StatusBadRequest, &ErrorResponse{ + Type: "/errors/validation", + Title: "secondary key validation failed", + Status: http.StatusBadRequest, + Detail: err.Error(), + Instance: path, + }) + return + } + + if _, ok := err.(notFoundError); ok { + errType = "not_found" + c.JSON(http.StatusNotFound, &ErrorResponse{ + Type: "/errors/not-found", + Title: "secondary key not found", + Status: http.StatusNotFound, + Detail: err.Error(), + Instance: path, + }) + return + } + logError(log, "error when updating a secondary key", prod, err)🤖 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/server/web/admin/admin.go` around lines 699 - 718, The error handling in getUpdateKeyHandler currently treats all failures from m.UpdateSecondaryKey as internal (500); update the handler to detect validationError and notFoundError returned by UpdateSecondaryKey and map them to 400 and 404 respectively, setting errType to "validation" or "not_found" before the deferred telemetry incr, return the corresponding HTTP status and ErrorResponse (400 for validationError, 404 for notFoundError), and only fall back to "internal" and 500 for other errors; use the existing function/method names (getUpdateKeyHandler, UpdateSecondaryKey) and the current logError/telemetry.Incr/ErrorResponse flow to implement these branches.
🤖 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/authenticator/authenticator.go`:
- Around line 241-250: The call to getHashViaSecondary is returning an error
that is ignored and then shadowed by a later :=; update the logic in the
authenticator to check and handle the error from getHashViaSecondary immediately
(do not discard it), avoid shadowing err when calling a.kc.GetKeyViaCache (use =
not :=), and either return or log the error with context (including the function
names getHashViaSecondary and kc.GetKeyViaCache and variables hash, rKey, err)
so failures from the secondary lookup are surfaced instead of silently
proceeding with an invalid hash.
In `@internal/manager/key.go`:
- Around line 256-275: In GetKeyHashBySecondary, the
telemetry.Incr("...cache_hit") is called unconditionally causing misses to be
counted as hits; change the logic so
telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_hit", ...) is
only called in the cache-hit branch (i.e., where h != "" after
m.secondaryKC.Get(sHash)), keep the existing cache_miss increment where you
fetch from storage via m.s.GetKeyHashBySecondary(sHash), and ensure you do not
increment cache_hit after setting h = stored; reference m.secondaryKC.Get,
m.s.GetKeyHashBySecondary, h, sHash and the telemetry.Incr calls to locate and
move the hit increment.
In `@internal/server/web/admin/admin.go`:
- Line 688: Replace the typo in the log message passed to logError so it
correctly reads "error when unmarshalling secondary key update request body"
instead of "creation"; locate the call to logError(log, "error when
unmarshalling secondary key creation request body", prod, err) in admin.go (the
handler that processes secondary key updates) and update the message string to
use "update".
In `@internal/storage/postgresql/secondary_key.go`:
- Around line 49-56: UpdateSecondaryKey currently uses the read timeout s.rt and
silently returns nil when no row matches; switch the context timeout to the
write timeout (e.g. use s.writeTimeout or the store's write timeout field
instead of s.rt), call s.db.ExecContext to get the sql.Result, call
RowsAffected() on that result and if it returns 0 return a not-found error (for
example sql.ErrNoRows or a descriptive fmt.Errorf including secondaryHash),
otherwise return nil or the exec error; keep references to UpdateSecondaryKey,
s.rt, secondaryHash, keyHash and the secondary_keys update query to locate the
change.
- Around line 23-37: GetKeyHashBySecondary currently scans a nullable DB column
into a string which returns a Scan error for NULL and bypasses the app’s “not
found” handling; change GetKeyHashBySecondary to scan into sql.NullString (or
equivalent) and if the value is NULL or no rows return the canonical not-found
error the manager expects (map to internal_errors.NewNotFoundError in
internal/manager/key.go GetKeyHashBySecondary flow), and for UpdateSecondaryKey
use a context with the write timeout instead of s.rt and check RowsAffected()
after ExecContext to return an error when no row was updated so silent success
for non-existent secondary_hash is prevented.
---
Nitpick comments:
In `@internal/manager/key.go`:
- Around line 284-307: In UpdateSecondaryKey, avoid hashing keyUpdate.Key twice:
compute hashed := hasher.Hash(keyUpdate.Key) once, pass hashed to
m.s.UpdateSecondaryKey and to m.secondaryKC.Set (instead of calling hasher.Hash
twice), preserving existing error handling and TTL; update symbols to use the
local variable (hashed) referenced in Manager.UpdateSecondaryKey,
m.s.UpdateSecondaryKey, and m.secondaryKC.Set.
In `@internal/server/web/admin/admin.go`:
- Around line 624-643: The error handling after calling
m.CreateSecondaryKey(*secondaryKeyCreate) currently always returns 500; change
it to detect a validationError (same pattern used in getCreateKeyHandler /
getCreateProviderSettingHandler) and return HTTP 400 with a validation-related
ErrorResponse when err is a validationError, otherwise keep the existing
telemetry increment, logging via logError, and the 500 response; refer to the
CreateSecondaryKey call and the telemetry key
"bricksllm.admin.get_create_secondary_key_handler.create_secondary_key_error" to
locate the block to modify.
- Around line 699-718: The error handling in getUpdateKeyHandler currently
treats all failures from m.UpdateSecondaryKey as internal (500); update the
handler to detect validationError and notFoundError returned by
UpdateSecondaryKey and map them to 400 and 404 respectively, setting errType to
"validation" or "not_found" before the deferred telemetry incr, return the
corresponding HTTP status and ErrorResponse (400 for validationError, 404 for
notFoundError), and only fall back to "internal" and 500 for other errors; use
the existing function/method names (getUpdateKeyHandler, UpdateSecondaryKey) and
the current logError/telemetry.Incr/ErrorResponse flow to implement these
branches.
🪄 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: 3f0780d4-9804-4639-8bd7-8591437af8d4
📒 Files selected for processing (7)
cmd/bricksllm/main.gointernal/authenticator/authenticator.gointernal/manager/key.gointernal/secondary-key/secondary_key.gointernal/server/web/admin/admin.gointernal/storage/postgresql/secondary_key.gointernal/storage/redis/seconadry-key-cache.go
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
vvlevykin
approved these changes
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://bugtracker.codiodev.com/issue/codio-17502/There-are-no-LLM-based-env-variables-in-primed-jupyter-sandbox
Summary by CodeRabbit