Control: Claude Code with no review tool: test run, 2026-10-10

# pr-01-list-pagination

- **blocking** — `shop/orders.py:31`: `offset = page * PAGE_SIZE` is off by one page. The docstring says pages start at 1, but `page=1` (the default) gives offset 20, so it skips the first 20 orders. A user with fewer than 21 orders gets `[]` from `list_orders(user_id)`. I checked this: 3 orders → `[]`. It should be `(page - 1) * PAGE_SIZE`. This also breaks every existing caller that relies on the default.
- **should fix** — `shop/orders.py:29`: `page` is not validated. With `page <= 0` the offset is zero or negative (SQLite treats a negative offset as 0), so the call silently returns page 1 instead of raising.
- **should fix** — `tests/test_shop.py`: there is no test for pagination (first page, second page, page boundary). A first-page test would have caught the off-by-one.

# pr-02-extract-row-helper

No findings.

(Pure refactor: `get_order` still raises `NotFound` and then `Forbidden`, in the same order and with the same return value.)

# pr-03-search-by-note

- **blocking** — `shop/orders.py:31-32`: SQL injection. `text` (user input) and `user_id` go straight into the SQL string through an f-string. For example, `text = "x' or '1'='1"` returns every user's orders, which bypasses the `user_id` filter and leaks other users' data, and other payloads can read any table. Use parameters instead: `"... where user_id = ? and note like ? ..."` with `(user_id, f"%{text}%")`.
- **nit** — `shop/orders.py:31`: `%` and `_` in `text` are not escaped, so they act as LIKE wildcards. Escape them and add `ESCAPE '\'` if a literal match is intended.
- **should fix** — `tests/test_shop.py`: there are no tests for `search_orders`. It needs at least one test for matching and one for "does not return other users' orders".

# pr-04-payment-logging

- **blocking** — `shop/payments.py:27`: this logs `card_token` at INFO level. Card or payment tokens are sensitive credentials, and anyone with log access could use them to charge the card. Keeping them out of logs is also a PCI-DSS requirement. Remove the token from the message, or log only a masked suffix if support really needs it.

# pr-05-docstrings-and-types

No findings.

(The new docstring is accurate. Small point: the title mentions type hints but none are added; `stock.py` already has them.)

# pr-06-cancel-order

- **blocking** — `shop/orders.py:29-36`: there is no ownership check. `user_id` is accepted but never used, so any user can cancel any other user's order just by knowing its id. Do the same `Forbidden` check as `get_order`, for example by calling `get_order(order_id, user_id)` to load the row.
- **should fix** — `shop/orders.py:31-36`: check-then-act race. The status is read in one statement and updated in another, so an order paid between the two gets cancelled anyway. Make the update conditional instead: `update orders set status='cancelled' where id=? and user_id=? and status not in ('paid','cancelled')`, then check the rowcount.
- **nit** — `shop/orders.py:34`: cancelling an order that is already cancelled succeeds silently. The bare `ValueError` is also inconsistent with the module's own exception types (`NotFound`/`Forbidden`); consider a dedicated exception.
- **should fix** — `tests/test_shop.py`: there are no tests for `cancel_order`. It needs a happy path, a paid order being rejected, and another user being forbidden.

# pr-07-payment-retry

- **blocking** — `shop/payments.py:30-36`: infinite loop. `attempt` is never incremented, so if the gateway keeps failing, `pay()` spins forever and logs a warning on each pass. I confirmed this: with `Gateway.fail` set very high, `pay()` hangs and timed out after 5 seconds.
- **blocking** — `shop/payments.py:37`: even once the loop is bounded, when all attempts fail the code falls through and marks the order `paid` without capturing any money. After the last failed attempt it must re-raise `PaymentError`, or use a `for ... else: raise`, rather than reaching the `update`.
- **should fix** — `shop/payments.py:33`: retrying immediately after a gateway *timeout* can double-charge, because the first capture may have succeeded on the gateway side. Send an idempotency key (for example `order_id`) or check the capture status before retrying. Also consider adding backoff between attempts.
- **should fix** — `tests/test_shop.py`: there are no tests for the retry path, such as fail once then succeed, or fail on every attempt → raises and the order is not paid. `Gateway.fail` already exists for exactly this.

# pr-08-validate-create-order

- **nit** — `tests/test_shop.py:16-18`: only the `total_cents == 0` case is tested. Add tests for a negative total and for a note longer than `MAX_NOTE` (and exactly `MAX_NOTE`, which should be accepted).

# pr-09-capture-errors

- **blocking** — `shop/payments.py:27-31`: `except Exception: pass` swallows every capture failure, and execution then continues to mark the order `paid` and log "order paid". Any gateway error (declined card, timeout) produces a paid order with no money collected. The caller also loses the error. Catch `PaymentError` only, and either re-raise it or record a failed or pending status instead of `paid`.
- **should fix** — `shop/payments.py:29`: the broad `except Exception` also hides programming errors (TypeError and similar). Even if swallowing were intended, nothing is logged.
- **should fix** — `tests/test_shop.py`: there is no test for the failure path. With `Gateway.fail = 1`, the order must not end up `paid`.

# pr-10-reserve-check

- **blocking** — `shop/stock.py:20`: the comparison is inverted. `if in_stock > qty: raise OutOfStock` rejects reservations when there *is* enough stock and allows them when there isn't, so stock goes negative. The original `<` check was correct. The existing suite fails on this branch: `test_reserve_reduces_stock` errors with OutOfStock, and `test_reserve_more_than_available_fails` fails.

# pr-11-test-list-orders

No findings.

(Small point: the test relies on user ids 41/42 being unused because all tests share one in-memory DB. That holds today, so this is not an issue.)

# pr-12-speed-up-tests

- **blocking** — `tests/test_shop.py:13-16`: replacing `assertRaises` with `try/except Forbidden: pass` removes the assertion. The test now passes even when `get_order` returns another user's order, so the authorization check is no longer covered. The title says "speed up" or "less flaky", but this makes nothing faster or more stable; it only stops the test from failing. Revert to `with self.assertRaises(orders.Forbidden):`, or add `self.fail(...)` after the call. If there is real flakiness, find its root cause (for example, the shared in-memory DB).