Conversation
Replace type switch based error detection with shared sentinel errors and errors.Is. Each model error now implements Is() to map to the appropriate sentinel. Removes large IsNotFoundError and IsUniqueConstraintViolatedError switches.
This is a draft pr I will expand on later.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/errors.go (1)
48-66:⚠️ Potential issue | 🔴 CriticalCritical: Metric recording executes on every request, not just panics.
Lines 49-50 are placed outside the
if rvr := recover()block, causingRecordPostgresCode("25005")andRecordErrorCode(ErrorCodeUserSSOManaged)to execute on every request that passes through the recoverer middleware, not just when a panic occurs. This will corrupt metrics with massive false positives.Proposed fix - move metric calls inside the panic handler
fn := func(w http.ResponseWriter, r *http.Request) { defer func() { - apierrors.RecordPostgresCode(r.Context(), "25005") - apierrors.RecordErrorCode(r.Context(), apierrors.ErrorCodeUserSSOManaged) if rvr := recover(); rvr != nil { + apierrors.RecordPostgresCode(r.Context(), "25005") + apierrors.RecordErrorCode(r.Context(), apierrors.ErrorCodeUserSSOManaged) logEntry := observability.GetLogEntry(r)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/errors.go` around lines 48 - 66, The metrics RecordPostgresCode(r.Context(), "25005") and RecordErrorCode(r.Context(), apierrors.ErrorCodeUserSSOManaged) are executed unconditionally; move those two calls inside the panic branch so they run only when rvr != nil (i.e., within the if rvr := recover(); rvr != nil { ... } block). Update the defer in errors.go so that RecordPostgresCode and RecordErrorCode are invoked immediately after entering the panic handler (before logging and before creating the HTTPError/HandleResponseError), keeping the existing observability logging and HTTP error handling intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/apierrors/errorcode_test.go`:
- Line 64: Change the file permission used in the os.WriteFile call in
internal/api/apierrors/errorcode_test.go from 0644 to a more restrictive mode
(e.g., 0600) to satisfy Gosec G306; locate the os.WriteFile invocation that
writes "errorcode_gen.go" and replace the permission argument with 0600
(os.FileMode(0600)) so the generated file is created with restricted
permissions.
In `@internal/api/apierrors/metrics.go`:
- Around line 43-47: The initMetrics function validates the wrong map: it checks
the package-level errorCodesMap instead of the passed-in parameter ecm; update
the validation in initMetrics to use len(ecm) and ensure subsequent uses inside
initMetrics reference the parameter ecm (not the global errorCodesMap) so the
function actually validates and operates on the provided map.
---
Outside diff comments:
In `@internal/api/errors.go`:
- Around line 48-66: The metrics RecordPostgresCode(r.Context(), "25005") and
RecordErrorCode(r.Context(), apierrors.ErrorCodeUserSSOManaged) are executed
unconditionally; move those two calls inside the panic branch so they run only
when rvr != nil (i.e., within the if rvr := recover(); rvr != nil { ... }
block). Update the defer in errors.go so that RecordPostgresCode and
RecordErrorCode are invoked immediately after entering the panic handler (before
logging and before creating the HTTPError/HandleResponseError), keeping the
existing observability logging and HTTP error handling intact.
ℹ️ Review info
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
cmd/serve_cmd.gointernal/api/apierrors/errorcode.gointernal/api/apierrors/errorcode_gen.gointernal/api/apierrors/errorcode_test.gointernal/api/apierrors/metrics.gointernal/api/apierrors/metrics_test.gointernal/api/errors.gointernal/api/middleware.gointernal/models/errors.gointernal/models/oauth_authorization.gointernal/models/oauth_client.gointernal/models/one_time_token.gointernal/observability/metrics.go
| sb.WriteString("}\n\n") | ||
| } | ||
|
|
||
| os.WriteFile("errorcode_gen.go", []byte(sb.String()), 0644) |
There was a problem hiding this comment.
Fix file permissions to resolve pipeline failure.
Gosec G306: os.WriteFile uses permissions 0644; recommended 0600 or less.
Proposed fix
- os.WriteFile("errorcode_gen.go", []byte(sb.String()), 0644)
+ os.WriteFile("errorcode_gen.go", []byte(sb.String()), 0600)📝 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.
| os.WriteFile("errorcode_gen.go", []byte(sb.String()), 0644) | |
| os.WriteFile("errorcode_gen.go", []byte(sb.String()), 0600) |
🧰 Tools
🪛 GitHub Actions: Test
[error] 64-64: Gosec G306: os.WriteFile uses permissions 0644; recommended 0600 or less.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/apierrors/errorcode_test.go` at line 64, Change the file
permission used in the os.WriteFile call in
internal/api/apierrors/errorcode_test.go from 0644 to a more restrictive mode
(e.g., 0600) to satisfy Gosec G306; locate the os.WriteFile invocation that
writes "errorcode_gen.go" and replace the permission argument with 0600
(os.FileMode(0600)) so the generated file is created with restricted
permissions.
| func initMetrics(ecm map[string]string) error { | ||
| if len(errorCodesMap) == 0 { | ||
| const msg = "InitMetrics: errorCodesMap is empty" | ||
| return errors.New(msg) | ||
| } |
There was a problem hiding this comment.
Bug: Validation uses global errorCodesMap instead of parameter ecm.
The function accepts ecm as a parameter but line 44 validates the global errorCodesMap instead. This makes the parameter partially unused and the validation incorrect when ecm differs from the global.
Proposed fix
func initMetrics(ecm map[string]string) error {
- if len(errorCodesMap) == 0 {
+ if len(ecm) == 0 {
const msg = "InitMetrics: errorCodesMap is empty"
return errors.New(msg)
}📝 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.
| func initMetrics(ecm map[string]string) error { | |
| if len(errorCodesMap) == 0 { | |
| const msg = "InitMetrics: errorCodesMap is empty" | |
| return errors.New(msg) | |
| } | |
| func initMetrics(ecm map[string]string) error { | |
| if len(ecm) == 0 { | |
| const msg = "InitMetrics: errorCodesMap is empty" | |
| return errors.New(msg) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/apierrors/metrics.go` around lines 43 - 47, The initMetrics
function validates the wrong map: it checks the package-level errorCodesMap
instead of the passed-in parameter ecm; update the validation in initMetrics to
use len(ecm) and ensure subsequent uses inside initMetrics reference the
parameter ecm (not the global errorCodesMap) so the function actually validates
and operates on the provided map.
1 participant
This is a draft PR I've gone through to get a better understanding of our error handling and how to implement metrics for errors. I'll go over the changes in detail in a follow up pr.