Repository layer code review: fix critical transaction bug and high-severity issues #16

New Issue

hermes · 2026-07-09T13:37:26Z

hermes commented

2026-07-09 13:37:26 +00:00

Code Review Summary

OpenCode performed a detailed review of the repository layer (domain interfaces + SQLite/YDB adapters). Found 1 Critical, 5 High, 9 Medium, 10 Low issues plus significant test coverage gaps.

Priority 1 — Fix First

🔴 C1. `CreateBatch` commits partial failures & swallows commit errors

File: internal/kurious/adapters/sqlite_course_repository.go:292-327

The loop _, err := stmt.ExecContext(...) shadows the outer err via :=. When a row insert fails, the defer reads the outer (still-nil) err, takes the else branch, and COMMITS the partially-inserted batch. The caller gets an error but half the rows are persisted — transaction guarantee is broken.

On the happy path, tx.Commit() failure is silently dropped because err = errors.Join(err, errTx) writes to a discarded local (no named return).

Fix: Use named return (err error), rename loop var to execErr.

🟠 H2. `Organization().List` drops query arguments

File: internal/kurious/adapters/sqlite_organization_repository.go:190

r.db.SelectContext(ctx, &organizations, query) — args not passed! The IN (?,?) placeholders are unbound. List with IDs always errors.

Fix: r.db.SelectContext(ctx, &organizations, query, args...)

🟠 H3. `CourseRepository.Get` never returns `ErrNotFound`

File: internal/kurious/adapters/sqlite_course_repository.go:279-281

Wraps sql.ErrNoRows with fmt.Errorf instead of translating to cerrors.ErrNotFound. Breaks the port contract. Organization and LearningCategory repos handle this correctly.

Fix: Add errors.Is(err, sql.ErrNoRows) check before wrapping.

Priority 2 — Security & Contract

H1. SQL injection surface — OrderBy concatenated raw into query (sqlite_course_repository.go:79). Mitigated by HTTP layer allowlist but repo should validate independently.
H4. YDB List ignores Offset, OrderBy, Ascending; cursor never fed back. Port contract divergence.
H5. AGENTS.md falsely claims YDB is unsupported — service.go:57-65 still wires it.

Priority 3 — Consistency & Cleanup (Medium)

Create returns empty Course{} — no created entity back
Inconsistent time.Now() — local vs UTC across repos
Prepared statement leak in Organization.Create
Organization.Get with empty params returns arbitrary row
NextPageToken computed but never consumed (dead cursor)
WHERE clause duplicated between List and listCount
memory_mapper has no mutex — potential data race
Stub methods use errors.New(...) instead of sentinel
Duration precision loss in SQLite vs full precision in YDB

Priority 4 — Typos & Style (Low)

OrganizaitonID, sqliteLearingCategoryRepository, nullable.ValutPtr(), "organizaitons" trace, duplicate trace.WithSpanKind, no-op Close() timeout, dead FromDomain methods, config naming drift.

Test Coverage Gaps

No test covers C1, H2, or H3. Missing: transaction rollback, Organization.List with IDs, Course.Get not-found, all filter/order/pagination combos, ListStatistics, ListStats, Delete, all YDB repos. Existing TestListLimitOffset is fragile.

## Code Review Summary OpenCode performed a detailed review of the repository layer (domain interfaces + SQLite/YDB adapters). Found **1 Critical, 5 High, 9 Medium, 10 Low** issues plus significant test coverage gaps. ### Priority 1 — Fix First #### 🔴 C1. `CreateBatch` commits partial failures & swallows commit errors **File:** `internal/kurious/adapters/sqlite_course_repository.go:292-327` The loop `_, err := stmt.ExecContext(...)` shadows the outer `err` via `:=`. When a row insert fails, the defer reads the **outer** (still-nil) `err`, takes the **else branch, and COMMITS** the partially-inserted batch. The caller gets an error but half the rows are persisted — transaction guarantee is broken. On the happy path, `tx.Commit()` failure is silently dropped because `err = errors.Join(err, errTx)` writes to a discarded local (no named return). **Fix:** Use named return `(err error)`, rename loop var to `execErr`. #### 🟠 H2. `Organization().List` drops query arguments **File:** `internal/kurious/adapters/sqlite_organization_repository.go:190` `r.db.SelectContext(ctx, &organizations, query)` — `args` not passed! The `IN (?,?)` placeholders are unbound. `List` with `IDs` always errors. **Fix:** `r.db.SelectContext(ctx, &organizations, query, args...)` #### 🟠 H3. `CourseRepository.Get` never returns `ErrNotFound` **File:** `internal/kurious/adapters/sqlite_course_repository.go:279-281` Wraps `sql.ErrNoRows` with `fmt.Errorf` instead of translating to `cerrors.ErrNotFound`. Breaks the port contract. Organization and LearningCategory repos handle this correctly. **Fix:** Add `errors.Is(err, sql.ErrNoRows)` check before wrapping. ### Priority 2 — Security & Contract - **H1.** SQL injection surface — `OrderBy` concatenated raw into query (`sqlite_course_repository.go:79`). Mitigated by HTTP layer allowlist but repo should validate independently. - **H4.** YDB `List` ignores `Offset`, `OrderBy`, `Ascending`; cursor never fed back. Port contract divergence. - **H5.** AGENTS.md falsely claims YDB is unsupported — `service.go:57-65` still wires it. ### Priority 3 — Consistency & Cleanup (Medium) - `Create` returns empty `Course{}` — no created entity back - Inconsistent `time.Now()` — local vs UTC across repos - Prepared statement leak in `Organization.Create` - `Organization.Get` with empty params returns arbitrary row - `NextPageToken` computed but never consumed (dead cursor) - WHERE clause duplicated between `List` and `listCount` - `memory_mapper` has no mutex — potential data race - Stub methods use `errors.New(...)` instead of sentinel - Duration precision loss in SQLite vs full precision in YDB ### Priority 4 — Typos & Style (Low) - `OrganizaitonID`, `sqliteLearingCategoryRepository`, `nullable.ValutPtr()`, `"organizaitons"` trace, duplicate `trace.WithSpanKind`, no-op `Close()` timeout, dead `FromDomain` methods, config naming drift. ### Test Coverage Gaps No test covers C1, H2, or H3. Missing: transaction rollback, Organization.List with IDs, Course.Get not-found, all filter/order/pagination combos, ListStatistics, ListStats, Delete, all YDB repos. Existing `TestListLimitOffset` is fragile.

hermes self-assigned this 2026-07-09 13:37:26 +00:00

hermes referenced a pull request that will close this issue

2026-07-09 13:42:33 +00:00

Fix #16: Repository layer code review: fix critical transaction bug and high-severity issues #17

hermes closed this issue

2026-07-20 16:49:36 +00:00

hermes referenced this issue from a commit

2026-07-20 16:49:37 +00:00

Fix #16: Repository layer code review: fix critical transaction bug and high-severity issues (#17)

hermes commented

2026-07-20 16:50:07 +00:00

Issue #16 Resolved — Merged

Root Cause

Code review identified a critical transaction bug and multiple high-severity issues in the repository layer (sqlite_course_repository.go).

Key Changes

Critical transaction bug fix in repository layer — transaction handling corrected
ORDER BY whitelist changed from map[string]bool to map[string]struct{} (zero-size value type, idiomatic Go for set membership)
High-severity repository issues addressed (see commit 7bf95f1)

Review Feedback Addressed

@frx requested map[string]struct{} for the fields whitelist → implemented in 590fed2, lookup updated to comma-ok idiom

Test Results

go build ./... — clean
go test ./... — all packages pass

PR: #17 (merged at 2026-07-20T16:49:36Z)
Merge commit: 7e83e5a
Changes: +97 / -5

## Issue #16 Resolved — Merged ### Root Cause Code review identified a critical transaction bug and multiple high-severity issues in the repository layer (`sqlite_course_repository.go`). ### Key Changes - **Critical transaction bug fix** in repository layer — transaction handling corrected - **ORDER BY whitelist** changed from `map[string]bool` to `map[string]struct{}` (zero-size value type, idiomatic Go for set membership) - High-severity repository issues addressed (see commit `7bf95f1`) ### Review Feedback Addressed - @frx requested `map[string]struct{}` for the fields whitelist → implemented in `590fed2`, lookup updated to comma-ok idiom ### Test Results - `go build ./...` — clean - `go test ./...` — all packages pass --- - PR: #17 (merged at 2026-07-20T16:49:36Z) - Merge commit: `7e83e5a` - Changes: +97 / -5

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: frx/kurious#16

Repository layer code review: fix critical transaction bug and high-severity issues #16