17502_env_variables_in_primed_jupyter_sandbox#23
Open
sergei-bronnikov wants to merge 7 commits into
Open
Conversation
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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🎯 3 (Moderate) | ⏱️ ~25 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
Comment on lines
+241
to
250
| hash, err := a.getHashViaSecondary(raw) | ||
|
|
||
| key, err := a.kc.GetKeyViaCache(hash) | ||
| if key != nil { | ||
| rKey, err := a.kc.GetKeyViaCache(hash) | ||
| if rKey != nil { | ||
| telemetry.Incr(metricname.COUNTER_AUTHENTICATOR_FOUND_KEY_FROM_MEMDB, nil, 1) | ||
| } | ||
|
|
||
| if key == nil { | ||
| key, err = a.kc.GetKeyViaCache(raw) | ||
| if rKey == nil { | ||
| rKey, err = a.kc.GetKeyViaCache(raw) | ||
| } |
There was a problem hiding this comment.
Critical: Error from secondary key resolution is silently discarded.
Line 241 returns an error that is never checked. Line 243 redeclares err with :=, so the original error is lost. If getHashViaSecondary fails (e.g., secondary key doesn't exist or storage error), the code continues with whatever hash value was returned and may produce confusing authentication failures.
🐛 Proposed fix
hash, err := a.getHashViaSecondary(raw)
+ if err != nil {
+ var nFoundError notFoundError
+ if errors.As(err, &nFoundError) {
+ return nil, nil, internal_errors.NewAuthError(fmt.Sprintf("key %s is not found", anonymize(raw)))
+ }
+ return nil, nil, err
+ }
- rKey, err := a.kc.GetKeyViaCache(hash)
+ rKey, err = a.kc.GetKeyViaCache(hash)📝 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.
Suggested change
| hash, err := a.getHashViaSecondary(raw) | |
| key, err := a.kc.GetKeyViaCache(hash) | |
| if key != nil { | |
| rKey, err := a.kc.GetKeyViaCache(hash) | |
| if rKey != nil { | |
| telemetry.Incr(metricname.COUNTER_AUTHENTICATOR_FOUND_KEY_FROM_MEMDB, nil, 1) | |
| } | |
| if key == nil { | |
| key, err = a.kc.GetKeyViaCache(raw) | |
| if rKey == nil { | |
| rKey, err = a.kc.GetKeyViaCache(raw) | |
| } | |
| hash, err := a.getHashViaSecondary(raw) | |
| if err != nil { | |
| var nFoundError notFoundError | |
| if errors.As(err, &nFoundError) { | |
| return nil, nil, internal_errors.NewAuthError(fmt.Sprintf("key %s is not found", anonymize(raw))) | |
| } | |
| return nil, nil, err | |
| } | |
| rKey, err = a.kc.GetKeyViaCache(hash) | |
| if rKey != nil { | |
| telemetry.Incr(metricname.COUNTER_AUTHENTICATOR_FOUND_KEY_FROM_MEMDB, nil, 1) | |
| } | |
| if rKey == nil { | |
| rKey, err = a.kc.GetKeyViaCache(raw) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 241-241: ineffectual assignment to err
(ineffassign)
🤖 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/authenticator/authenticator.go` around lines 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.
Comment on lines
+256
to
+275
| func (m *Manager) GetKeyHashBySecondary(sHash string) (string, error) { | ||
| h, _ := m.secondaryKC.Get(sHash) | ||
| if h == "" { | ||
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_miss", nil, 1) | ||
| stored, err := m.s.GetKeyHashBySecondary(sHash) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if stored == "" { | ||
| return "", errors.New("key hash not found") | ||
| } | ||
| err = m.secondaryKC.Set(sHash, stored, 24*time.Hour) | ||
| if err != nil { | ||
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.set_error", nil, 1) | ||
| } | ||
| h = stored | ||
| } | ||
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_hit", nil, 1) | ||
| return h, nil | ||
| } |
There was a problem hiding this comment.
Telemetry bug: cache_hit is incremented even on cache misses.
Line 273 unconditionally increments the cache hit counter, but this executes after both the cache hit path (when h != "") and the cache miss path (after fetching from storage). This will cause every request to be counted as a hit, making the metrics unreliable.
🐛 Proposed fix
func (m *Manager) GetKeyHashBySecondary(sHash string) (string, error) {
h, _ := m.secondaryKC.Get(sHash)
if h == "" {
telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_miss", nil, 1)
stored, err := m.s.GetKeyHashBySecondary(sHash)
if err != nil {
return "", err
}
if stored == "" {
return "", errors.New("key hash not found")
}
err = m.secondaryKC.Set(sHash, stored, 24*time.Hour)
if err != nil {
telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.set_error", nil, 1)
}
- h = stored
+ return stored, nil
}
telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_hit", nil, 1)
return h, nil
}📝 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.
Suggested change
| func (m *Manager) GetKeyHashBySecondary(sHash string) (string, error) { | |
| h, _ := m.secondaryKC.Get(sHash) | |
| if h == "" { | |
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_miss", nil, 1) | |
| stored, err := m.s.GetKeyHashBySecondary(sHash) | |
| if err != nil { | |
| return "", err | |
| } | |
| if stored == "" { | |
| return "", errors.New("key hash not found") | |
| } | |
| err = m.secondaryKC.Set(sHash, stored, 24*time.Hour) | |
| if err != nil { | |
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.set_error", nil, 1) | |
| } | |
| h = stored | |
| } | |
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_hit", nil, 1) | |
| return h, nil | |
| } | |
| func (m *Manager) GetKeyHashBySecondary(sHash string) (string, error) { | |
| h, _ := m.secondaryKC.Get(sHash) | |
| if h == "" { | |
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_miss", nil, 1) | |
| stored, err := m.s.GetKeyHashBySecondary(sHash) | |
| if err != nil { | |
| return "", err | |
| } | |
| if stored == "" { | |
| return "", errors.New("key hash not found") | |
| } | |
| err = m.secondaryKC.Set(sHash, stored, 24*time.Hour) | |
| if err != nil { | |
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.set_error", nil, 1) | |
| } | |
| return stored, nil | |
| } | |
| telemetry.Incr("bricksllm.manager.get_key_hash_by_secondary.cache_hit", nil, 1) | |
| return h, 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 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.
| secondaryKeyUpdate := &secondarykey.SecondaryKeyUpdate{} | ||
| err = json.Unmarshal(data, secondaryKeyUpdate) | ||
| if err != nil { | ||
| logError(log, "error when unmarshalling secondary key creation request body", prod, err) |
There was a problem hiding this comment.
Typo in log message: "creation" should be "update".
🐛 Proposed fix
- logError(log, "error when unmarshalling secondary key creation request body", prod, err)
+ logError(log, "error when unmarshalling secondary key update request body", prod, err)📝 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.
Suggested change
| logError(log, "error when unmarshalling secondary key creation request body", prod, err) | |
| logError(log, "error when unmarshalling secondary key update request body", 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` at 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".
Comment on lines
+23
to
+37
| func (s *Store) GetKeyHashBySecondary(sHash string) (string, error) { | ||
| ctxTimeout, cancel := context.WithTimeout(context.Background(), s.rt) | ||
| defer cancel() | ||
| query := "SELECT key_hash FROM secondary_keys WHERE secondary_hash = $1" | ||
|
|
||
| row := s.db.QueryRowContext(ctxTimeout, query, sHash) | ||
|
|
||
| var keyHash string | ||
| err := row.Scan(&keyHash) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return keyHash, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nullable column + scan target mismatch in this file.
rg -n -C2 'key_hash VARCHAR|CreateSecondaryKey|INSERT INTO secondary_keys|GetKeyHashBySecondary|Scan\(&keyHash\)' internal/storage/postgresql/secondary_key.goRepository: codio/BricksLLM
Length of output: 791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/storage/postgresql/secondary_key.go"
echo "== GetKeyHashBySecondary (full function) =="
rg -n "func \\(s \\*Store\\) GetKeyHashBySecondary\\(" -n "$FILE"
sed -n '1,140p' "$FILE" | nl -ba | sed -n '20,120p'
echo "== UpdateSecondaryKey (function) =="
rg -n "func \\(s \\*Store\\) UpdateSecondaryKey\\(" "$FILE"
# Print around it if present
python3 - <<'PY'
import re, itertools, sys, pathlib
p=pathlib.Path("internal/storage/postgresql/secondary_key.go")
lines=p.read_text().splitlines()
for i,l in enumerate(lines):
if "func (s *Store) UpdateSecondaryKey" in l:
start=max(0,i-10); end=min(len(lines), i+60)
for j in range(start,end):
print(f"{j+1:4d}: {lines[j]}")
PY
echo "== GetKeyHashBySecondary call sites =="
rg -n "GetKeyHashBySecondary\\(" -S .
echo "== sql.ErrNoRows usage / expectations (near call sites) =="
rg -n "sql\\.ErrNoRows|ErrNoRows|no rows|not linked|secondary" -S . | head -n 80Repository: codio/BricksLLM
Length of output: 237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/storage/postgresql/secondary_key.go"
echo "== GetKeyHashBySecondary (full function) =="
rg -n "func \\(s \\*Store\\) GetKeyHashBySecondary\\(" "$FILE"
awk 'NR>=1 && NR<=140 {print NR ":" $0}' "$FILE" | sed -n '20,120p'
echo "== UpdateSecondaryKey (function) =="
rg -n "func \\(s \\*Store\\) UpdateSecondaryKey\\(" "$FILE" || true
if rg -n "func \\(s \\*Store\\) UpdateSecondaryKey\\(" "$FILE" >/dev/null; then
python3 - <<'PY'
import pathlib
p=pathlib.Path("internal/storage/postgresql/secondary_key.go")
lines=p.read_text().splitlines()
for i,l in enumerate(lines):
if "func (s *Store) UpdateSecondaryKey" in l:
start=max(0,i-10); end=min(len(lines), i+70)
for j in range(start,end):
print(f"{j+1:4d}: {lines[j]}")
PY
fi
echo "== GetKeyHashBySecondary call sites =="
rg -n "GetKeyHashBySecondary\\(" -S . || true
echo "== sql.ErrNoRows usage / expectations (near call sites) =="
rg -n "sql\\.ErrNoRows|ErrNoRows|no rows|not linked|secondary" -S . | head -n 80Repository: codio/BricksLLM
Length of output: 10913
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show manager GetKeyHashBySecondary implementation and error handling
FILE="internal/manager/key.go"
rg -n "func \\(m \\*Manager\\) GetKeyHashBySecondary\\(" "$FILE"
awk 'NR>=240 && NR<=310 {print NR ":" $0}' "$FILE"
# Show authenticator forwarding logic
FILE2="internal/authenticator/authenticator.go"
rg -n "GetKeyHashBySecondary\\(" "$FILE2"
awk 'NR>=180 && NR<=260 {print NR ":" $0}' "$FILE2"
# Inspect update semantics in manager (if any) for secondary keys
rg -n "func \\(m \\*Manager\\) UpdateSecondaryKey\\(" internal/manager/key.go
awk 'NR>=260 && NR<=340 {print NR ":" $0}' internal/manager/key.goRepository: codio/BricksLLM
Length of output: 6560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "key hash not found" -S . || true
rg -n "GetKeyHashBySecondary\\(" internal -S
rg -n "NotFound\\(" internal -S | head -n 80
rg -n "NewNotFoundError|NewAuthError|internal_errors.NewNotFoundError" internal -SRepository: codio/BricksLLM
Length of output: 4798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/authenticator/authenticator.go"
# print around AuthenticateHttpRequest block where getHashViaSecondary is called
python3 - <<'PY'
import pathlib
p=pathlib.Path("internal/authenticator/authenticator.go")
lines=p.read_text().splitlines()
start=210
end=290
for i in range(start-1, end):
if 0<=i<len(lines):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: codio/BricksLLM
Length of output: 2938
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect admin update handler for secondary keys
FILE="internal/server/web/admin/admin.go"
rg -n "getUpdateSecondaryKeyHandler|PATCH.*/secondary-keys|UpdateSecondaryKey\\(" "$FILE"
python3 - <<'PY'
import pathlib, re
p=pathlib.Path("internal/server/web/admin/admin.go")
lines=p.read_text().splitlines()
# print a chunk around the PATCH route and around UpdateSecondaryKey call in handler
for i,l in enumerate(lines):
if "PATCH(\"/api/key-management/secondary-keys" in l or "getUpdateSecondaryKeyHandler" in l:
start=max(0,i-40); end=min(len(lines), i+120)
for j in range(start,end):
print(f"{j+1:5d}: {lines[j]}")
break
PY
# Inspect internal/secondary-key package for semantics
# (list files)
find internal -maxdepth 3 -type f -path "*internal/secondary-key/*" -print
for f in $(find internal/secondary-key -type f -maxdepth 2 -name "*.go" -print); do
echo "==== $f ===="
wc -l "$f"
sed -n '1,200p' "$f"
doneRepository: codio/BricksLLM
Length of output: 9220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect admin update secondary key handler logic
FILE="internal/server/web/admin/admin.go"
python3 - <<'PY'
import pathlib
p=pathlib.Path("internal/server/web/admin/admin.go")
lines=p.read_text().splitlines()
for i,l in enumerate(lines):
if "func getUpdateSecondaryKeyHandler" in l:
start=max(0,i-20); end=min(len(lines), i+120)
for j in range(start,end):
print(f"{j+1:5d}: {lines[j]}")
break
PY
# 2) Inspect storage/postgresql/secondary_key.go (top portion with init table + any update rowcount handling)
FILE2="internal/storage/postgresql/secondary_key.go"
python3 - <<'PY'
import pathlib
p=pathlib.Path("internal/storage/postgresql/secondary_key.go")
lines=p.read_text().splitlines()
for i,l in enumerate(lines):
if "InitializeSecondaryKeyTable" in l:
start=max(0,i-20); end=min(len(lines), i+120)
for j in range(start,end):
print(f"{j+1:4d}: {lines[j]}")
break
PYRepository: codio/BricksLLM
Length of output: 7147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "GetKeyHashBySecondary\\(" -S internal cmd . || trueRepository: codio/BricksLLM
Length of output: 1185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect NotFoundError type
sed -n '1,120p' internal/errors/not_found_err.go
# Inspect pattern in other storage Get* functions (e.g., key.go) to see how sql.ErrNoRows is mapped
rg -n "errors\\.Is\\(err, sql\\.ErrNoRows\\)|err == sql\\.ErrNoRows" internal/storage/postgresql/key.go internal/storage/postgresql/route.go internal/storage/postgresql/custom_provider.go internal/storage/postgresql/user.go | head -n 60Repository: codio/BricksLLM
Length of output: 1093
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "AuthenticateHttpRequest\\(" -S . || true
rg -n "getHashViaSecondary\\(" -S . || true
rg -n "internal_errors\\.NewAuthError\\(" -S internal/server internal/middleware internal -g'*.go' | head -n 50Repository: codio/BricksLLM
Length of output: 1843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/server/web/proxy/middleware.go"
# Print around AuthenticateHttpRequest call + error handling
python3 - <<'PY'
import pathlib
p=pathlib.Path("internal/server/web/proxy/middleware.go")
lines=p.read_text().splitlines()
# locate call site
for i,l in enumerate(lines):
if "AuthenticateHttpRequest(c.Request" in l:
start=max(0,i-40); end=min(len(lines), i+80)
for j in range(start,end):
print(f"{j+1:5d}: {lines[j]}")
break
PY
# Also search for status code mapping based on NotFound/Auth errors
rg -n "Status|StatusUnauthorized|Unauthorized|StatusNotFound|NotFound|NewAuthError|NotFound\\(" "$FILE"Repository: codio/BricksLLM
Length of output: 6662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/server/web/proxy/middleware.go"
python3 - <<'PY'
import pathlib
p=pathlib.Path("internal/server/web/proxy/middleware.go")
lines=p.read_text().splitlines()
for i,l in enumerate(lines[:160], start=1):
if "type notAuthorizedError" in l or "type notFoundError" in l:
start=max(1,i-5); end=min(len(lines), i+40)
for j in range(start,end+1):
print(f"{j:5d}: {lines[j-1]}")
break
PYRepository: codio/BricksLLM
Length of output: 1065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' internal/errors/authentication_err.goRepository: codio/BricksLLM
Length of output: 303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,190p' internal/storage/postgresql/key.goRepository: codio/BricksLLM
Length of output: 1322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' internal/storage/postgresql/secondary_key.goRepository: codio/BricksLLM
Length of output: 1517
Handle nullable key_hash and “not found” flow in secondary lookup.
secondary_keys.key_hashis nullable, butinternal/storage/postgresql/secondary_key.go(GetKeyHashBySecondary, lines 23-37) scans it intostring, soNULLreturns aScanerror that bypasses the app’s “not found” handling and ends up as an internal auth error (500).- Ensure the “secondary lookup missing/NULL” case is mapped to
internal_errors.NewNotFoundErrorininternal/manager/key.go(GetKeyHashBySecondary) soAuthenticator.AuthenticateHttpRequestconverts it to an unauthorized response. internal/storage/postgresql/secondary_key.go(UpdateSecondaryKey, lines 49-57) uses the read timeout (s.rt) and ignoresRowsAffected(), so updating a non-existentsecondary_hashcurrently succeeds silently.
🤖 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/storage/postgresql/secondary_key.go` around lines 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.
Comment on lines
+49
to
+56
| func (s *Store) UpdateSecondaryKey(secondaryHash, keyHash string) error { | ||
| ctxTimeout, cancel := context.WithTimeout(context.Background(), s.rt) | ||
| defer cancel() | ||
|
|
||
| query := "UPDATE secondary_keys SET key_hash = $1 WHERE secondary_hash = $2" | ||
|
|
||
| _, err := s.db.ExecContext(ctxTimeout, query, keyHash, secondaryHash) | ||
| return err |
There was a problem hiding this comment.
Use write timeout and detect no-op updates in UpdateSecondaryKey.
This is a write operation but it uses the read timeout, and it returns nil even when no row matches secondary_hash.
💡 Proposed fix
+import "database/sql"
@@
func (s *Store) UpdateSecondaryKey(secondaryHash, keyHash string) error {
- ctxTimeout, cancel := context.WithTimeout(context.Background(), s.rt)
+ ctxTimeout, cancel := context.WithTimeout(context.Background(), s.wt)
defer cancel()
@@
- _, err := s.db.ExecContext(ctxTimeout, query, keyHash, secondaryHash)
- return err
+ result, err := s.db.ExecContext(ctxTimeout, query, keyHash, secondaryHash)
+ if err != nil {
+ return err
+ }
+ affected, err := result.RowsAffected()
+ if err != nil {
+ return err
+ }
+ if affected == 0 {
+ return sql.ErrNoRows
+ }
+ 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/storage/postgresql/secondary_key.go` around lines 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.
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.
https://bugtracker.codiodev.com/issue/codio-17502/There-are-no-LLM-based-env-variables-in-primed-jupyter-sandbox
Summary by CodeRabbit
Release Notes