-
Notifications
You must be signed in to change notification settings - Fork 7
.pr_agent_auto_best_practices
Pattern 1: Guard all reads of optional/uninitialized values and results of fallible operations (atomic loads, file info calls, async job waits) with explicit nil/error checks before type assertions or dereferences, and ensure resources are closed exactly once on all exit paths.
Example code before:
v := a.Load().(string) // may panic if unset
f, _ := entry.Info() // ignores error; f can be nil
job, _ := startJob(ctx)
status, _ := job.Wait(ctx) // ignores error; status may be invalid
defer client.Close() // may run when client is nil if init failed
Example code after:
if v := a.Load(); v != nil {
_ = v.(string)
}
f, err := entry.Info()
if err != nil {
return err
}
status, err := job.Wait(ctx)
if err != nil {
return err
}
client, err := newClient()
if err != nil {
return err
}
defer client.Close()
Relevant past accepted suggestions:
Suggestion 1:
Add nil check
The Checksum() method doesn't handle the case when the atomic value hasn't been initialized yet, which could lead to a panic with "interface conversion: interface {} is nil, not string". Add a nil check before attempting to convert the loaded value.
func (s *Safe) Checksum() string {
- return s.checksum.Load().(string)
+ val := s.checksum.Load()
+ if val == nil {
+ return ""
+ }
+ return val.(string)
}- Apply this suggestion
Suggestion 2:
Add nil checks
All the getter methods for atomic values (Path(), Created(), IsDir()) are missing nil checks, which could cause panics if the values haven't been initialized. Add nil checks to all these methods similar to the fix for Checksum().
func (s *Safe) Path() string {
- return s.path.Load().(string)
+ val := s.path.Load()
+ if val == nil {
+ return ""
+ }
+ return val.(string)
}
// SetCreated will set the Created field in the
// format time.RFC3339 in UTC.
func (s *Safe) SetCreated(t time.Time) {
s.created.Store(t.In(time.UTC).Format(time.RFC3339))
}
func (s *Safe) Created() string {
- return s.created.Load().(string)
+ val := s.created.Load()
+ if val == nil {
+ return ""
+ }
+ return val.(string)
}
func (s *Safe) SetDir(isDir bool) {
s.isDir.Store(isDir)
}
func (s *Safe) IsDir() bool {
- return s.isDir.Load().(bool)
+ val := s.isDir.Load()
+ if val == nil {
+ return false
+ }
+ return val.(bool)
}- Apply this suggestion
Suggestion 3:
Use safe getter methods
The Stats() method directly accesses atomic values without nil checks, which could cause panics. Use the safe getter methods you've defined (Checksum(), Path(), etc.) instead of direct access to the atomic values.
func (s *Safe) Stats() Stats {
return Stats{
LineCnt: atomic.LoadInt64(&s.LineCnt),
ByteCnt: atomic.LoadInt64(&s.ByteCnt),
Size: atomic.LoadInt64(&s.Size),
Files: atomic.LoadInt64(&s.Files),
- Checksum: s.checksum.Load().(string),
- Path: s.path.Load().(string),
- Created: s.created.Load().(string),
- IsDir: s.isDir.Load().(bool),
+ Checksum: s.Checksum(),
+ Path: s.Path(),
+ Created: s.Created(),
+ IsDir: s.IsDir(),
}
}- Apply this suggestion
Suggestion 4:
Fix EOF handling bug
The commented-out Lines() method has a critical bug in its EOF handling. When err == io.EOF, the code attempts to yield the last line, but at this point ln might be empty or invalid. Additionally, the reader should be properly closed when the iterator is done or interrupted.
/*
func (r *Reader) Lines() iter.Seq[[]byte] {
return func(yield func([]byte) bool) {
var ln []byte
var err error
for ln, err = r.ReadLine(); err == nil; ln, err = r.ReadLine() {
if !yield(ln) {
- // todo: Should we close reader?
-
+ r.Close()
return
}
}
- if err == io.EOF {
+ if err == io.EOF && len(ln) > 0 {
yield(ln)
}
+ r.Close()
}
}
*/- Apply this suggestion
Suggestion 5:
Handle file info errors
The code ignores potential errors from d.Info() which could lead to accessing nil fInfo and cause a panic. Always check for errors when calling methods that can fail, especially when working with file operations.
for _, d := range dirInfo {
- fInfo, _ := d.Info()
+ fInfo, err := d.Info()
+ if err != nil {
+ continue // Skip files that can't be accessed
+ }
sts := stat.Stats{
Created: fInfo.ModTime().Format(time.RFC3339),
Path: path.Join(pth, fInfo.Name()),
Size: fInfo.Size(),
IsDir: fInfo.IsDir(),
}- Apply this suggestion
Suggestion 6:
Fix potential nil pointer dereference by properly handling job wait errors
The error handling in the Load method ignores the error from job.Wait() and continues with status checks, which could lead to nil pointer dereferences.
apps/workers/bigquery/worker.go [220-228]
-if err == nil {
- if status.Err() != nil {
- return task.Failf("job completed with error: %v", status.Errors)
- }
- if sts, ok := status.Statistics.Details.(*bigquery.LoadStatistics); ok {
+if err != nil {
+ return task.Failf("job wait error: %v", err)
+}
+if status.Err() != nil {
+ return task.Failf("job completed with error: %v", status.Errors)
+}
+if sts, ok := status.Statistics.Details.(*bigquery.LoadStatistics); ok {- Apply this suggestion
Suggestion 7:
Prevent potential resource leaks by properly ordering error checks and defer statements
The DoTask method doesn't handle the error from client.Close() which could lead to resource leaks. Move the defer statement after the error check.
apps/workers/bigquery/worker.go [82-86]
client, err := bigquery.NewClient(ctx, w.Project, opts...)
-defer client.Close()
if err != nil {
return task.Failf("bigquery client init %s", err)
}
+defer client.Close()- Apply this suggestion
Pattern 2: Prefer existing shared helpers and correct domain primitives (e.g., Stats.Add(), correct per-second rates, correct identifier fields) instead of re-implementing logic or using the wrong units/fields that can silently skew results.
Example code before:
// duplicate stats math
if t.Result == "error" { stat.ErrorCount++ }
// wrong units: derivative already returns per-second
rate := derivative()/poll.Seconds()
// wrong identifier field used in reference
ref := dataset.Table(cfg.Project)
Example code after:
// reuse the canonical helper
stat.Add(task)
// keep correct units
rate := derivative()
// use the correct identifier field
ref := dataset.Table(cfg.Table)
Relevant past accepted suggestions:
Suggestion 1:
Avoid duplicating business logic
Refactor generateSummaryFromTasks to use the existing stat.Add() method for calculating statistics, eliminating duplicated logic and improving maintainability.
apps/flowlord/handler.go [520-582]
// generateSummaryFromTasks creates a summary of tasks grouped by type:job
func generateSummaryFromTasks(tasks []cache.TaskView) map[string]*cache.Stats {
summary := make(map[string]*cache.Stats)
for _, t := range tasks {
-...
+ // Get job from TaskView.Job or extract from Meta
+ job := t.Job
+ if job == "" {
+ if meta, err := url.ParseQuery(t.Meta); err == nil {
+ job = meta.Get("job")
+ }
+ }
+
+ // Create key in format "type:job"
+ key := strings.TrimRight(t.Type+":"+job, ":")
+
+ // Get or create stats for this type:job combination
+ stat, found := summary[key]
+ if !found {
+ stat = &cache.Stats{
+ CompletedTimes: make([]time.Time, 0),
+ ErrorTimes: make([]time.Time, 0),
+ ExecTimes: &cache.DurationStats{},
+ }
+ summary[key] = stat
+ }
+
// Convert TaskView to task.Task for processing
- taskTime := tmpl.TaskTime(task.Task{
+ tsk := task.Task{
ID: t.ID,
Type: t.Type,
Job: t.Job,
Info: t.Info,
Result: task.Result(t.Result),
Meta: t.Meta,
Msg: t.Msg,
Created: t.Created,
Started: t.Started,
Ended: t.Ended,
- })
-
- // Process based on result type
- if t.Result == "error" {
- stat.ErrorCount++
- stat.ErrorTimes = append(stat.ErrorTimes, taskTime)
- } else if t.Result == "complete" {
-...
}
- // Note: warn and alert results don't contribute to execution time stats
+ // Use the existing Add method to avoid duplicating logic
+ stat.Add(tsk)
}
return summary
}Suggestion 2:
Fix double-scaling of rate
The derivative you compute already returns units per second; dividing again by the poll period will understate the rate and break threshold comparisons. Remove the extra division to preserve correct rate calculation. Update any thresholds to continue expecting per-second rates.
apps/utils/nsq-monitor/monitor.go [269]
-channel.Rate = channel.Derivative() / app.PollPeriod.Seconds()
+channel.Rate = channel.Derivative()Suggestion 3:
Fix incorrect table name reference in BigQuery table creation method
The BqTable method incorrectly uses the Project field instead of the Table field when creating the table reference, which will cause operations to fail.
apps/workers/bigquery/main.go [59-61]
func (d Destination) BqTable(client *bigquery.Client) *bigquery.Table {
- return client.DatasetInProject(d.Project, d.Dataset).Table(d.Project)
+ return client.DatasetInProject(d.Project, d.Dataset).Table(d.Table)
}- Apply this suggestion
[Auto-generated best practices - 2026-04-09]