Code Review dengan Claude Code: Verifikasi Kode Golang vs Spesifikasi

 1Lakukan spec compliance review untuk implementasi cancel order.
 2
 3SPEC (specs/order/cancel-order.md v1.3):
 4@specs/order/cancel-order.md
 5
 6IMPLEMENTASI:
 7File 1: internal/usecase/order/cancel_order.go
 8@internal/usecase/order/cancel_order.go
 9
10File 2: internal/repository/postgres/order_repository.go
11@internal/repository/postgres/order_repository.go
12
13File 3: internal/delivery/http/handler/order_handler.go
14@internal/delivery/http/handler/order_handler.go
15
16Review checklist:
171. Untuk setiap AC (AC1-AC10): apakah ter-implement? Di mana?
182. Untuk setiap EC (EC1-EC4): apakah ter-handle? Bagaimana?
193. Apakah response format sesuai spec (HTTP status, error_code, message)?
204. Apakah error mapping dari usecase ke handler sudah lengkap?
215. Apakah ada AC/EC yang missing?
22
23Output: tabel compliance dengan status ✅/❌/⚠️

 1## Spec Compliance Review — Cancel Order
 2## Spec: specs/order/cancel-order.md v1.3
 3## Reviewed: 2025-07-01
 4
 5| Item | Status | Location | Notes |
 6|------|--------|----------|-------|
 7| AC1: DELETE /api/v1/orders/:id dengan path param | ✅ | handler.go:45 | Path param :id parsed correctly |
 8| AC2: ownership check | ✅ | repo.go:89 | GetByIDAndUserID returns nil for non-owner |
 9| AC3: PENDING status check | ✅ | entity.go:34 | CanBeCancelled() checks StatusPending |
10| AC4: 15-min window | ✅ | entity.go:35 | time.Since(CreatedAt) <= 15*time.Minute — batas INKLUSIF: order berumur tepat 15 menit 00 detik masih bisa dibatalkan |
11| AC5: atomic stock restore | ✅ | repo.go:103 | Within same pgx transaction |
12| AC6: 204 response | ✅ | handler.go:89 | c.NoContent(http.StatusNoContent) |
13| AC7: not found → ErrOrderNotFound | ✅ | usecase.go:45 | Correct sentinel error |
14| AC8: not owner → same error | ✅ | repo.go:89 | Returns nil, nil for security |
15| AC9: non-PENDING → 409 | ✅ | handler.go:73 | OrderNotCancellableError → 409 |
16| AC10: window expired → 409 | ✅ | handler.go:79 | CancelWindowExpiredError → 409 |
17| EC1: concurrent cancel | ✅ | repo.go:98 | SELECT FOR UPDATE in transaction |
18| EC2: Kafka failure | ✅ | usecase.go:65 | Best effort + slog.WarnContext |
19| EC3: DB timeout | ⚠️ | - | No explicit timeout handling. Consider context with timeout |
20| EC4: stock restore rollback | ✅ | repo.go:~110 | defer tx.Rollback, order tetap PENDING |
21
22## Issues Found
23
24### ⚠️ EC3: Database Timeout Not Handled
25**Location:** usecase.Execute → repo.CancelWithStockRestore
26**Issue:** No explicit timeout set on the context for DB operations.
27**Risk:** Long-running DB locks (during concurrent cancel EC1) could block indefinitely.
28**Recommendation:** Add context deadline:
29    cancelCtx, cancel := context.WithTimeout(ctx, 3*time.Second)
30    defer cancel()
31    err := uc.repo.CancelWithStockRestore(cancelCtx, input.OrderID)
32
33### ✅ Overall Compliance Score: 13/14 items covered, 3/4 ECs fully handled
34
35## Notes
36- Error codes in handler match spec exactly (ORDER_NOT_FOUND, ORDER_NOT_CANCELLABLE, CANCEL_WINDOW_EXPIRED)
37- Security consideration maintained: same error for "not found" and "not owner"
38- Logging implemented per NFR-O3 (INFO on success, WARNING on Kafka failure)

 1Review kode CancelOrderUseCase.Execute dari sudut pandang senior Go engineer.
 2
 3Context dari spec:
 4- NFR-P2: p95 latency < 500ms (jadi perhatian ke performance)
 5- NFR-S4: Audit log untuk semua operasi yang mengubah data order
 6- NFR-O3: Log INFO setiap state change
 7
 8Review checklist:
 91. Apakah error handling sudah idiomatic Go?
102. Apakah ada potential panic atau nil dereference?
113. Apakah ada goroutine atau resource leak?
124. Apakah logging sudah sesuai NFR-S4 dan NFR-O3?
135. Apakah ada cara untuk membuat kode lebih readable?
146. Apakah ada performance concern untuk NFR-P2?
15
16Berikan feedback spesifik dengan saran perbaikan yang actionable.

 1// BEFORE
 2func (uc *CancelOrderUseCase) Execute(ctx context.Context, input CancelOrderInput) error {
 3    order, err := uc.repo.GetByIDAndUserID(ctx, input.OrderID, input.UserID)
 4    if err != nil || order == nil {
 5        return ErrOrderNotFound
 6    }
 7    // ...
 8}
 9
10// AFTER
11func (uc *CancelOrderUseCase) Execute(ctx context.Context, input CancelOrderInput) error {
12    order, err := uc.repo.GetByIDAndUserID(ctx, input.OrderID, input.UserID)
13    if err != nil {
14        return fmt.Errorf("get order: %w", err)
15    }
16    if order == nil {
17        return ErrOrderNotFound
18    }
19    // ...
20}

1Review perubahan ini:
21. Apakah perubahan sudah benar? (sekarang error dari DB tidak di-silently ignore)
32. Apakah ada side effect yang perlu diperhatikan?
43. Apakah ini sesuai dengan spec AC7/AC8?

 1#!/bin/bash
 2# scripts/check-spec-compliance.sh
 3
 4SPEC_FILE="specs/order/cancel-order.md"
 5IMPL_DIR="internal"
 6
 7echo "=== Spec Compliance Check: Cancel Order ==="
 8
 9# Check AC6: 204 No Content response
10if ! grep -r "StatusNoContent" "$IMPL_DIR/delivery/http/handler/"; then
11    echo "❌ AC6: StatusNoContent (204) not found in handler"
12    exit 1
13fi
14
15# Check EC2: Kafka failure handled
16if ! grep -r "WarnContext\|Warn(" "$IMPL_DIR/usecase/order/cancel_order.go"; then
17    echo "⚠️ EC2: No warning log found for Kafka failure"
18fi
19
20# Check error codes match spec
21EXPECTED_CODES=("ORDER_NOT_FOUND" "ORDER_NOT_CANCELLABLE" "CANCEL_WINDOW_EXPIRED")
22for code in "${EXPECTED_CODES[@]}"; do
23    if ! grep -r "\"$code\"" "$IMPL_DIR/delivery/http/handler/"; then
24        echo "❌ Error code $code not found in handler"
25        exit 1
26    fi
27done
28
29echo "✅ Basic spec compliance check passed"

11. Developer selesai implementasi
22. AI spec compliance review → tangani issues yang ditemukan
33. Developer self-review dengan checklist
44. PR ke team → human code review
55. AI lakukan final check sebelum merge jika ada concerns

1Saya mau rename field OrderID menjadi ID di CancelOrderInput struct.
2
3Sebelum melakukan perubahan:
41. Apakah ini breaking change untuk caller yang ada?
52. File mana yang menggunakan CancelOrderInput?
63. Apakah ini sesuai dengan naming convention di spec atau CLAUDE.md?
74. Apakah ada test yang perlu diupdate?
8
9Berikan daftar semua tempat yang perlu diupdate jika perubahan dilakukan.

 1Review implementasi cancel order dari perspektif security:
 2
 3Spec security requirements yang harus dipenuhi:
 4- NFR-S1: Hanya authenticated user (JWT Bearer)
 5- NFR-S3: Rate limit 5 cancel per user per jam
 6- NFR-S12: Audit log untuk semua operasi yang mengubah data order
 7
 8Pertanyaan review:
 91. Apakah mungkin user tanpa autentikasi mencapai usecase layer?
102. Apakah audit log sudah ada dan cukup?
113. Apakah ada information leakage (misal: exposing order existence ke unauthorized user)?
124. Apakah rate limiting sudah di-test?
135. Apakah ada SQL injection vulnerability?
14
15Files:
16@internal/usecase/order/cancel_order.go

 1Ini PR yang akan di-merge:
 2
 3PR Title: feat(order): implement cancel order endpoint
 4Branch: feature/cancel-order
 5
 6Perubahan:
 7[paste diff atau list file changes]
 8
 9Sebagai third reviewer:
101. Apakah ada hal yang human reviewer mungkin terlewat?
112. Apakah ada security atau performance concern yang subtle?
123. Apakah konsisten dengan pola di codebase existing?
134. Apakah ada test yang seharusnya ada tapi tidak ada?
14
15Context: spec di specs/order/cancel-order.md, conventions di CLAUDE.md

 1Review migration ini:
 2
 3CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_orders_user_id_status
 4ON orders(user_id, status) WHERE deleted_at IS NULL;
 5
 61. Apakah migration ini aman untuk dijalankan di production tanpa downtime?
 72. Apakah index design-nya sudah optimal untuk query GetByIDAndUserID?
 83. Apakah ada potential issue dengan CONCURRENTLY di PostgreSQL version kita?
 94. Apakah conditional `WHERE deleted_at IS NULL` sudah benar?
105. Apakah ada index yang sudah ada yang overlap?
11
12Context: PostgreSQL 15, tabel orders punya ~5 juta rows

 1Review implementasi CancelWithStockRestore dari perspektif performance:
 2
 3Query yang digunakan:
 4SELECT id FROM orders WHERE id = $1 FOR UPDATE;
 5UPDATE orders SET status = 'CANCELLED', updated_at = NOW() WHERE id = $1;
 6UPDATE products p SET stock = p.stock + oi.quantity
 7FROM order_items oi WHERE oi.order_id = $1 AND oi.product_id = p.id;
 8
 9Context:
10- orders: 5 juta rows, indexed pada (id)
11- order_items: 20 juta rows, indexed pada (order_id, product_id)
12- products: 500.000 rows, indexed pada (id)
13- Target NFR-P2: p95 < 500ms
14
151. Apakah semua query menggunakan index yang tepat?
162. Apakah ada N+1 query yang tersembunyi?
173. Apakah lock contention dari SELECT FOR UPDATE bisa menjadi bottleneck?
184. Estimasi waktu query di normal load?

 1Bandingkan implementasi cancel order dengan create order dari sisi consistency:
 2
 3Create Order (internal/usecase/order/create_order.go):
 4@internal/usecase/order/cancel_order.go
 5
 6Cancel Order (internal/usecase/order/cancel_order.go):
 7@internal/usecase/order/cancel_order.go
 8
 9Identifikasi inconsistency dalam:
101. Error handling style (wrapping, sentinel errors)
112. Logging pattern (level, structured fields)
123. Naming conventions (input struct, error types)
134. Dependency injection pattern
14
15Tujuan: keduanya harus feel seperti ditulis oleh orang yang sama.
16Berikan daftar inconsistency dan saran untuk menyeragamkan.

 1## Pre-Merge Checklist — Cancel Order PR
 2
 3### Spec Compliance
 4- [ ] Semua AC (AC1-AC10) ter-implement
 5- [ ] Semua EC (EC1-EC4) ter-handle
 6- [ ] Response format sesuai spec (HTTP status codes, error_code, message)
 7- [ ] NFR considerations ter-implement (metrics, logging)
 8
 9### Code Quality
10- [ ] go build ./... berhasil
11- [ ] go test -race ./... berhasil
12- [ ] go vet ./... berhasil
13- [ ] golangci-lint run ./... berhasil
14- [ ] Coverage >= 85% untuk usecase layer
15
16### Database
17- [ ] Migration sudah dibuat jika ada schema change
18- [ ] Migration sudah di-test di local
19- [ ] Migration aman untuk production (CONCURRENTLY jika ada index)
20
21### Security
22- [ ] Tidak ada sensitive data di log
23- [ ] SQL queries menggunakan parameterized queries
24- [ ] Authentication check ada di middleware (tidak di usecase)
25
26### Documentation
27- [ ] Komentar di kode menjelaskan "why" bukan "what"
28- [ ] Spec-to-code tracing ada di comments penting
29
30### AI Review
31- [ ] Spec compliance review sudah dilakukan
32- [ ] Code quality review sudah dilakukan
33- [ ] Semua issues dari AI review sudah ditangani atau di-acknowledge

 1Accept dan Fix:
 2Terima kasih atas catatan EC3. Kamu benar bahwa DB timeout tidak di-handle.
 3Saya akan tambahkan context.WithTimeout sesuai saran.
 4Pertanyaan: timeout 3 detik sudah sesuai dengan NFR-M4 di spec?
 5NFR-M4 bilang "downstream service call timeout: 2 detik." Apakah 3 detik terlalu lama?
 6
 7Acknowledge dan Defer:
 8Catatan tentang audit log (NFR-S12) valid. Ini memang belum di-implement
 9karena dalam scope awal tidak termasuk. Akan saya buat ticket untuk implement
10di sprint berikutnya. Apakah ini acceptable untuk merge dengan ticket yang ada?
11
12Disagree dengan Justification:
13Regarding EC3 timeout: saya tidak akan tambahkan context.WithTimeout di usecase layer.
14Alasannya: context deadline sudah di-set di HTTP middleware (30 detik timeout),
15dan context propagation akan handle cancellation jika client disconnect.
16Menambahkan timeout tambahan di usecase bisa menyebabkan partial state yang sulit di-debug.

 1// BEFORE
 2type Repository interface {
 3    GetByID(ctx context.Context, id uuid.UUID) (*Order, error)
 4}
 5
 6// AFTER
 7type Repository interface {
 8    GetByID(ctx context.Context, id uuid.UUID) (*Order, error)
 9    GetByIDAndUserID(ctx context.Context, orderID, userID uuid.UUID) (*Order, error)
10    CancelWithStockRestore(ctx context.Context, orderID uuid.UUID) error
11}

11. Apakah ini breaking change?
22. File mana yang implement interface ini yang perlu diupdate?
33. Apakah ada test yang akan break?
44. Apakah perlu deprecation period untuk method lama?
5
6Jalankan: grep -r "Repository" internal/ untuk temukan semua user.

 1🔴 RED — Harus di-fix sebelum merge:
 2- Spec AC yang tidak ter-implement
 3- Security vulnerability
 4- Data corruption risk
 5- Test yang cover kasus berbahaya tapi di-skip
 6
 7🟡 YELLOW — Perlu diskusi, mungkin OK:
 8- Pattern yang inconsistent dengan codebase existing
 9- Performance yang di bawah NFR target tapi masih acceptable
10- Missing audit log yang dalam scope "nice to have"
11
12🟢 GREEN — Suggestion, optional:
13- Refactoring untuk readability
14- Alternative approach yang tidak lebih baik, hanya berbeda
15- Komentar yang bisa ditambahkan untuk clarity

 1## Cancel Order Implementation
 2
 3**Spec:** specs/order/cancel-order.md v1.3
 4**Plan:** [link ke implementation plan]
 5
 6### Spec Compliance Self-Review
 7AI spec compliance review dilakukan. 13/14 item covered (10 AC + 4 EC).
 8
 9**Known gap:** EC3 (DB timeout) tidak di-handle eksplisit.
10Justification: Context dari HTTP middleware (30s timeout) sudah cover ini.
11Tracking: [jika perlu ticket]
12
13### Changes
14- internal/usecase/order/cancel_order.go (new)
15- internal/domain/order/entity.go (add CanBeCancelled)
16- internal/repository/postgres/order_repository.go (add 2 methods)
17- internal/delivery/http/handler/order_handler.go (add CancelOrder)