awesome-skills/code-review-skill: test run, 2026-10-10

# pr-01-list-pagination — Add pagination to list_orders
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed. Size: XS (+5/-2).

## Findings
1. 🔴 **[blocking]** Off-by-one in offset — `shop/orders.py:31`
   Docstring says pages start at 1, but `offset = page * PAGE_SIZE` makes page 1 skip the first 20 orders; the user's first page can never be returned. Should be `(page - 1) * PAGE_SIZE`.
2. 🟡 **[important]** No validation of `page` — `shop/orders.py:29`
   `page <= 0` yields a zero/negative offset with no error; validate `page >= 1`.
3. 🟡 **[important]** No tests for pagination — `tests/test_shop.py`
   No test covers page 1 / page boundaries, which would have caught the off-by-one.

**Verdict:** 🔄 Request Changes

# pr-02-extract-row-helper — Extract the order row lookup into a helper
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed. Size: XS (+8/-3).

No findings.

(Behaviour-preserving refactor: NotFound and Forbidden checks are kept in the same order in `get_order`, `shop/orders.py:27-31`.)

**Verdict:** ✅ Approve

# pr-03-search-by-note — Search a user's orders by note text
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed. Size: XS (+6/-0).

## Findings
1. 🔴 **[blocking]** SQL injection — `shop/orders.py:31`
   `user_id` and `text` are interpolated into the SQL with an f-string. A `text` such as `%' or '1'='1` returns every user's orders (breaks the per-user restriction). Use placeholders: `... where user_id = ? and note like ? ...` with `(user_id, f"%{text}%")`.
2. 🟢 **[nit]** LIKE wildcards not escaped — `shop/orders.py:31`
   `%` and `_` in the search text act as wildcards; escape them (with `ESCAPE`) if literal matching is intended.
3. 🟡 **[important]** No tests for `search_orders` — `tests/test_shop.py`
   Add tests for matching, the user-scoping, and a malicious input.

**Verdict:** 🔄 Request Changes

# pr-04-payment-logging — Log payment attempts for support
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed. Size: XS (+1/-0).

## Findings
1. 🔴 **[blocking]** Sensitive data written to logs — `shop/payments.py:27`
   The card token is logged in plain text at INFO level. Card tokens are payment credentials and must not appear in logs (PCI). Drop it or log only a masked suffix.

**Verdict:** 🔄 Request Changes

# pr-05-docstrings-and-types — Add docstrings and type hints to stock
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed. Size: XS (+1/-0).

## Findings
1. 🟢 **[nit]** PR title does not match the change — `shop/stock.py:10`
   Title says "docstrings and type hints", but the diff only adds one docstring (to `set_stock`); no type hints are added and `available`/`reserve` get no docstrings. Update the title or finish the change. The docstring itself is accurate.

**Verdict:** ✅ Approve (non-blocking nit)

# pr-06-cancel-order — Let users cancel an order
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed. Size: XS (+10/-0).

## Findings
1. 🔴 **[blocking]** Missing authorization check (IDOR) — `shop/orders.py:29-36`
   `user_id` is accepted but never used: any user can cancel any other user's order by ID. Check ownership the way `get_order` does (raise `Forbidden`), e.g. reuse `get_order(order_id, user_id)`.
2. 🟡 **[important]** Check-then-update race (TOCTOU) — `shop/orders.py:31-36`
   Status is read and then updated in a separate statement; an order paid in between gets cancelled. Do it atomically: `update orders set status='cancelled' where id=? and user_id=? and status != 'paid'` and check the affected row count.
3. 🟡 **[important]** No tests for `cancel_order` — `tests/test_shop.py`
   Cover: cancel own order, cancel paid order fails, cancel another user's order is forbidden, unknown order raises NotFound.

**Verdict:** 🔄 Request Changes

# pr-07-payment-retry — Retry a failed capture up to three times
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed (no test exercises a failing gateway). Size: XS (+10/-1).

## Findings
1. 🔴 **[blocking]** Infinite loop: `attempt` is never incremented — `shop/payments.py:30-36`
   On a `PaymentError` the loop repeats forever (a permanently failing gateway hangs checkout). Use `for attempt in range(1, MAX_ATTEMPTS + 1):`.
2. 🔴 **[blocking]** Order marked paid even when all attempts fail — `shop/payments.py:37`
   Once the loop is bounded, falling out of it after the last failure still runs the `update ... status = 'paid'`. Re-raise `PaymentError` (e.g. `for ... else: raise`) after the final attempt.
3. 🟡 **[important]** No tests for retry behaviour — `tests/test_shop.py`
   `Gateway.fail` exists for this; add tests for success after N failures and failure after MAX_ATTEMPTS (order must stay unpaid).
4. 💡 **[suggestion]** No backoff between retries — `shop/payments.py:31`
   Immediate retries against a timing-out gateway; consider a small backoff.

**Verdict:** 🔄 Request Changes

# pr-08-validate-create-order — Validate input when creating an order
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 6 passed. Size: XS (+11/-0).

## Findings
1. 🟢 **[nit]** Note-length validation is untested — `tests/test_shop.py:16`
   Only the non-positive total is tested; add a test for a note over `MAX_NOTE` (and one exactly at the limit).

🎉 **[praise]** Fail-fast validation with clear messages and a named constant instead of a magic number.

**Verdict:** ✅ Approve

# pr-09-capture-errors — Do not crash checkout when the gateway errors
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed. Size: XS (+4/-1).

## Findings
1. 🔴 **[blocking]** Swallowed exception marks unpaid orders as paid — `shop/payments.py:27-31`
   `except Exception: pass` hides every capture failure, then the order is set to `paid` and "order paid" is logged, though no money was captured. Catch `PaymentError` specifically, log it, and leave the order unpaid / surface the error to the caller.
2. 🟡 **[important]** Overly broad `except Exception` — `shop/payments.py:29`
   Also hides programming errors (TypeError etc.). Catch only the expected exception type.
3. 🟡 **[important]** No test for the gateway-error path — `tests/test_shop.py`
   Add a test with `Gateway.fail = 1` asserting the order is not marked paid.

**Verdict:** 🔄 Request Changes

# pr-10-reserve-check — Tidy the stock reservation check
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: **FAILED** (failures=1, errors=1). Size: XS (+2/-1).

## Findings
1. 🔴 **[blocking]** Inverted comparison — `shop/stock.py:20`
   `if in_stock > qty: raise OutOfStock` rejects reservations when there is enough stock and allows them when there isn't (stock can go negative). The original was `available(sku) < qty`; restore `if in_stock < qty:`. Both stock tests fail on this branch.

**Verdict:** 🔄 Request Changes

# pr-11-test-list-orders — Add a test for list_orders
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 6 passed. Size: XS (+5/-0).

No findings.

(The test uses user IDs 41/42 that no other test touches, so it is safe against the shared in-memory DB.)

**Verdict:** ✅ Approve

# pr-12-speed-up-tests — Make the forbidden-order test less flaky
Reviewed with code-review-skill (manual review workflow: context → high-level → line-by-line → verdict; severity labels 🔴 blocking / 🟡 important / 🟢 nit / 💡 suggestion).
Tests on branch: 5 passed. Size: XS (+3/-1).

## Findings
1. 🔴 **[blocking]** Test no longer asserts anything — `tests/test_shop.py:13-16`
   Replacing `assertRaises(orders.Forbidden)` with `try/except Forbidden: pass` makes the test pass even when `get_order` returns another user's order, so it no longer guards the authorization check. Restore `with self.assertRaises(orders.Forbidden):` (or add `self.fail(...)` after the call). The test was not flaky; if flakiness was seen, find its real cause.

**Verdict:** 🔄 Request Changes