Charm Go Find Bugs

Charm Go Bug Finder

Find bugs in Canonical Charm Tech Go codebases using domain-specific knowledge from 143+ historical bug fixes across pebble and concierge.

Scope

Report on: only the files or diff the user specifies. Research: the entire codebase to build confidence before reporting.

Do not report issues based solely on pattern matching. Investigate data flow and confirm the bug is real before reporting.

Step 1: Determine review scope

Identify what code areas are being reviewed and load the relevant reference sections:

Always read references/anti-patterns.md for concrete code patterns to search for.

Step 2: Search for known bug patterns

For each relevant code area, search the codebase for the anti-patterns listed in references/anti-patterns.md. Use Grep with the provided search patterns.

For each match:

1. Read surrounding context (at least 30 lines around the match).
2. Trace data flow — especially lock acquisition order and error propagation.
3. Check if there is an existing fix, test, or guard.
4. Only report if the bug is confirmed after investigation.

Step 3: Hunt for novel bugs

After searching for known patterns:

1. Read each file in scope end-to-end — concurrency bugs hide between patterns.
2. Trace lock acquisition order: for each mutex in the codebase, list every function that acquires it and what other locks are held at that point. Look for inconsistent ordering.
3. Trace error propagation: pick 3-5 error returns and follow them up the call stack. Check for lost errors, wrong wrapping, and incorrect retry behavior.
4. Check constructor usage: for each exported type with a New* constructor, search for &Type{} zero-value construction that bypasses it.
5. Compare parallel implementations: for provider code (K8s, LXD, MicroK8s), diff analogous methods side by side. Copy-paste errors are common.
6. Check recent commits (git log --oneline -20 -- <file>).

Step 4: Check cross-cutting concerns

Always check these regardless of code area:

1. Lock ordering: Are multiple locks acquired in a consistent order across all goroutines?
2. Nil map guards: Are map struct fields initialized before assignment?
3. Error wrapping: Is %w used (not %s or %v) when wrapping errors for later errors.Is/errors.As?
4. Deferred cleanup: Do deferred Close()/Stop() calls check errors? Do they shadow named returns?
5. Idempotency: Do provisioning/bootstrap operations check existing state before acting?
6. Retry discrimination: Does retry logic distinguish permanent from transient errors?
7. os.Chown vs os.Lchown: Are file ownership operations safe against symlink traversal?

Step 5: Verify each finding

For each potential bug:

• Confirm the goroutine/caller context where this code runs.
• Check if another code path already handles the case.
• Search for existing tests.
• Check git blame for recent changes.
• For concurrency bugs, confirm the lock is actually shared across goroutines.

Step 6: Report findings

Severity classification

Output format

## Bug Review: [Scope Description]

### Summary
- **Findings**: X (Y High, Z Medium, ...)
- **Code areas reviewed**: [list]

### Findings

#### [BUG-001] [Category] — [Brief description] (Severity)
- **Location**: `file.go:123`
- **Pattern**: [Which known pattern this matches, or "novel"]
- **Issue**: [What the bug is]
- **Impact**: [What goes wrong at runtime]
- **Evidence**:
  ```go
  [Buggy code snippet]

• Recommended fix:

  [Fixed code snippet]
  

• Historical precedent: [Commit that fixed a similar bug, if applicable]

Confirmed Safe

[Patterns that looked suspicious but were verified as correct]

### False-positive controls

Do NOT flag:
- Locks that are acquired and released within the same function with no other lock held
- `sync.Cond.Broadcast()` called while the associated lock is held
- `&Type{}` for types that are designed to work with zero values (e.g., `sync.Mutex`, `sync.WaitGroup`)
- `strings.Contains(err.Error()...)` with explicit code comments referencing the upstream issue being worked around
- Named returns used with `errors.Join(err, file.Close())` — this is the correct pattern for capturing close errors
- `json.Marshal` on initialized empty slices (`make([]T, 0)` or `[]T{}`) — these produce `[]` not `null`
- Deferred unlocks in short functions with no other lock acquisition
- Retry logic that has an explicit timeout/max-duration boundary

### Known false-positive patterns

| Pattern | Why it's safe |
|---------|--------------|
| `defer m.servicesLock.Unlock()` in single-lock functions | No other lock acquired in scope |
| `noticeCond.Broadcast()` in `AddNotice` | Called inside `s.writing()` which holds the state lock |
| `errors.Join(err, file.Close())` with named return | Intentional pattern to capture both errors |
| `make([]serviceInfo, 0, len(services))` | Initialized empty slice, marshals as `[]` |
| `tomb.Go(func)` immediately after tomb creation | Goroutine started, `Wait()` won't block |
| User lookup string matching with documented Go issue reference | Acknowledged workaround for Go #67912 |
| `lxd init --minimal` on already-initialised system | Tested: `lxd init --minimal` is idempotent and succeeds on re-run |