fix(errors): improve error handling ergonomics and docs#63
Conversation
Implement missing GetTags/CollectTags APIs, fix ErrCode.As and registry panics, remove WrapStack stderr side effects, and align errparser with structured errors. Co-authored-by: Cursor <cursoragent@cursor.com>
Add FormatChain, CollectUserTags, and errcode lookup APIs; align JSON serialization and wire error chains into log and result helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the errors package by introducing structured error chain formatting (FormatChain), metadata tag collection (CollectTags, CollectUserTags), and a new errcode sub-package for typed business error codes with protobuf/gRPC compatibility. It also updates logging and result integrations to leverage these new features. The review feedback highlights several critical areas for improvement: adding synchronization to prevent concurrent map access panics in the error code and parser registries, avoiding redundant message duplication in FormatChain when the wrap message matches the underlying error, ensuring GetTags supports both pointer and value types of Err, and adding a nil-pointer guard in ErrCode.As to prevent panics on zero-initialized error codes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func RegisterErrCode(code *errorpb.ErrCode) error { | ||
| if code == nil { | ||
| return fmt.Errorf("errcode: code is nil") | ||
| } | ||
| if code.Name == "" { | ||
| return fmt.Errorf("errcode: code name is empty") | ||
| } | ||
| if errorCodes[code.Name] != nil { | ||
| panic(fmt.Sprintf("code exists, code=%s", errorCodes[code.Name])) | ||
| return fmt.Errorf("errcode: already registered: name=%q", code.Name) | ||
| } | ||
|
|
||
| errorCodes[code.Name] = proto.Clone(code).(*errorpb.ErrCode) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The global map errorCodes is accessed concurrently by RegisterErrCode, LookupErrCode, and GetErrCodes without any synchronization. In Go, concurrent map writes and reads/writes will cause a runtime panic. Please introduce a sync.RWMutex to protect access to errorCodes.
| func RegisterParser(errParser ErrParser) bool { | ||
| key := stack.CallerWithFunc(errParser).String() | ||
| if errParserRegistry[key] != nil { | ||
| slog.Error("errParser already exists", "parser", errParserRegistry[key]) | ||
| if errParser == nil { | ||
| slog.Error("errParser is nil") | ||
| return false | ||
| } | ||
|
|
||
| if errParser == nil { | ||
| slog.Error("errParser is nil") | ||
| key := stack.CallerWithFunc(errParser).String() | ||
| if errParserRegistry[key] != nil { | ||
| slog.Error("errParser already exists", "parser", errParserRegistry[key]) | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
The global map errParserRegistry is accessed concurrently by RegisterParser and parseError without any synchronization. This can lead to concurrent map read/write panics at runtime. Please use a sync.RWMutex or sync.Map to ensure thread safety.
| case *ErrWrap: | ||
| if msg, ok := e.Tags["msg"]; ok { | ||
| return strings.TrimSpace(fmt.Sprint(msg)) | ||
| } |
There was a problem hiding this comment.
When WrapCaller or WrapStack wraps an error, they automatically set the "msg" tag to err.Error(). In FormatChain, this causes the root error message to be duplicated in the formatted chain (e.g., "init failed: db failed: db failed"). By checking if the "msg" tag is identical to the underlying error's message, we can filter out these automatic/redundant messages and keep the formatted chain clean.
| case *ErrWrap: | |
| if msg, ok := e.Tags["msg"]; ok { | |
| return strings.TrimSpace(fmt.Sprint(msg)) | |
| } | |
| case *ErrWrap: | |
| if msg, ok := e.Tags["msg"]; ok { | |
| msgStr := strings.TrimSpace(fmt.Sprint(msg)) | |
| if msgStr != "" && msgStr != strings.TrimSpace(e.Err.Error()) { | |
| return msgStr | |
| } | |
| } | |
| return "" |
| if e, ok := current.(*Err); ok && len(e.Tags) > 0 { | ||
| inner = e.Tags | ||
| } |
There was a problem hiding this comment.
The GetTags function only checks for *Err (pointer type) when traversing the error chain. However, Err can also be used as a value type (as seen in errLayerData switch cases). To ensure robustness, please check for both *Err and Err types.
if e, ok := current.(*Err); ok && len(e.Tags) > 0 {
inner = e.Tags
} else if e, ok := current.(Err); ok && len(e.Tags) > 0 {
inner = e.Tags
}| func (t *ErrCode) As(target any) bool { | ||
| if target == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
If ErrCode is zero-initialized (i.e., t.pb is nil), calling As can cause a panic during proto.Clone(t.pb).(*errorpb.ErrCode) due to type asserting a nil interface. Consider adding a guard check if t.pb == nil { return false } at the beginning of the As method.
| func (t *ErrCode) As(target any) bool { | |
| if target == nil { | |
| return false | |
| } | |
| func (t *ErrCode) As(target any) bool { | |
| if target == nil || t.pb == nil { | |
| return false | |
| } |
1 participant
Summary
log.Errandresult.ErrorTest plan
go test ./errors/...go test ./log/... ./result/...Made with Cursor