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:

[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.

undefined

**Step 3: Display summary**

Show the verdict, must-fix count, and top 5 findings inline. Point to the full report file.

**Gate**: Report complete.

---

undefined

undefined

undefined

This is too generic. A project reviewer wouldn't comment on this in most cases.

---