nathankim0/clean-architecture-skills: test run, 2026-10-10

# pr-01-list-pagination

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-01-list-pagination`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | High | kent-beck-style (Rule 1: code must work correctly) | shop/orders.py:31 | `offset = page * PAGE_SIZE` skips the first page. Pages are documented as starting at 1, so `page=1` (the default) returns rows 20–39. Any existing caller of `list_orders(user_id)` with 20 or fewer orders now gets an empty list. Use `(page - 1) * PAGE_SIZE`. |
| 2 | Medium | kent-beck-style (Rule 1: passes the tests) | shop/orders.py:29 | The new pagination behaviour has no test. A test that lists page 1 would have caught finding 1. |
| 3 | Long-term | clean-architecture (2-2: what data crosses boundaries) | shop/orders.py:32 | `list_orders` returns raw database row tuples straight from `db.query`. Database rows are crossing the boundary instead of a DTO. This was already true before the PR and is now extended to paged results. |

## Kent Beck Style Code Review Report
- Overall Code Health: Needs Improvement
- Positive: it reuses the existing `PAGE_SIZE` constant (no magic number) and keeps the query parameterized.

## Clean Architecture Review Report
- Overall Assessment: Good. The PR adds no new dependency-rule violations. The only item is finding 3, which is long-term.

# pr-02-extract-row-helper

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-02-extract-row-helper`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | Low | kent-beck-style (Rename Method / intention-revealing names) | shop/orders.py:20 | `_row` does not say what it does. Rename it to something like `_fetch_order_row`. |
| 2 | Low | kent-beck-style (Primitive Obsession / magic index) | shop/orders.py:29 | `row[1] != user_id` uses a positional tuple index to mean "owner". Name the field, for example with a namedtuple/dataclass or a `OWNER_COLUMN` constant. |

## Kent Beck Style Code Review Report
- Overall Code Health: Good
- Positive: a clean Extract Method that preserves behaviour (NotFound/Forbidden semantics unchanged; tests pass).

## Clean Architecture Review Report
- Overall Assessment: Good. No findings.

# pr-03-search-by-note

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-03-search-by-note`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | High | kent-beck-style (Rule 1: code must work correctly) | shop/orders.py:31 | The SQL is built with an f-string that interpolates `user_id` and `text`. Any note search containing `'` breaks the query, and a crafted `text` (e.g. `%' or 1=1 --`) returns other users' orders (SQL injection). Use `?` placeholders as the rest of the module does: `... where user_id = ? and note like ? ...`, with `(user_id, f"%{text}%")`. |
| 2 | Medium | kent-beck-style (Rule 1: passes the tests) | shop/orders.py:29 | The new `search_orders` has no test. |
| 3 | Low | kent-beck-style (Rule 1) | shop/orders.py:31 | The `%` and `_` characters in `text` are not escaped, so they act as LIKE wildcards. |
| 4 | Low | kent-beck-style (Duplicate Code) | shop/orders.py:31 | `select id, status, total_cents from orders where user_id = ... order by id` duplicates the `list_orders` query (line 36). Consider a shared column list/helper. |
| 5 | Medium | clean-architecture (Dependency Rule / DIP) | shop/orders.py:31-32 | The order use-case module now hand-builds raw SQL strings and bypasses the `db` helper's parameter mechanism, which pushes persistence details further into business logic. Long-term, put order queries behind a repository function. |

## Kent Beck Style Code Review Report
- Overall Code Health: Critical (finding 1)

## Clean Architecture Review Report
- Overall Assessment: Needs Improvement (finding 5)

# pr-04-payment-logging

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-04-payment-logging`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | High | kent-beck-style (Rule 1: code must work correctly) | shop/payments.py:27 | The new log line writes `card_token` to the application log in plain text. Card tokens are payment credentials and must not end up in support logs. Log only the order id and amount, or a masked token. |

## Kent Beck Style Code Review Report
- Overall Code Health: Needs Improvement
- Positive: it uses lazy `%s` logging arguments.

## Clean Architecture Review Report
- Overall Assessment: Good. No findings.

# pr-05-docstrings-and-types

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-05-docstrings-and-types`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

No findings.

(kent-beck-style: the docstring added at shop/stock.py:10 states intent, including the "replacing any earlier value" semantics, so it is not a "comment as smell". clean-architecture: no structural change.)

# pr-06-cancel-order

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-06-cancel-order`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | High | kent-beck-style (Rule 1: code must work correctly) | shop/orders.py:29-36 | The `user_id` parameter is never used, so any user can cancel any other user's order. `get_order` enforces ownership (`Forbidden`), but `cancel_order` does not. |
| 2 | Medium | kent-beck-style (Duplicate Code) | shop/orders.py:31-33 | It reimplements the order lookup and `NotFound` handling from `get_order`. Reusing `get_order(order_id, user_id)` would remove the duplication and also add the ownership check. |
| 3 | Medium | kent-beck-style (Rule 1: passes the tests) | shop/orders.py:29 | There are no tests for cancellation: the happy path, paid orders, and another user's order. |
| 4 | Low | kent-beck-style (Primitive Obsession / magic literals) | shop/orders.py:34,36 | The status values `"paid"` and `"cancelled"` are bare string literals. Use named constants or an enum. Only `paid` is guarded, so an already-cancelled order can be "cancelled" again. |
| 5 | Low | kent-beck-style (intention-revealing code) | shop/orders.py:35 | The generic `ValueError` is used for a domain rule. A dedicated exception (like `NotFound`/`Forbidden`) would reveal intent. |
| 6 | Long-term | clean-architecture (Layer separation) | shop/orders.py:29-36 | The business rule "paid orders cannot be cancelled" is mixed with SQL in one function. Long-term, put the rule on an Order entity and keep persistence in a repository. |

## Kent Beck Style Code Review Report
- Overall Code Health: Critical (finding 1)

## Clean Architecture Review Report
- Overall Assessment: Needs Improvement (finding 6, long-term)

# pr-07-payment-retry

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-07-payment-retry`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | High | kent-beck-style (Rule 1: code must work correctly) | shop/payments.py:30-36 | `attempt` is never incremented, so `while attempt <= MAX_ATTEMPTS` never terminates when the gateway keeps failing: an infinite retry loop that hammers the gateway. |
| 2 | High | kent-beck-style (Rule 1: code must work correctly) | shop/payments.py:37 | There is no failure path after the retries. Once the loop exits (after fixing finding 1), the order is still marked `paid` even though every capture failed. Re-raise `PaymentError` when attempts are exhausted, e.g. `for attempt in range(1, MAX_ATTEMPTS + 1): ... else: raise`. |
| 3 | Medium | kent-beck-style (Rule 1: passes the tests) | shop/payments.py:29 | The retry behaviour is untested: transient failure then success, and permanent failure. `Gateway.fail` already supports both. |
| 4 | Low | kent-beck-style (KISS) | shop/payments.py:30-36 | A manual counter plus `while`/`break` is harder to read than a `for ... range` loop with `else`. |
| 5 | Long-term | clean-architecture (Layer separation) | shop/payments.py:30-36 | The retry policy is a gateway/infrastructure concern embedded in the payment use case. Consider moving it into the gateway adapter. |

## Kent Beck Style Code Review Report
- Overall Code Health: Critical
- Positive: `MAX_ATTEMPTS` is a named constant, not a magic number.

## Clean Architecture Review Report
- Overall Assessment: Good, except for the long-term item (finding 5).

# pr-08-validate-create-order

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-08-validate-create-order`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | Medium | kent-beck-style (Rule 1: passes the tests) | shop/orders.py:21-22 | The note-length validation has no test. Only the non-positive total is tested (tests/test_shop.py:16-18). |
| 2 | Low | kent-beck-style (Naming) | shop/orders.py:15 | `MAX_NOTE` does not say what is limited. Rename it to `MAX_NOTE_LENGTH`. |
| 3 | Low | kent-beck-style (Primitive Obsession) | shop/orders.py:18-20 | Money is a bare `int` (`total_cents`) validated inline. Consider a small value object (Replace Primitive with Object) so the "positive amount" rule lives in one place. |
| 4 | Long-term | clean-architecture (Entities) | shop/orders.py:19-22 | The order invariants (positive total, note length) are enforced in the use-case function next to the SQL insert. Long-term, they belong on an Order entity. |

## Kent Beck Style Code Review Report
- Overall Code Health: Good
- Positive: guard clauses, a named constant, and a test added with the change.

## Clean Architecture Review Report
- Overall Assessment: Good (finding 4 is long-term only)

# pr-09-capture-errors

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-09-capture-errors`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | High | kent-beck-style (Rule 1: code must work correctly) | shop/payments.py:27-31 | `except Exception: pass` swallows every capture failure. Execution then falls through to mark the order `paid` and log "order paid" although no money was captured. It also hides programming errors. Catch `PaymentError` and propagate it, or record a failed status, instead of marking the order paid. |
| 2 | Medium | kent-beck-style (Rule 1: passes the tests) | shop/payments.py:26 | There is no test for a failing gateway (`Gateway.fail = 1`). Such a test would show the order wrongly becoming `paid`. |

## Kent Beck Style Code Review Report
- Overall Code Health: Critical

## Clean Architecture Review Report
- Overall Assessment: Good. No findings.

# pr-10-reserve-check

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-10-reserve-check`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | High | kent-beck-style (Rule 1: passes the tests) | shop/stock.py:20 | The comparison is inverted: `if in_stock > qty: raise OutOfStock`. Reservations now fail whenever there is enough stock and succeed when there is not, which drives `available` negative. The existing suite fails on this branch: `python -m unittest` gives 1 failure and 1 error (test_reserve_reduces_stock, test_reserve_more_than_available_fails). It should be `if in_stock < qty`. A "tidy" refactor must preserve behaviour. |

## Kent Beck Style Code Review Report
- Overall Code Health: Critical
- Positive: introducing the explaining variable `in_stock` is fine in itself.

## Clean Architecture Review Report
- Overall Assessment: Good. No findings.

# pr-11-test-list-orders

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-11-test-list-orders`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | Low | kent-beck-style (Magic numbers / intention-revealing code) | tests/test_shop.py:17-18 | User ids `41`/`42` are magic numbers chosen to avoid colliding with the other tests in the shared module-level in-memory DB. Name them, or isolate DB state per test, so the test does not depend on other tests' data. |
| 2 | Low | kent-beck-style (Magic index) | tests/test_shop.py:19 | `row[0]` relies on tuple position to mean the order id. |

## Kent Beck Style Code Review Report
- Overall Code Health: Good
- Positive: a focused test with an intention-revealing name that covers ownership filtering.

## Clean Architecture Review Report
- Overall Assessment: Good. No findings.

# pr-12-speed-up-tests

Tool: clean-architecture-skills (~/src) — the `kent-beck-style` and `clean-architecture` review skills, applied to `git diff main...pr-12-speed-up-tests`. Priorities are the ones in each skill's report format (High / Medium / Low; Clean Architecture: High / Medium / Long-term).

## Findings

| # | Priority | Skill | Location | Finding |
|---|---|---|---|---|
| 1 | High | kent-beck-style (Rule 1: passes the tests — tests must be meaningful) | tests/test_shop.py:13-16 | Replacing `assertRaises(orders.Forbidden)` with `try/except Forbidden: pass` makes the test pass even if `get_order` returns another user's order. The ownership check is no longer tested. This does not fix flakiness; it removes the assertion. Restore `assertRaises`. |

## Kent Beck Style Code Review Report
- Overall Code Health: Critical

## Clean Architecture Review Report
- Overall Assessment: Good. No findings.