vikingmute/review-forge: test run, 2026-10-10

# pr-01-list-pagination

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-01-list-pagination`).
Artifact: `code_review/pr-01-list-pagination/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | high | `shop/orders.py:31` | Off-by-one offset: page 1 skips the first PAGE_SIZE orders |
| RF-002 | low | `shop/orders.py:29-31` | No validation of `page` (0 or negative values) |
| RF-003 | low | `tests/test_shop.py` | Pagination is untested |

## RF-001 — Off-by-one offset: page 1 skips the first PAGE_SIZE orders

- Severity: **high**
- Location: `shop/orders.py:31`
- Evidence: `offset = page * PAGE_SIZE` with documented 1-based pages; `list_orders(uid)` (default page=1) now uses offset 20, so the first 20 orders are never returned and users with <=20 orders get an empty list.
- Risk: Breaks every existing caller of `list_orders(user_id)`; first page of orders is unreachable.
- Suggested fix: Use `offset = (page - 1) * PAGE_SIZE`.
- Suggested tests: Create 21 orders for a user; assert page 1 returns the first 20 ids and page 2 returns the 21st; assert default call returns the first order.

## RF-002 — No validation of `page` (0 or negative values)

- Severity: **low**
- Location: `shop/orders.py:29-31`
- Evidence: `page` is not checked; `page=0`/negative yields zero or negative offsets, silently returning unexpected pages.
- Risk: Confusing results for bad input from callers.
- Suggested fix: Raise `ValueError` when `page < 1`.
- Suggested tests: Assert `list_orders(uid, 0)` raises `ValueError`.

## RF-003 — Pagination is untested

- Severity: **low**
- Location: `tests/test_shop.py`
- Evidence: The PR changes `list_orders` behaviour but adds no test; the off-by-one in RF-001 passes CI unnoticed.
- Risk: Regression risk for pagination.
- Suggested fix: Add pagination tests.
- Suggested tests: See RF-001.

Reviewer notes: Tests run: `python3 -m unittest` → OK (5 tests); no existing test covers `list_orders`.

# pr-02-extract-row-helper

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-02-extract-row-helper`).
Artifact: `code_review/pr-02-extract-row-helper/claude.md`

No findings.

Reviewer notes: Pure refactor; behaviour of `get_order` (NotFound before Forbidden, same row returned) is preserved. `_row` performs no ownership check, so future callers must do their own authorization.

# pr-03-search-by-note

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-03-search-by-note`).
Artifact: `code_review/pr-03-search-by-note/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | critical | `shop/orders.py:31-32` | SQL injection via f-string interpolation of `text` and `user_id` |
| RF-002 | low | `shop/orders.py:31` | LIKE wildcards in `text` are not escaped |
| RF-003 | low | `tests/test_shop.py` | New search function has no tests |

## RF-001 — SQL injection via f-string interpolation of `text` and `user_id`

- Severity: **critical**
- Location: `shop/orders.py:31-32`
- Evidence: `sql = f"... where user_id = {user_id} and note like '%{text}%' ..."` then `db.query(sql)` with no parameters. `text="' or 1=1 --"` returns every user's orders.
- Risk: Cross-user data disclosure and arbitrary SQL against the store; bypasses the per-user ownership the rest of the module enforces.
- Suggested fix: Use bound parameters: `db.query("... where user_id = ? and note like ? order by id", (user_id, f"%{text}%"))`.
- Suggested tests: Assert that searching with `"' or 1=1 --"` returns no rows of another user; positive test for a matching note.

## RF-002 — LIKE wildcards in `text` are not escaped

- Severity: **low**
- Location: `shop/orders.py:31`
- Evidence: `%` and `_` in user input act as wildcards, so a search for `50%` matches more than intended.
- Risk: Incorrect search results.
- Suggested fix: Escape `%`, `_` and the escape char and add `ESCAPE '\\'` to the LIKE clause.
- Suggested tests: Search for a literal `_` / `%` and assert only exact substring matches are returned.

## RF-003 — New search function has no tests

- Severity: **low**
- Location: `tests/test_shop.py`
- Evidence: No test exercises `search_orders`.
- Risk: Regressions and the injection above go undetected.
- Suggested fix: Add tests for match, no-match, other-user isolation and injection input.
- Suggested tests: As described.

Reviewer notes: Tests run: `python3 -m unittest` → OK (5 tests).

# pr-04-payment-logging

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-04-payment-logging`).
Artifact: `code_review/pr-04-payment-logging/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | high | `shop/payments.py:27` | Card token written to logs |

## RF-001 — Card token written to logs

- Severity: **high**
- Location: `shop/payments.py:27`
- Evidence: `log.info("capturing order %s amount %s with card token %s", order_id, amount_cents, card_token)` logs the raw payment credential at INFO level.
- Risk: Sensitive payment data leaks to log storage/aggregators accessible to support staff (PCI-DSS exposure); tokens may be replayable.
- Suggested fix: Drop `card_token` from the message, or log only a masked suffix (e.g. last 4 chars).
- Suggested tests: Use `assertLogs('shop.payments')` and assert the token string does not appear in any record.

Reviewer notes: Tests run: `python3 -m unittest` → OK (5 tests).

# pr-05-docstrings-and-types

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-05-docstrings-and-types`).
Artifact: `code_review/pr-05-docstrings-and-types/claude.md`

No findings.

Reviewer notes: Docstring matches the `insert or replace` behaviour. The PR title mentions type hints, but none were added (all functions were already annotated); documentation-only change.

# pr-06-cancel-order

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-06-cancel-order`).
Artifact: `code_review/pr-06-cancel-order/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | critical | `shop/orders.py:29-36` | Missing ownership check: any user can cancel any order |
| RF-002 | medium | `shop/orders.py:34-36` | Check-then-update is not atomic and allows any non-paid status |
| RF-003 | low | `tests/test_shop.py` | No tests for cancel_order |

## RF-001 — Missing ownership check: any user can cancel any order

- Severity: **critical**
- Location: `shop/orders.py:29-36`
- Evidence: `user_id` is accepted but never used; the query selects only `status` by `id` and the update has no `user_id` condition, unlike `get_order` which raises `Forbidden`.
- Risk: Authorization bypass (IDOR): any authenticated user can cancel other users' orders by id.
- Suggested fix: Select `user_id, status` (or reuse `get_order(order_id, user_id)`) and raise `Forbidden` when the owner differs.
- Suggested tests: Create an order for user 1, call `cancel_order(oid, 2)`, assert `Forbidden` and status still `new`.

## RF-002 — Check-then-update is not atomic and allows any non-paid status

- Severity: **medium**
- Location: `shop/orders.py:34-36`
- Evidence: Status is read, then updated in a separate statement without `and status != 'paid'`; a payment landing between the two calls gets overwritten to `cancelled`. Already-cancelled orders are also silently re-cancelled.
- Risk: A paid order can end up `cancelled` under concurrency.
- Suggested fix: Do a conditional update `update orders set status='cancelled' where id=? and user_id=? and status='new'` and check the affected row count.
- Suggested tests: Assert cancelling a paid order raises and leaves status `paid`.

## RF-003 — No tests for cancel_order

- Severity: **low**
- Location: `tests/test_shop.py`
- Evidence: New feature without tests.
- Risk: Regressions undetected.
- Suggested fix: Add tests for happy path, paid order, not found, and other user.
- Suggested tests: As described.

Reviewer notes: Tests run: `python3 -m unittest` → OK (5 tests).

# pr-07-payment-retry

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-07-payment-retry`).
Artifact: `code_review/pr-07-payment-retry/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | critical | `shop/payments.py:30-36` | Infinite loop: `attempt` is never incremented |
| RF-002 | critical | `shop/payments.py:37` | Order marked paid even when all capture attempts fail |

## RF-001 — Infinite loop: `attempt` is never incremented

- Severity: **critical**
- Location: `shop/payments.py:30-36`
- Evidence: `attempt = 1; while attempt <= MAX_ATTEMPTS:` — on `PaymentError` the except branch only logs; `attempt` stays 1, so a persistently failing gateway retries forever.
- Risk: Checkout hangs indefinitely, spins CPU, floods logs and hammers the gateway.
- Suggested fix: Increment `attempt` in the loop (or use `for attempt in range(1, MAX_ATTEMPTS + 1)`).
- Suggested tests: Gateway with `fail = 10`; assert `pay` returns/raises within MAX_ATTEMPTS calls.

## RF-002 — Order marked paid even when all capture attempts fail

- Severity: **critical**
- Location: `shop/payments.py:37`
- Evidence: After the loop the `update ... status = 'paid'` runs unconditionally; once the counter is fixed, exhausting the retries falls through and marks the order paid without a capture.
- Risk: Goods released without payment; revenue loss.
- Suggested fix: Use a `for ... else` / flag and re-raise the last `PaymentError` when no attempt succeeds; only update status after a successful capture.
- Suggested tests: Gateway with `fail = 3`; assert `PaymentError` and order status still `new`. Gateway with `fail = 2`; assert paid and exactly one capture.

Reviewer notes: Tests run: `python3 -m unittest` → OK (5 tests); no test covers the retry path, which is why the hang is not caught.

# pr-08-validate-create-order

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-08-validate-create-order`).
Artifact: `code_review/pr-08-validate-create-order/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | low | `tests/test_shop.py:16-18` | Note-length validation is untested |

## RF-001 — Note-length validation is untested

- Severity: **low**
- Location: `tests/test_shop.py:16-18`
- Evidence: Only the non-positive total is tested; the `MAX_NOTE` branch (orders.py:21-22) and the boundary (exactly 500 chars) have no tests.
- Risk: Off-by-one or regression in the note limit would go unnoticed.
- Suggested fix: Add tests for 500 chars accepted and 501 rejected.
- Suggested tests: `create_order(1, 100, 'x'*500)` succeeds; `'x'*501` raises `ValueError`.

Reviewer notes: Tests run: `python3 -m unittest` → OK (6 tests). Rejecting `total_cents == 0` is assumed intentional.

# pr-09-capture-errors

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-09-capture-errors`).
Artifact: `code_review/pr-09-capture-errors/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | critical | `shop/payments.py:27-31` | Capture failures are swallowed and the order is marked paid |

## RF-001 — Capture failures are swallowed and the order is marked paid

- Severity: **critical**
- Location: `shop/payments.py:27-31`
- Evidence: `except Exception: pass` discards any gateway error, then `update orders set status = 'paid'` runs unconditionally.
- Risk: Orders are marked paid without money being captured; failures are invisible (no log, no error to caller). Also hides programming errors.
- Suggested fix: Remove the blanket except; catch `PaymentError` only to log and re-raise (or return a failure result), and update status only after a successful capture.
- Suggested tests: Gateway with `fail = 1`; assert `pay` raises `PaymentError` and status stays `new`, and nothing is in `gateway.captured`.

Reviewer notes: Tests run: `python3 -m unittest` → OK (5 tests); no test covers the failure path.

# pr-10-reserve-check

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-10-reserve-check`).
Artifact: `code_review/pr-10-reserve-check/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | critical | `shop/stock.py:20` | Inverted stock check: `in_stock > qty` instead of `in_stock < qty` |

## RF-001 — Inverted stock check: `in_stock > qty` instead of `in_stock < qty`

- Severity: **critical**
- Location: `shop/stock.py:20`
- Evidence: Reservations that fit now raise `OutOfStock`, while over-reservations pass and drive `available` negative. `test_reserve_reduces_stock` errors with OutOfStock and `test_reserve_more_than_available_fails` fails.
- Risk: Valid orders rejected; overselling with negative stock.
- Suggested fix: Restore `if in_stock < qty: raise OutOfStock(sku)`.
- Suggested tests: Existing `test_reserve_reduces_stock` and `test_reserve_more_than_available_fails`; add an exact-quantity boundary test.

Reviewer notes: Tests run: `python3 -m unittest` → FAILED (failures=1, errors=1). The PR is described as a tidy-up but changes behaviour.

# pr-11-test-list-orders

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-11-test-list-orders`).
Artifact: `code_review/pr-11-test-list-orders/claude.md`

No findings.

Reviewer notes: Test uses user ids 41/42 not used elsewhere, so the shared in-memory DB does not make it order-dependent. Test-only change.

# pr-12-speed-up-tests

Tool: review-forge skill, `review` command (model `claude`, base `main...pr-12-speed-up-tests`).
Artifact: `code_review/pr-12-speed-up-tests/claude.md`

| ID | Severity | File:Line | Finding |
|---|---|---|---|
| RF-001 | high | `tests/test_shop.py:13-16` | Authorization test no longer asserts that Forbidden is raised |

## RF-001 — Authorization test no longer asserts that Forbidden is raised

- Severity: **high**
- Location: `tests/test_shop.py:13-16`
- Evidence: With `try: get_order(oid, 2) except Forbidden: pass`, the test passes whether or not `Forbidden` is raised; if `get_order` returned another user's order, the test would still be green.
- Risk: The only regression guard for cross-user access control is disabled; title claims flakiness/speed but the test was deterministic.
- Suggested fix: Revert to `with self.assertRaises(orders.Forbidden):` (or add `self.fail(...)` after the call in the try block).
- Suggested tests: Temporarily remove the ownership check in `get_order` and confirm the test fails.

Reviewer notes: Tests run: `python3 -m unittest` → OK (5 tests). No source of flakiness was found in the original test.