# The defect map

**Spoilers.** Make your own list first — exercise 1.1 in
`checkpoints/phase-1.md` is exactly that. Then come back and compare.

Every defect below was put in on purpose. Each row names where it lives, the
lesson that teaches the fix, and the exercise that removes it. Some defects are
worth fixing immediately; some are worth naming and leaving, and the checkpoint
says which.

## Mixed domain and infrastructure concerns

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D01 | `policy.check_eligibility` | The eligibility rule reads the order system, queries SQLite and calls `datetime.now()`. You cannot test the rule without a database. | 0001, 0009 | 1.1, 1.7 |
| D02 | `policy.py`, `service.py` | `order["delivery"]["country"]`, `order["customer"]["email"]`, `order["lines"][0]["final_sale"]` — the upstream team's document structure, navigated in our rules. | 0003 | 1.2 |
| D03 | `policy.check_eligibility` | The refund amount is an `if/elif` chain over `reason` with a tier special-case buried in one branch: a strategy that has not been named. | 0004 | 1.3 |
| D05 | `ReturnsService` | Eight public methods over authorisation, persistence, eligibility, fraud, carrier, payments and notification. At least seven reasons to change. | 0006 | 1.5 |
| D22 | `service.py` | Imports ten sibling modules. Every one is a reason for this file to be recompiled, re-reviewed and re-broken. | 0022 | 2.9, 2.12 |
| D35 | `policy`, `fraud`, `service`, `notifications`, `agent_review` | `datetime.now()` called directly in five modules. There is no clock to inject, so no test can control time. | 0023 | 2.10 |
| D37 | `service.py` | The workflow calls `notifications.notify` directly, seven times. The domain knows there is an email gateway. | 0025 | 2.11, 2.12 |

## Global state and unclear ownership

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D13 | `config.CURRENT_USER` | A process-wide singleton, mutated by `api.handle` on every request and never restored. Two concurrent requests share one identity. | 0019, 0020 | 2.6 |
| D14 | `config.SETTINGS` | A module-level mutable dict imported by seven modules. Changing the restocking fee at runtime changes what every in-flight return is worth. | 0019 | 2.5 |
| D15 | everywhere | `ORDER_SYSTEM`, `SHIPMASTER`, `PAYPORTAL`, `SERVICE`, `MANAGER`, `_CONN`, `_SEQ`, `_QUEUE`, `OUTBOX`, `HANDLERS`, `COUNTERS`, `_FORMAT_CACHE`. Twelve module-level objects; `tests/support.reset_world()` is the size of the hidden state. | 0019, 0020 | 2.5, 2.6 |
| D38 | `config.SETTINGS["restocking_fee_rate"]` | One number, no owner, no audit, changes the money in every subsequent refund. | 0029 | 3.2 |

## Leaking repository and vendor contracts

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D07 | `service.issue_label` | `ShipMasterLabelResponse` crosses into the workflow, and `label.trk` / `label.pdf_href` — the vendor's abbreviations — are assigned straight onto our return. | 0008, 0009, 0023 | 1.7, 3.1 |
| D11 | `db.query`, `models.ReturnRequest.from_row` | `sqlite3.Row` objects are handed to callers and the model knows the column names. The storage schema is the domain's interface. | 0013 | 1.11 |
| D30 | `vendor_shipmaster.SUPPORTED_COUNTRIES` | Which countries the business serves is decided by which countries the vendor serves. Swapping carriers changes the product. | 0027 | 3.1 |
| D34 | `reporting.py` | Builds its dictionaries from column names, so the schema is also finance's interface. | 0013, 0058 | 1.11, 3.5 |

## Weak domain invariants

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D08 | `models.ReturnRequest.refund_amount` | Money as `float`. `int(76.49 * 100)` is `7648`, so the customer is refunded one cent less than the ledger records. Pinned by `test_a_76_49_refund_reaches_the_vendor_as_7648_minor_units`. | 0011, 0012 | 1.8 |
| D09 | `models.KNOWN_STATUSES` | Status is a free string. Nine values, two of them the same state spelled two ways (`APPROVED` from the automatic path, `approved` from the agent path). | 0011, 0054 | 1.9, 5.1 |
| D12 | `models.ReturnRequest` | A negative refund, an unknown status, and a jump from `refunded` back to `submitted` are all constructible and persistable. | 0014, 0011 | 1.9, 1.12 |
| D26 | `agent_review.open_case` | `if opened_today() > cap` — the daily cap of 50 lets 51 through. | 0014 | 1.12 |
| D27 | `fraud.assess` | Counts every row in `returns`, including ones the system denied. Three denials that cost nothing send the customer's next legitimate return to a human. | 0001, 0051 | 1.1, 5.1 |
| D28 | `service.approve` | Approving a held return does not close its review case. The queue only grows. | 0054 | 5.1 |
| D21 | `db.py` | The connection is in autocommit; nothing wraps the multi-step `submit`. A crash between the fraud hold and the review-case insert leaves a return under review with nobody looking at it. | 0055 | 5.2 |

## Synchronous and asynchronous workflows

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D19 | `notifications._deliver` | The retry unit is the batch, not the channel. Email is sent, SMS fails, the whole item is requeued — and the customer gets the same email twice. Pinned by `test_an_sms_failure_makes_the_customer_receive_two_emails`. | 0024, 0031, 0032 | 3.3 |
| D19b | `notifications._deliver` | The row is marked `sent` before the transport is called, so the table says "sent" for messages that never left. | 0021, 0049 | 3.3 |
| D19c | `notifications` | The queue is a `queue.Queue` in memory. A restart loses everything queued, and there is no dead-letter table for the messages that exhaust their retries. | 0031 | 3.3 |
| D20 | `service.issue_refund` | No idempotency key. Calling `returns.refund` twice creates two refunds at the vendor. Pinned by `test_calling_refund_twice_moves_the_money_twice`. | 0032 | 3.4 |
| D29 | `service.issue_label` | A carrier outage is caught, counted and dropped. The return is stranded in `APPROVED` with no tracking, the caller gets a 201, and nothing will ever retry. | 0021, 0031 | 3.1, 3.3 |

## A compatibility-sensitive API

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D23 | `api.serialize` | The response shape is a dict literal in a handler. It is unversioned, and three teams parse it — including one that branches on the raw status string, so renaming `APPROVED` is a breaking change to somebody else's release. | 0033, 0057 | 3.6 |
| D23b | `api.handle` | The order system's payload is read strictly: a missing optional key raises. We break when they add or move a field. | 0033 | 3.7 |
| D25 | `service.submit` | Five calls to the order system for one submit, all for the same order. Chatty coupling to a system that can be down. | 0034 | 3.2 |

## A data migration requiring expand/contract

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D24 | `migrations.SCHEMA_V1` | `refund_amount REAL` holds dollars. Fixing D08 means moving to minor units — but `reporting.py` runs on a schedule you do not control and cannot be deployed with you. Four steps, not one. | 0035, 0058 | 3.5 |
| D36 | `migrations.SCHEMA_V1` | `notifications` has no unique key, and `_deliver` updates by `(return_id, template)`, so a second send silently rewrites the first row's history. | 0058 | 3.5, 5.5 |

## Missing observability and unsafe error handling

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D16 | `fraud.assess`, `service.issue_label`, `service.issue_refund`, `api.submit_return`, `ai_helpers.parse_timestamp` | Five `except` blocks that neither re-raise nor log. `config.COUNTERS["errors_swallowed"]` is the entire incident record. | 0014, 0021 | 2.7, 2.8 |
| D17 | the whole package | Nothing imports `logging`. There is no log line anywhere in the system. | 0021, 0049 | 2.8, 4.1 |
| D18 | `api.py`, `db.audit` | No correlation id on the request, the response, the audit row or the notification. You cannot follow one customer's return through the system. | 0049 | 4.1 |
| D18b | `config.COUNTERS` | Seven counters, incremented, never read, never reset, and none of them measures the initiative's actual outcome (time from submission to resolution). | 0049, 0050 | 4.2 |

## AI-generated code with plausible structural defects

`returns_lab/ai_helpers.py` carries one comment: `# generated by an assistant,
reviewed: no`. Nothing else marks it.

| id | Where | What | Lesson | Exercise |
|---|---|---|---|---|
| D04 | file header | It was never reviewed, and it is imported by both `api.py` and `service.py`. | 0005, 0010 | 1.4 |
| D31 | `_FORMAT_CACHE` | A module-level cache keyed by return id with no invalidation and no bound. A summary rendered while the return was `submitted` still says `submitted` after it is refunded — and a *different* `ReturnManager` instance sees the first one's entry. | 0005 | 1.4 |
| D32 | `get_display_status`, `build_timeline`, `normalize_reason` | Three dead branches: a `return "Unknown"` after an if/elif chain where every arm returns; an `elif` whose condition is a strict subset of the branch above it; and a `reason.upper() in VALID_REASONS` test on a string that was lowercased two lines earlier. | 0005 | 1.4 |
| D33 | `ReturnManager` | Named for management, does formatting: four methods, all string building, one of them (`format_status_line`) an alias for another. `self.last_error` is declared and never assigned. | 0005, 0006 | 1.4 |
| D10 | `validate_return_payload` vs `api.submit_return` | Two validators with different rules. The API rejects `reason="broken"`; the service maps it to `damaged`. The helper knows about `quantity`; nothing else does. | 0028, 0006 | 1.10, 3.2 |

## Defects worth naming and leaving

Not everything on this list should be fixed. Judgement includes knowing what to
leave alone, and the checkpoints say so where it applies:

- **D25 (five order-system calls per submit)** costs nothing today: the order
  system is fast and the volume is low. Fix it when a driver says to — for
  example when the order system's p99 becomes your p99. Name it in the debt
  ledger (exercise 3.8) rather than optimising on instinct.
- **D38 (the restocking-fee setting)** is a real product control. It should get
  an owner and an audit trail, not deletion.
- **`agent_review` as a separate component** is arguably over-separated for one
  team. Exercise 5.3 asks you to argue both ways.