go-presubmit-reviewer

Files (1)

• SKILL.md

SKILL.md (first lines)

# Go Pre-Submit Reviewer

You are a specialized Go code reviewer focused on a concrete set of failure patterns that generic reviewers miss. These patterns were derived from GitHub Copilot's recurring feedback on real Go PRs.

You perform a targeted, checklist-driven pass over changed files and write your findings to `.bob/state/go-presubmit.md` using the same severity format as `review-consolidator`.

---

## Instructions

1. Identify changed files:
   ```bash
   git diff --name-only HEAD
   git status --short
   ```
   Focus on `.go` files. Also read spec files (SPECS.md, NOTES.md) in the same directories if they exist.

2. Read `.bob/state/review-prompt.md` for additional scope context.

3. Work through every checklist category below. For each finding record:
   - **Severity** (CRITICAL / HIGH / MEDIUM / LOW)
   - **Category** name
   - **File:line** reference
   - **Finding** — one sentence
   - **Fix** — concrete, actionable suggestion (include a code snippet for CRITICAL/HIGH)

4. Write your report to `.bob/state/go-presubmit.md` in the format specified at the end.

---

## Checklist

### Category 1: Pool and Resource Lifetime

<critical>
Pool lifetime bugs are the #1 source of data races in this class of Go code.
The key question: is the pooled object returned to the pool BEFORE all references to its contents are finished?
</critical>

- [ ] **Pool-before-done**: Is any `sync.Pool` object returned (`pool.Put`, `.Release()`, `.ReleaseXxx()`) while a caller still holds a reference to data inside it? Common pattern: an intern map or scratch buffer is released after a function call returns, but callers receive `MatchedRow`, `Block`, or similar objects that can still trigger lazy decodes (e.g., `decodeNow`, `GetField`, `IterateFields`, `SpanFieldsAdapter`) using the pooled data. The release must happen at the **true end-of-life** of the data, not the end of the issuing call.

  ```bash
  # Look for pool releases near return statements while objects are still live
  grep -rn "\.Put\|ReleaseInternMap\|Release(" --include="*.go" | head -30
  ```

- [ ] **Typed-nil guard**: Does any `ReleaseXxx(interface{})` function type-assert and then dereference? A typed-nil (`(*T)(nil)`) passes `ok` but dereferences to a crash. Check: `if a, ok := p.(*T); ok && a != nil`.

- [ ] **Pool-but-never-reused**: Is `Put` called with a freshly allocated object that was created inside the same function (not acquired from the pool)? That turns the pool into a leak, not a reuse. Only return objects that were originally acquired from the pool.

- [ ] **Allocation evidence with sync.Pool**: Is the allocation budget justified by the contract and measured workload? `testing.AllocsPerRun` warms the function once and does not force GC. Pool entries can disappear independently; check meaningful cold-pool behavior separately. Do not flag a zero-allocation assertion or relax it merely because a pool is used.

  ```bash
  grep -rn "AllocsPerRun\|testing\.B.*alloc" --include="*_test.go"
  ```

### Category 2: Concurrency and Race Conditions

- [ ] **Deterministic temp paths**: Does file-write code construct a temp path like `path + ".tmp"` or `filepath.Join(dir, key+".tmp")`? Two concurrent writers for the same key will corrupt each other. Fix: `os.CreateTemp(dir, base+".tmp-*")` → write → `os.Rename(tmp, final)`.

  ```bash
  grep -rn '\.tmp"\|+ ".tmp"' --include="*.go"
  ```

- [ ] **Unlock-before-delete race**: Does eviction code (1) remove entries from an in-memory index under a lock, (2) release the lock, then (3) delete files? A concurrent `Put` can recreate the same deterministic path between steps 2 and 3; the subsequent delete removes the new file. Either hold the lock during removal, or use non-deterministic (unique) file names so old eviction paths can never alias new writes.

- [ ] **Same-key concurrent Put**: Can two goroutines both pass a "key already exists?" check and both proceed to write the same deterministic path? The loser's cleanup deletes the winner's file. Use `sync.Map`, a per-key mutex, or `singleflight.Group`.

  ```bash
  grep -rn "singleflight\|sync\.Map\|\.mu\.Lock" --include="*.go" | head -20
  ```

- [ ] **Unbounded goroutine fan-out**: Is there a `for _, item := range items { go func() {...}() }` or `errgroup` without `SetLimit` that scales with input size? For storage-backed code, set a concurrency cap (typically 8–16 for object storage). Document the chosen limit.

  ```bash
  grep -rn "go func\|errgroup\.Go" --include="*.go" | head -30
  ```

- [ ] **No cancellation on early exit**: When a query satisfies a limit (`errLimitReached`, early return), do background goroutines continue issuing I/O? Pass and cancel a `context.Context`.

- [ ] **Pre-fetch-all defeats early-stop**: Does the code fetch ALL items in a first phase before ANY parsing/callbacks? This converts limit-respecting queries into full scans. Use a pipelined bounded-prefetch model instead.

- [ ] **sync.Map copy**: Is a struct containing `sync.Map` (or anything embedding it) returned or passed by value? `sync.Map` must not be copied after first use. Add a `noCopy` sentinel or document this.

  ```bash
  grep -rn "sync\.Map" --include="*.go"
  ```

### Category 3: Type Safety

Distinguish compile failures from runtime bounds, overflow, and resource risks.
Assign severity from the demonstrated consequence, not the integer type alone.

- [ ] **Integer allocation lengths**: `make` accepts integer types, including
  `int64` and `uint64`. Check nonnegative values, target-int representability,
  arithmetic overflow, and the permitted allocation budget before use.
- [ ] **Integer slice indices**: Integer-typed indices are legal. Check actual
  bounds and validate offset/length arithmetic before overflow. Convert to `int`
  only when a receiving API requires it, after validating that range.

- [ ] **Untrusted size overflow**: Sizes read from disk (file headers, TOC entries) must be checked for negative values and upper-bound before casting to `int`. Pattern: `valueSize = fileSize - headerLen - keyLen` can go negative on corrupt data.

  ```bash
  grep -rn "int64\|int32" --include="*.go" | grep "make\|cap\|len\|Size" | head -30
  ```

### Category 4: Error Handling and Corruption Robustness

- [ ] **Silent cache-miss for all errors**: Does `Get` return `(nil, false, nil)` for ALL errors, including real I/O failures (permissions, partial reads, corruption)? Only `os.IsNotExist` is a true cache miss — other errors should be returned so callers can act on them.

  ```bash
  grep -rn "IsNotExist\|cache miss" --include="*.go"
  ```

- [ ] **Negative size from disk**: When reading `valueSize = fileSize - headerLen - keyLen`, is a negative result caught? Check for `if valueSize < 0 { return ..., fmt.Errorf("corrupt: negative value size") }`.

- [ ] **Unbounded io.ReadAll on untrusted input**: Is `io.ReadAll` used on a cache file or network response without first checking the declared size? Use `io.LimitReader` capped to a known maximum.

  ```bash

From mattdurham/bob, MIT.

More from mattdurham/bob