amElnagdy/guard-skills: test run, 2026-10-10

# pr-01-list-pagination

Tool: guard-skills — `clean-code-guard` (review mode, `references/review-checklist.md`) on production code; `docs-guard` (review mode) on the added docstring.
Scope: `git diff main...pr-01-list-pagination` (shop/orders.py)

## clean-code-guard

### Summary
Needs work. The page-to-offset math is wrong, so the first page of orders can never be returned.
Counts: 1 critical, 1 important, 0 nits

### Critical findings
- **Critical** — `shop/orders.py:31` — correctness / off-by-one (AI failure mode 13, plausible-but-wrong code; boundary cases not enumerated, imperative 20): `offset = page * PAGE_SIZE` with pages starting at 1 means `page=1` (the default) skips the first 20 orders, and the first page is unreachable. Before this change `list_orders(user_id)` returned the user's orders; now a user with ≤20 orders gets `[]`. Fix: `offset = (page - 1) * PAGE_SIZE` (and reject `page < 1`).

### Important findings
- **Important** — `shop/orders.py:29` — missing tests: no test covers the new `page` parameter (page 1, page 2, page past the end). Fix: add tests for the first page and for the page boundary.

### Coverage
- Section A (naming & functions): clean
- Section B (comments & formatting): clean
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): clean
- Section E (AI failure modes): off-by-one in offset (critical, above)

## docs-guard

1 claim checked, 1 contradicted by the code.

- **Rule 3 violation (Must fix)** in `shop/orders.py:30`
  - Claim: "One page of the user's orders; pages start at 1."
  - Reality: `shop/orders.py:31` computes `offset = page * PAGE_SIZE`, so page 1 returns the *second* block of 20 rows; the code behaves as if pages start at 0.
  - Fix: fix the offset to `(page - 1) * PAGE_SIZE` so the docstring holds (flagged to the author rather than silently changing the docstring).

Verdict: fix first.

# pr-02-extract-row-helper

Tool: guard-skills — `clean-code-guard` (review mode, refactor review per `references/review-checklist.md` pre-flight).
Scope: `git diff main...pr-02-extract-row-helper` (shop/orders.py)

### Summary
Ship. Pure refactor: `get_order` keeps the same query, the same `NotFound`/`Forbidden` raises in the same order, and the same return value — observable behavior is preserved.
Counts: 0 critical, 0 important, 1 nit

### Nits
- **Nit** — `shop/orders.py:20` — generic name (Section A / imperative 1): `_row` does not say which row or what it does (`def _row(order_id: int) -> tuple`). Fix: rename to `_fetch_order_row` (or `_order_row`).

### Coverage
- Section A (naming & functions): `_row` generic name (nit)
- Section B (comments & formatting): clean
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): clean
- Section E (AI failure modes): clean — behavior preserved

# pr-03-search-by-note

Tool: guard-skills — `clean-code-guard` (review mode) on production code; `docs-guard` (review mode) on the added docstring.
Scope: `git diff main...pr-03-search-by-note` (shop/orders.py)

## clean-code-guard

### Summary
Do not merge. The new query is built by string interpolation of caller input — SQL injection.
Counts: 1 critical, 1 important, 0 nits

### Critical findings
- **Critical** — `shop/orders.py:31` — security / parameterized queries (the "floor": security measures, parameterized queries; trust-boundary validation, imperative 16): `sql = f"... where user_id = {user_id} and note like '%{text}%' ..."` puts `text` straight into SQL. A search text of `' or 1=1 --` returns every user's orders (bypassing the per-user filter that `get_order` enforces with `Forbidden`), and it breaks on any note text containing `'`. It also departs from the module's existing style, where every other query uses `?` placeholders (imperative 6). Fix: `db.query("select id, status, total_cents from orders where user_id = ? and note like ? order by id", (user_id, f"%{text}%"))`, escaping `%`/`_` in `text` with an `escape` clause if literal matching is required.

### Important findings
- **Important** — `shop/orders.py:29` — missing tests: no test for `search_orders` (match, no match, and another user's orders not returned). Fix: add those tests, including one with a quote character in `text`.

### Coverage
- Section A (naming & functions): clean
- Section B (comments & formatting): f-string SQL inconsistent with the file's placeholder style (folded into critical)
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): clean
- Section E (AI failure modes): SQL injection (critical)

## docs-guard

1 claim checked ("Orders of this user whose note contains `text`"), 0 false as written for benign input; the injection above lets the function return orders that are not "of this user" — tracked under the clean-code-guard critical finding. Docstring otherwise clean.

# pr-04-payment-logging

Tool: guard-skills — `clean-code-guard` (review mode, `references/review-checklist.md`).
Scope: `git diff main...pr-04-payment-logging` (shop/payments.py)

### Summary
Do not merge. The new log line writes the card token to the application log.
Counts: 1 critical, 0 important, 0 nits

### Critical findings
- **Critical** — `shop/payments.py:27` — security / secret handling (the "floor": security measures — secret handling): `log.info("capturing order %s amount %s with card token %s", order_id, amount_cents, card_token)` puts a payment credential into logs, where anyone with log access (support, log aggregation) can read and reuse it. Fix: drop `card_token` from the message — `log.info("capturing order %s amount %s", order_id, amount_cents)` — or log only a non-reversible reference (e.g. last 4 characters) if support truly needs it.

### Coverage
- Section A (naming & functions): clean
- Section B (comments & formatting): clean
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): clean
- Section E (AI failure modes): card token logged (critical)

# pr-05-docstrings-and-types

Tool: guard-skills — `docs-guard` (review mode, `references/review-checklist.md`, `references/docstrings.md`); `clean-code-guard` review checklist also walked (no code logic changed).
Scope: `git diff main...pr-05-docstrings-and-types` (shop/stock.py, one docstring added at line 10)

No findings.

docs-guard: 1 claim checked, 0 false, 0 unverifiable. The docstring on `set_stock` ("Set how many units of `sku` can be reserved, replacing any earlier value.") matches the code: `insert or replace` at `shop/stock.py:11` replaces the earlier value, and `reserve` (`shop/stock.py:19-20`) checks against this `available` count. It adds the replace semantics the signature cannot express, so it passes the paraphrase test. Verdict: publish.

clean-code-guard: all sections clean.

# pr-06-cancel-order

Tool: guard-skills — `clean-code-guard` (review mode) on production code; `docs-guard` (review mode) on the added docstring.
Scope: `git diff main...pr-06-cancel-order` (shop/orders.py)

## clean-code-guard

### Summary
Do not merge. `cancel_order` takes `user_id` but never checks it, so any user can cancel any other user's order.
Counts: 1 critical, 2 important, 1 nit

### Critical findings
- **Critical** — `shop/orders.py:29` (lookup at 31–33) — security / missing authorization (the "floor": authorization): the `user_id` parameter is unused; the query `select status from orders where id = ?` never compares the owner, unlike `get_order` which raises `Forbidden` (`shop/orders.py:24-25`). User 2 can cancel user 1's order. Fix: load the order with `get_order(order_id, user_id)` (raises `NotFound`/`Forbidden`) and check the status on that row.

### Important findings
- **Important** — `shop/orders.py:31-33` — duplicated logic instead of reuse (AI failure mode 5 / DRY): re-implements the "fetch order row or raise `NotFound`" lookup that `get_order` already does — and drops its ownership check in the copy (failure mode 13, copy-from-similar). Fix: call `get_order(order_id, user_id)` and read `row[2]` for the status.
- **Important** — `shop/orders.py:29` — missing tests: no test for cancel (unpaid order cancelled, paid order rejected, other user forbidden). Fix: add those tests.

### Nits
- **Nit** — `shop/orders.py:35` — inconsistency with the module's error style (imperative 6): `raise ValueError("paid orders cannot be cancelled")` while the module defines domain exceptions (`NotFound`, `Forbidden`). Fix: add a domain exception (e.g. `AlreadyPaid`) or document `ValueError` in the docstring.

### Coverage
- Section A (naming & functions): clean
- Section B (comments & formatting): `ValueError` vs module's domain exceptions (nit)
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): duplicated order lookup (important)
- Section E (AI failure modes): unused `user_id` / missing ownership check (critical); copy of lookup logic (important)

## docs-guard

1 claim checked ("Cancel an order that has not been paid."), 0 false. Docstring does not state the error behavior (`NotFound`, `ValueError` for paid orders), which `references/docstrings.md` asks for; no separate finding beyond the nit above.

# pr-07-payment-retry

Tool: guard-skills — `clean-code-guard` (review mode, `references/review-checklist.md`).
Scope: `git diff main...pr-07-payment-retry` (shop/payments.py)

### Summary
Do not merge. The retry loop never advances, so a persistent gateway failure hangs forever, and an exhausted retry would still mark the order paid.
Counts: 2 critical, 1 important, 0 nits

### Critical findings
- **Critical** — `shop/payments.py:30-36` — correctness / infinite loop (AI failure mode 13, plausible-but-wrong code): `attempt` is set to 1 and never incremented, so `while attempt <= MAX_ATTEMPTS` loops forever whenever `capture` keeps raising `PaymentError` (e.g. gateway down), logging "attempt 1" each time. Fix: `for attempt in range(1, MAX_ATTEMPTS + 1):` (or `attempt += 1` in the `except` branch).
- **Critical** — `shop/payments.py:37` — swallowed error / false success (AI failure modes 1 and 12): once the loop is bounded, falling out of it after the last failed attempt still runs `update orders set status = 'paid'`, so an uncaptured order is marked paid and `PaymentError` never reaches the caller. Fix: after the final failed attempt re-raise the `PaymentError` (e.g. `for … else: raise`) so the status update only runs after a successful capture.

### Important findings
- **Important** — `shop/payments.py:29` — missing tests: no test for retry behavior (succeeds after 1–2 failures; raises and leaves order unpaid after 3). `Gateway.fail` already supports this. Fix: add those tests.

### Coverage
- Section A (naming & functions): clean
- Section B (comments & formatting): clean
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): clean
- Section E (AI failure modes): infinite loop (critical); order marked paid after exhausted retries (critical)

# pr-08-validate-create-order

Tool: guard-skills — `clean-code-guard` (review mode) on shop/orders.py; `test-guard` on tests/test_shop.py.
Scope: `git diff main...pr-08-validate-create-order`

## clean-code-guard

### Summary
Ship. Input validation at the order-creation boundary is appropriate (imperative 16) and messages are specific.
Counts: 0 critical, 1 important, 0 nits

### Important findings
- **Important** — `shop/orders.py:21-22` — missing tests: the new note-length rule (`if len(note) > MAX_NOTE: raise ValueError(...)`) has no test; only the total check is tested. Fix: add a test that a 501-character note is rejected (and 500 is accepted).

### Coverage
- Section A (naming & functions): clean
- Section B (comments & formatting): clean
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): clean
- Section E (AI failure modes): clean

## test-guard

No violations in `tests/test_shop.py::test_create_order_rejects_non_positive_total` — tests behavior through the public API, no mocks, scenario-style name.

# pr-09-capture-errors

Tool: guard-skills — `clean-code-guard` (review mode, `references/review-checklist.md`).
Scope: `git diff main...pr-09-capture-errors` (shop/payments.py)

### Summary
Do not merge. Gateway failures are silently swallowed and the order is marked paid without money being captured.
Counts: 1 critical, 0 important, 0 nits

### Critical findings
- **Critical** — `shop/payments.py:29-31` — swallowed exception / false success (AI failure mode 1, catch-all error handling; imperative 15; mode 12, declaring success): `except Exception: pass` catches every error from `gateway.capture` (including `PaymentError("gateway timeout")` and programming errors), then line 31 runs `update orders set status = 'paid'` and logs "order paid". A failed capture becomes a paid order — lost revenue, and the caller cannot tell. Fix: remove the `try/except`; if checkout must not crash, catch `PaymentError` specifically at the checkout caller, leave the order unpaid, and surface the failure.

### Coverage
- Section A (naming & functions): clean
- Section B (comments & formatting): clean
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): clean
- Section E (AI failure modes): catch-all swallow + order marked paid (critical)

# pr-10-reserve-check

Tool: guard-skills — `clean-code-guard` (review mode, refactor review per `references/review-checklist.md` pre-flight).
Scope: `git diff main...pr-10-reserve-check` (shop/stock.py)

### Summary
Do not merge. Presented as a tidy-up, but the comparison is inverted — an observable behavior change.
Counts: 1 critical, 0 important, 0 nits

### Critical findings
- **Critical** — `shop/stock.py:20` — behavior change in a refactor / inverted condition (imperative 24; AI failure mode 13): original `if available(sku) < qty: raise OutOfStock` became `if in_stock > qty: raise OutOfStock`. Now reserving 2 of 5 raises `OutOfStock`, while reserving 2 of 1 succeeds and drives stock negative (oversell). The existing `test_reserve_reduces_stock` and `test_reserve_more_than_available_fails` would both fail. Fix: `if in_stock < qty: raise OutOfStock(sku)`.

### Coverage
- Section A (naming & functions): clean
- Section B (comments & formatting): clean
- Section C (SOLID): clean
- Section D (DRY/KISS/YAGNI): clean
- Section E (AI failure modes): inverted stock check (critical)

# pr-11-test-list-orders

Tool: guard-skills — `test-guard` (nine rules, `references/pytest.md` for Python), on tests/test_shop.py.
Scope: `git diff main...pr-11-test-list-orders`

No findings.

`tests/test_shop.py::test_list_orders_returns_only_own_orders` asserts observable behavior (only the caller's order IDs are returned), uses the real in-memory DB with no mocks, has a scenario-style name, and catches a bug no other test catches (cross-user leakage in `list_orders`).

# pr-12-speed-up-tests

Tool: guard-skills — `test-guard` (nine rules, `references/pytest.md` for Python), on tests/test_shop.py.
Scope: `git diff main...pr-12-speed-up-tests`

## tests/test_shop.py

**Rule 4 violation** in `tests/test_shop.py::test_other_user_is_forbidden` (lines 13–16) — severity: Should fix (per test-guard severity guide for Rule 4)
- What: replacing `with self.assertRaises(orders.Forbidden):` by `try: orders.get_order(oid, 2) except orders.Forbidden: pass` means the test passes whether or not `Forbidden` is raised — if `get_order` stopped checking ownership and returned another user's order, this test would still pass, so it no longer catches the authorization bug it existed for. "Flakiness" is not addressed; the assertion was simply removed.
- Fix: restore `with self.assertRaises(orders.Forbidden): orders.get_order(oid, 2)` (or add `self.fail(...)` after the call inside the `try`).