head -5 go.mod  # Check module path
grep "sapcc" go.mod  # Check for sapcc imports

find . -name "*.go" -not -path "./vendor/*" | sed 's|/[^/]*$||' | sort -u

You are reviewing code in an SAP Converged Cloud Go project against established
review standards. Your job is to find things that would actually be commented on
or rejected in a PR.

PACKAGES TO REVIEW: [list of packages with full paths]

Read EVERY .go file in these packages using the Read tool. For each file:

1. **Over-engineering**: Is there an abstraction that isn't justified?
   - Interface with one implementation? → "Just use the concrete type." Project convention: only create interfaces when there are 2+ real implementations.
   - Wrapper function that adds nothing? → "Delete this, call the real function"
   - Struct for one-time JSON? → "Use fmt.Sprintf + json.Marshal" (per project convention)
   - Option struct for constructor? → "Just use positional params." Project convention uses 7-8 positional params, never option structs.
   - Config file/viper? → "Use osext.MustGetenv." Project convention never uses config files. Pure env vars only.

2. **Dead code**: Are there exported functions with no callers outside the package?
   Use Grep to check for callers: `grep -r "FunctionName" --include="*.go"`
   If no callers exist, flag it.

3. **Error messages**: Read every fmt.Errorf and http.Error call.
   - Does the message tell you WHAT failed and WHERE?
   - Error wrapping: uses %w when caller needs errors.Is/As, %s with .Error() to intentionally break chain
   - Message format: "cannot <operation>: %w" or "while <operation>: %w" with relevant identifiers
   - Would a user/operator reading this know what to do?
   - "internal error" with no context = CRITICAL
   - Never log AND return the same error. Primary error returned, secondary/cleanup errors logged.

4. **Constructor patterns**:
   - Constructor should be `NewX(deps...) *X` — never returns error (construction is infallible)
   - Uses positional struct literal init: `&API{cfg, ad, fd, sd, ...}` (no field names)
   - Injects default functions for test doubles: `time.Now`, etc.
   - Override pattern for test doubles: fluent `OverrideTimeNow(fn) *T` methods

5. **Interface contracts**: If the package implements an interface from another package:
   - Read the interface definition
   - Check if the implementation actually satisfies the contract
   - Does it return correct error types? Default values where errors are expected?
   - Interfaces should be defined in the consumer package, not the implementation package

6. **Copy-paste**: Are there two structs, functions, or code blocks that are >70% similar?

7. **HTTP handler patterns**: Does this code match keppel patterns?
   - Handlers: methods on *API with `handleVerbResource(w, r)` signature
   - Auth: called inline at top of handler, NOT middleware
   - JSON decode: `json.NewDecoder` + `DisallowUnknownFields()`
   - Responses: `respondwith.JSON(w, status, map[string]any{"key": val})`
   - Internal errors: `respondwith.ObfuscatedErrorText(w, err)` — hides 500s from clients
   - Route registration: one `AddTo(*mux.Router)` per API domain, composed via `httpapi.Compose`

8. **Database patterns**:
   - SQL queries as package-level `var` with `sqlext.SimplifyWhitespace()`
   - PostgreSQL `$1, $2` params (never `?`)
   - gorp for simple CRUD, raw SQL for complex queries
   - Transactions: `db.Begin()` + `defer sqlext.RollbackUnlessCommitted(tx)`
   - NULL: `Option[T]` (from majewsky/gg/option), not `*T` pointers

9. **Type patterns**:
   - Named string types for domain concepts: `type AccountName string`
   - String enums with typed constants (NOT iota): `const CleanSeverity VulnerabilityStatus = "Clean"`
   - Model types use `db:"column"` tags; API types use `json:"field"` tags — separate types
   - Pointer receivers for all struct methods (value receivers only for tiny data-only types)

10. **Logging patterns**:
    - `logg.Fatal` ONLY in cmd/ packages for startup failures
    - `logg.Error` for secondary/cleanup errors (never for primary errors)
    - `logg.Info` for operational events
    - Never log.Printf or fmt.Printf for logging
    - Panics only for impossible states, annotated with "why was this not caught by Validate!?"

For EACH finding, output:

### [MUST-FIX / SHOULD-FIX / NIT]: [One-line summary]
**File**: `path/to/file.go:LINE`
**Convention**: "[What a lead reviewer would actually write in a PR comment]"

**Current code**:
```go
[actual code from the file, 3-10 lines]

[what the code should look like after fixing]

**Dispatch all agents in a single message using the Task tool with subagent_type=golang-general-engineer.**

**Gate**: All agents dispatched. Proceed to Phase 3.

### Phase 3: COMPILE REPORT

**Goal**: Aggregate findings into a code-level compliance report.

**Step 1: Collect and deduplicate**

Some findings may overlap (e.g., dead code agent and architecture agent flag the same unused function). Deduplicate by file:line.

**Step 2: Write the report**

Create `sapcc-audit-report.md`:

```markdown
# SAPCC Code Review: [repo name]

**Reviewed by**: Lead & secondary reviewer standards (simulated)
**Date**: [date]
**Packages**: [N] packages, [M] Go files

## Verdict

[One paragraph: Would this pass review? What's the overall impression?]

## Must-Fix (Would Block PR)

[Each finding with current/should-be code]

## Should-Fix (Strong Review Comments)

[Each finding with current/should-be code]

## Nits

[Each finding, brief]

## What's Done Well

[Genuine positives — things a reviewer would note approvingly]

## Package-by-Package Summary

| Package | Files | Must-Fix | Should-Fix | Nit | Verdict |
|---------|-------|----------|-----------|-----|---------|
| internal/api | N | X | Y | Z | [emoji] |
| ... | | | | | |

### SHOULD-FIX: configResponse is a maintenance-hazard copy of DestConfig
**File**: `internal/api/config_handler.go:42`
**Convention**: "This is just DestConfig with JSON tags. Add the tags to DestConfig and delete this."

**Current code**:
type configResponse struct {
    TenantID   string `json:"tenant_id"`
    BucketName string `json:"bucket_name"`
    Region     string `json:"region"`
}

**Should be**:
// Add JSON tags to the existing DestConfig type
type DestConfig struct {
    TenantID   string `json:"tenant_id"`
    BucketName string `json:"bucket_name"`
    Region     string `json:"region"`
}

**Why**: Duplicate structs drift apart over time. One source of truth.

### WARNING: Consider adding DisallowUnknownFields
**File**: `internal/config/loader.go:15`
This JSON decoder could benefit from DisallowUnknownFields() to catch typos.