code-review

name	code-review
description	Use to review Go backend changes for correctness, security, modular-monolith boundaries, error handling, SQL safety, domain invariants, and missing tests. Trigger when the user asks for code review, PR review, audit findings, or correctness/security review.

Code Review Skill

You are reviewing Go code in the VMARBLE Warehouse Management Service (modular monolith).

Review checklist

Architecture & Module boundaries

• Module does NOT import another module package (black box rule)
• Cross-module deps use interfaces in deps.go, wired only in main.go
• Business logic is in service.go, NOT in handler.go or pgstore.go
• HTTP concerns stay in handler.go (param binding, error mapping)
• SQL stays in pgstore.go (queries, row mapping)

Go conventions

• gofmt formatted
• Import grouping: stdlib → third-party → local
• No stutter in names (e.g., inventory.Service not inventory.InventoryService)
• Only iface.go exports and internal/domain/ are exported; rest unexported
• Explicit names in business logic (planID not pID)

Error handling

• Uses sentinel errors from internal/domain/errors.go
• Wraps with NewBizError(sentinel, humanMessage) for domain failures
• No raw pgx/SQL errors leaked to handlers
• Error wrapping with fmt.Errorf("...: %w", err), no double-wrapping BizError
• Correct HTTP status mapping (400/404/409/412/422)

Context & IO

• context.Context accepted in store/service methods touching IO
• Handlers pass c.Request.Context()
• No context.Background() in request flows

Database

• QueryRow/Scan for single-row; checks pgx.ErrNoRows → ErrNotFound
• Transactions for multi-write atomicity
• SQL parameterized, no string concatenation with user input
• RETURNING preferred over follow-up selects

Domain invariants (must NOT violate)

• WorkOrder state machine: PLANNED → IN_CUTTING → IN_PROCESSING → COMPLETED → COSTED
• Area conservation: used_area + remnant_area <= source_area
• Remnant lineage: parent_board_id + optional parent_remnant_id
• Costing immutability: finalized records are immutable (BR-C04)
• Metal requirement: requires_metal=true needs METAL consumption before COMPLETED

Testing

• Table-driven tests where practical
• Deterministic (no sleeps, no time dependencies)
• Unit tests for service.go with mocked store/deps
• make lint clean

Output format

Provide findings as:

1. Critical — must fix (invariant violations, data loss risks)
2. Important — should fix (convention violations, error handling gaps)
3. Suggestion — nice to have (readability, naming improvements)

Include file:line references and suggested fixes.