procurement-ingest/docs/refactor-evaluation.md

# procurement-ingest — Refactor Evaluation Report

**Date:** 2026-07-16 · **Branch audited:** `feature/po-derived-classifier` · **Findings:** 71 (55 confirmed by verification pass, 16 unverified) · **Suite:** 568 passed / 0 failed / 1.47s

---

## 1. Executive summary

**Verdict: a full rewrite is NOT warranted; a staged consolidation refactor IS — and one security-parity gap must land before (or as the first phase of) any restructuring.**

The core ingest path is in good shape: template-first parsers with fail-closed gates, 93% coverage on the tested tree, 0 ruff violations, clean CI. The problems are structural, not functional:

1. **Security drift between twin pipelines (fix first, not a refactor).** The AI-fallback hardening from #104 (XML-delimited prompt, tag neutralization, `validate_ai_fallback` fail-closed gate, `ai_fallback_rejected` metric + alarm) exists **only in WO**. PO's `extract_with_claude` sends the raw untrusted body bare after the prompt and routes unvalidated LLM output straight to `save_cancellation`/`save_new_po` — a DKIM-passing injected `{"email_type":"cancellation"}` sticky-cancels a live PO. This is the canonical drifted-copy failure and the strongest argument for consolidation.
2. **Duplication with no shared home.** `ses_auth.py` is a byte-identical 452-line copy (sync policed by a hand-parameterized test fixture); the web_ui auth gate is a byte-identical ~61-line block; the EMF emitter is copied 3×; ~379 identical lines sit between the two CDK stacks. The handbook's `lambdas/shared/` location is unused, blocked today by per-function `Code.from_asset` roots that can't reach it.
3. **The deploy pipeline cannot catch the refactor's main failure mode.** The hand-maintained `cp` allowlist in po_stack bundling has already caused production ImportErrors (PR #105); deploys run zero tests; cd-cdk's pre-flight/health-check steps are dormant (no stack-name passed); detection of a broken bundle is traffic-dependent. **Guards must land before any packaging change.**

**Shape:** ~11 PR-sized phases over the sequence *guards → security parity → bundling-root move → shared-code extraction → CDK dedup → handler decomposition → site_extractor reconciliation → test/doc consolidation*. Every phase is independently deployable and revertible per deploy-then-merge. Rough scope: ~2–3 weeks of focused work; phases 0–3 are the high-value 20% that eliminates the drifted-copy failure class.

**Hard constraints respected throughout:** fail-closed gates unchanged or strengthened (never weakened); shadow telemetry observe-only (no `derived_fields` behavior change until the bake concludes); ParseMethod EMF / fallback-alarm contracts preserved (with the two deliberate per-pipeline emission-ordering differences pinned by tests, not accidentally "fixed"); all CDK changes either provably logical-ID-safe or gated on a zero-change `cdk diff`.

---

## 2. Current-state assessment (measured)

### Tests
| Root | Tests |
|---|---|
| `tests/` (cross-pipeline) | 117 |
| `lambdas/wo/email_processor/tests/` | 143 |
| `lambdas/po/email_processor/tests/` | 308 |
| **Total** | **568 passed, 0 failed, 1.47s** |

- `pytest.ini` is the only config; `test_local.py` deliberately excluded (imports handler → boto3 clients at collection). CI runs bare `pytest` at repo root so all three roots are collected; no coverage step in CI.
- **Three divergent module-loading mechanisms** (tests/conftest importlib loader with sys.modules save/restore; `_po_parser_support.py` independent importlib reimplementation + load-bearing `import moto` ordering; `_wo_parser_support.py` bare sys.path `import handler`) exist solely because both pipelines duplicate bare module names.

### Coverage (pytest-cov 7.1.0, Python 3.12.13)
| Module | Coverage |
|---|---|
| po/derived_fields | 95% (296 stmts) |
| po/handler | 96% (186) |
| po/ses_auth · wo/ses_auth | 94% (223 each) |
| po/template_parser | 89% (494) |
| wo/handler | 96% (139) |
| wo/template_parser | 95% (274) |
| **Tested-tree total** | **93% (3,356 stmts)** |
| po/web_ui, wo/web_ui, po/site_extractor, scripts/ | **0% — 540 stmts, zero test references** |
| **True all-first-party coverage** | **~80% (3,896 stmts, 777 miss)** |

Gotcha: naive `--cov=lambdas` silently omits web_ui/site_extractor (missing `__init__.py`) — the 93% headline overstates reality.

### Duplication (difflib line-level, autojunk off)
| Pair | Similarity |
|---|---|
| ses_auth.py (po vs wo) | **100% — byte-identical, 452 lines** |
| web_ui/handler.py | 59.6% (212 common lines; auth block byte-identical) |
| cdk/po_stack.py vs wo_stack.py | 59.9% (379 identical lines measured) |
| email_processor/handler.py | 30.9% |
| template_parser.py | 15.4% (structurally parallel, legitimately divergent) |

### Lint
- ruff 0.15.12: `ruff check .` → 0 violations; `ruff format --check` → 135 files clean.
- **Caveat:** there is no ruff config file, so PLR/C901 are never selected — the three `noqa: PLR09xx` suppressions on `_validate_new_po_values` are inert, and `extract_new_po` (C901=35) passes lint for the same reason. "0 violations" reflects default rules only.

### Dead weight
- Untracked 44 MB `lambdas/po/email_processor/package/` — pre-Bedrock vendored anthropic SDK + stale handler. Not in any build; copied into cdk.out staging on every synth.

---

## 3. Refactor plan — phased PR sequence

Ordering principle: **guards first → byte-identical consolidation → behavior-preserving extraction → structural moves last.** Each phase deploys and verifies (deploy-then-merge per `git-workflow.md`) before the next merges. Rollback at every step: `git revert` + push (auto-redeploy ~5–10 min) or local redeploy of previous commit; dropped mail recoverable via DLQ (14 d) and S3 `inbound/` replay (90 d).

### Phase 0 — Deploy-pipeline guards (PR-0) · effort: S · risk: low
**Goal:** make a broken bundle fail the deploy job, not Monday's first email.
- Both handlers: healthcheck early-return branch (`{"healthcheck": true}` → ok) placed **before** S3 fetch and SES-auth so it neither creates an accept path nor trips the `sender_auth_rejected` substring metric-filter alarm (which pages at ≥1 reject, 2-of-6 × 5 min — two deploys in ~30 min would false-page otherwise).
- `scripts/post-deploy-smoke.sh`: **synchronous** invoke (`RequestResponse`) of both processors, checking the `FunctionError` response field (init ImportError returns HTTP 200 + `FunctionError=Unhandled` — exit-code checks false-pass). Wire via cd-cdk's `post-deploy-script` input (stack-name input takes only one stack; this repo has two).
- Replace PO's four-file `cp` allowlist with `cp ./*.py /asset-output/` (identical output today; non-recursive glob excludes tests/).
- CI-time ast consistency test: extract handler.py's first-party sibling imports, assert each ships in the bundle. Runs in the existing pytest step, before synth.
- **Gates:** deploy, smoke green, one real PO + WO email each showing `ParseMethod=template`, all alarms green. Handler touch = untrusted-input surface → **`/sh-security-review` mandatory**; new event field in handler → run cross-family review (`cross_review.py`) for the handler-contract change.

### Phase 1 — PO AI-fallback security parity (port of #104) · effort: M · risk: medium
**Goal:** close the highest-impact confirmed finding; make pipelines structurally symmetric.
- `lambdas/po/email_processor/`: add PO-specific `validate_ai_fallback` (NOT a WO copy — PO contract is nested: `CONTRACT_KEYS` exact key-set with missing-key normalization, `po_number` vs `_PO_ID_RE` (protects the DynamoDB partition key built at handler.py:523/621), `email_type` ∈ {new_po, revision, cancellation}, Decimal/int/None money types since PO parses with `parse_float=Decimal`). Call immediately after `extract_with_claude`, before `enrich_parsed`/dispatch; on failure emit `ParseMethod=ai_fallback_rejected` then `continue` (skip, never raise — avoids DLQ churn on attacker-controlled input).
- Port `<email>` data-block wrapping + `_EMAIL_TAG_RE` tag neutralization + `temperature=0` into `extract_with_claude`.
- **Preserve the deliberate PO metric ordering:** PO emits `ai_fallback` BEFORE the Bedrock call (so Bedrock-side errors still record the outcome — handler.py:656-669). Do not move it. `ai_fallback_rejected` is an additive second datapoint on rejection; document the intentional double-count.
- `cdk/po_stack.py` in the same PR: (a) fold `FILL(rej,0)` into the existing fallback-rate MathExpression numerator+denominator **inside** the IF volume floor (in-place property update to the existing alarm logical ID — safe; respect the post-#102 no-element-wise-MAX rule at po_stack.py:416-418); **exclude the rejected series from the rate numerator or account for the pre-call double-count** — do not copy WO's fb+rej math verbatim, it double-counts PO rejections. (b) net-new `EmailProcessorAiFallbackRejectedAlarm` — **retune for ~57 emails/day** (WO's 5-min/30-min sparse idiom is structurally dead at PO volume; use 1h–6h periods like the existing PO fallback-rate retune).
- Tests (mirroring WO one-for-one): non-dict model output, injected po_number (`123#x`, fullwidth digits), injected email_type (assert no save_* AND no misroute into `save_new_po` via the else branch), injected status, XML-wrap, forged-tag neutralization, linear-time regex.
- README: fix the false "Data is never corrupted" PO claim (line 29) and add `ai_fallback_rejected` to the PO ParseMethod list (line 145).
- **Gates:** `/sh-security-review` (untrusted-input, mandatory), full suite, `npx cdk synth po-ingest`, deploy-then-merge with live-email verification of both metric series.

### Phase 2 — Bundling-root move, no code move (PR-1 of migration) · effort: S · risk: medium (deploy-mechanics only)
**Goal:** make `lambdas/shared/` reachable before anything moves into it.
- Both stacks: `Code.from_asset("../lambdas")` with `command: cp po/email_processor/*.py /asset-output/` (resp. wo) and `pip install -r po/email_processor/requirements.txt` (the `-r` path change is required — bundling cwd is the asset root). Add `exclude=['**/__pycache__/**','**/tests/**','**/package/**']` — `from_asset` does NOT honor .gitignore, and the stale 44 MB `package/` dir would otherwise diverge local vs CI asset hashes.
- WO note: its current `cp -r .` ships tests/ (real scrubbed .eml fixtures), `__pycache__`, and requirements.txt in the prod zip. Accept and document the file-list shrinkage as deliberate cleanup; verification criterion for WO is "runtime-imported module set unchanged + smoke", byte-identical zip diff applies to PO only.
- Also add `exclude` to the three plain `from_asset` calls (po web_ui :502, site_extractor :581, wo web_ui :548) — local `__pycache__` currently makes their hashes nondeterministic.
- **Gates:** `aws lambda get-function` zip file-list diff (PO identical; WO shrinkage reviewed), smoke, one real email per pipeline. Zero handler diff.

### Phase 3 — `lambdas/shared/` extraction (PR-2) · effort: M · risk: medium
**Goal:** one canonical copy per bare module name where copies are byte-identical or trivially superset-able.
- Create `lambdas/shared/` (handbook `cdk-project-layout.md`). Move in dependency-risk order:
  1. **`ses_auth.py`** (byte-identical; the prime target — security-critical auth that currently requires every hardening fix to land twice). Add `cp shared/*.py /asset-output/` to both bundling commands; module lands flat so `from ses_auth import authenticate_inbound_email` is unchanged — zero handler diff keeps fail-closed auth byte-identical.
  2. **`web_ui_auth.py`**: the byte-identical block (`_get_auth_token`/`_header`/`is_authenticated` + the four cache globals). The web_ui functions have no bundling today — add the same staging mechanism. Keep the per-stack INFRA-74 comments in each handler (they've already drifted in wording; they're stack-specific).
  3. **`email_parsing.py`**: `parse_raw_email` superset returning `cc` unconditionally (PO ignores it — harmless).
  4. **`emf.py`**: generic emitter parameterized by namespace/dimension-sets/properties, serving all **three** hand-built `_aws` envelopes (both ParseMethod emitters + `_emit_derived_agreement_metric`). Dimension-set list `[["ParseMethod"],["ParseMethod","TemplateId"]]` is load-bearing for the alarms; a shared function makes one-sided dimension fixes impossible.
- **Do NOT move:** `template_parser.py` (990 vs 508 lines, genuinely divergent — stays per-pipeline in `_SIBLING_MODULES`), the Bedrock extraction functions (divergent contracts; share only the invocation/prompt-delimiting layer, parameterizing `json.loads` kwargs, `max_tokens` 2048/1024, prompt), per-pipeline `validate_ai_fallback` gates.
- Test plumbing in the same PR: update `_SIBLING_MODULES` resolution, `_po_parser_support.py:71`, the fixture-hygiene test, drop the `ses_auth` fixture params (halves the 448-line test_ses_auth run). The sys.modules save/restore dance **survives for template_parser** — it does not shrink to nothing.
- **Gates:** full suite, smoke, deploy-then-merge watching both sender-auth-rejected alarms through live mail. The post-merge CI redeploy being a no-op (unchanged asset hash) is itself a verification signal. Auth code moved → `/sh-security-review`.

### Phase 4 — `cdk/common.py` dedup · effort: M · risk: medium (logical-ID discipline)
**Goal:** collapse the 379 identical CDK lines.
- Extract as **plain functions taking `(scope, id, ...)` called with the SAME scope (the Stack) and SAME construct ids** — 100% logical-ID-safe. **Do NOT wrap in Construct subclasses**: that inserts a tree node, changes every child logical ID, and would attempt replacement of the RETAIN-protected `purchase-orders`/`WorkOrders` tables and named buckets.
- Contents: `_DDB_ALARM_OPERATIONS` + `add_ddb_alarms`, `add_sender_auth_rejected_alarm`, `add_standard_lambda_alarms(scope, id_prefix, fn, name_prefix, topic, *, duration_statistic, errors=True, dlq=None, descriptions=...)` (variance to preserve: PO p99 vs wo-email-processor p95, po-web-ui throttles+duration only, site_extractor no-DLQ, **wo web_ui has zero alarms — do not silently add any**; bespoke description strings passed verbatim), `make_bedrock_invoke_statement` (derive the inference-profile ARN from `Stack.account`/`Stack.region` instead of hardcoding 328440206208), `make_email_bucket`, `make_processor_dlq`, `make_fallback_rate_alarm(namespace, rejected_included, period, threshold, floor, evaluation_periods, datapoints_to_alarm)` reproducing expression strings/FILL/labels byte-for-byte. WO's rejected alarm stays a WO-only call (until Phase 1's PO twin).
- Do not import stack-specific services (kms/ssm/event_sources) into common.py.
- **Gates:** `cdk diff` on BOTH stacks showing **zero changes** (the acceptance test); moving IAM PolicyStatement construction → mandatory **cross-family review** (`cross_review.py`) even though semantics are identical.
- Same PR: add `account='328440206208'` to both `cdk.Environment` calls, pin `constructs==` exact, fix the stale "2.259.0" comments, add CfnOutputs for the five function ARNs + consumed table names (net-new — ID-safe).

### Phase 5 — Handler decomposition + lazy boto3 clients · effort: M · risk: medium
**Goal:** break the God-modules along the seams that already work (ses_auth/template_parser/derive_all prove the flat-sibling pattern).
- PO: `handler.py` (event loop + auth + routing) / `extraction.py` (parse_raw_email shim, extract_with_claude, prompt import) / `enrichment.py` (enrich_parsed, pad_zip — PO-only, WO has no enrichment stage) / `telemetry.py` / `persistence.py` (`_write_fields`/`_merge_update`/save_*; collapse the byte-identical `save_new_po`/`save_revision` into one `_save_merge`). WO: ~5 concerns, not 7 — its split must keep `validate_ai_fallback` + the `[0-9]+` work_order_id key guard in the handler loop ahead of both saves, and keep `_header_date_iso`/comment_id determinism with persistence.
- Move `EXTRACTION_PROMPT` (181/693 lines of po handler) to `prompts.py` with cross-reference headers to derived_fields; keep a re-export since 4 tests dereference `handler.EXTRACTION_PROMPT`.
- Lazy cached boto3 accessors in I/O modules; pure modules import no boto3. Update the `monkeypatch.setattr(handler, "dynamodb", fake)` patch surface in the same change. Preserve the **moto-before-handler import ordering** (`_po_parser_support.py:26-33`) or moto-backed suites hit real AWS.
- **Behavior-preservation obligations stated per pipeline:** PO metric before Bedrock call; WO metric after gate with mutually-exclusive `ai_fallback`/`ai_fallback_rejected`; shadow telemetry ai_fallback-only. Bundling: the glob from Phase 0 already ships new siblings automatically; the ast test verifies.
- **Gates:** full suite (goldens unchanged), smoke, one live email per pipeline. Handler decomposition keeps signatures — no cross-family review needed unless the event/return contract changes.

### Phase 6 — site_extractor reconciliation · effort: M · risk: medium · **timing: after the shadow bake concludes**
**Goal:** end the three-way site_code contradiction (KLAL-class all-letter codes currently can NEVER self-register — permanent pending-review rows).
- `extract_site_code` already prefers `record["site_code"]` (line 36) — the defect is the digit-requiring `SITE_CODE_PATTERN.match` (prefix-anchored, rejects KLAL, accepts overlong junk like `DLI6X`). Fix: validate the direct field with the canonical `derived_fields` shape + skip-list semantics (fullmatch), import `derive_site_code` for the fallback path (ship `derived_fields.py` into the site_extractor asset via the Phase 2/3 mechanism), delete the bespoke regex ladder (line 62's `[A-Z]{4,5}` alternation is dead code). Keep shape/skip validation on the ai_fallback-sourced field rather than blind trust (LLM value is authoritative during the bake). Keep parse_address + pending-review flow as-is.
- Repoint `scripts/backfill_sites.py` at `derive_site_code` (or delete it — one-time script).
- Characterization tests FIRST (pin current behavior, including the KLAL divergence, before changing it) — see §4.
- **Gates:** new site_extractor test suite, deploy, watch pending-site-review write rate drop.

### Phase 7 — Ops/recovery + dependency hygiene · effort: S · risk: low (can run parallel to 4–6)
- Generalize `scripts/reprocess.py`: `--pipeline po|wo`, `--key/--prefix/--since`; targeted replay primary, full-prefix demoted behind `--all` with documented caveats (Event-type invocation is concurrent → sorting doesn't serialize; switch to RequestResponse if order matters; metrics double-counted; Bedrock re-billed; out-of-order replay regresses merged fields).
- `docs/runbook-dlq-recovery.md`: async-destination DLQ has **no redrive-to-source**; procedure = receive-message → key from event body → targeted re-invoke → verify → purge. State the windows: 14 d DLQ breadcrumb, 90 d raw-email S3 (lifecycle overrides RETAIN). Document that sender-auth and ai_fallback_rejected drops intentionally never reach the DLQ. Link from README alarms section.
- Dependency hygiene: drop vendored boto3 (both email-processor requirements → handbook empty-with-comment form; only-boto3 functions correctly use the runtime copy per `lambda-template.md`); simplify bundling to cp-only; exact-pin `moto==` and add `/tests`, `/lambdas/po/web_ui`, `/lambdas/po/site_extractor` dependabot entries (pin first — floor pins make dependabot entries no-ops); reduce wo/web_ui's dead manifest to empty-with-comment.
- Delete the 44 MB `package/` dir (one benign asset-hash redeploy; do it before/with Phase 2's excludes).

### Phase 8 — Test-root consolidation (last PR — validates the new boundaries) · effort: M · risk: low
See §4. Also the small correctness/doc items batched here: WO `invalid_status` reason-code fix (+ grep dashboards for `malformed_site_code` first), WO Bedrock-error metric fix (wrap the call: emit `ai_fallback`/`bedrock_error` in an except-and-reraise — NOT a naive reorder, which double-counts against wo_stack's "rejected emits nothing else" alarm contract), `_validate_new_po_values` per-rule split (V4 anchor-frame dataclass must carry `summary_matches`/`price` for V13; add a ruff config enabling PLR/C901 or the noqa-drop is meaningless; include `extract_new_po` C901=35), docstring/README drift batch (findings 33–38 appendix refs).

---

## 4. Test-hardening plan

### Target layout — resolving the two-roots question
**Keep both roots; fix the loading.** The split itself is principled (per-lambda golden suites beside the code; cross-pipeline suites at top). Changes:
- **One loader.** New **repo-root `conftest.py`** (required anyway for package mapping; `tests/conftest.py` does not load for standalone `pytest lambdas/po` runs, so it cannot carry session invariants): dummy AWS env, **moto BUILTIN_HANDLERS registration before any handler import** (with the explanatory comment currently buried in `_po_parser_support.py:33`), and a single `load_lambda_module(pipeline, name)` keeping one copy of the sys.modules save/restore (still needed for template_parser's duplicated bare name).
- Rewrite `_wo_parser_support.py` off the bare-import strategy (it is the source of the collision the other two loaders defend against). Shared `tests/support/` package: superset `FakeTable` (update_item + WO's put_item/keyed store), `FakeDynamoResource`, `load_email`, `load_golden` **keeping PO's `parse_float=Decimal`** (load-bearing for exact money comparison).
- File moves: `test_po_merge.py`, `test_pad_zip.py` → `lambdas/po/email_processor/tests/`. `test_parse_raw_email.py` and `test_ses_auth.py` **stay at root** (genuinely cross-pipeline, parameterized over both handlers).
- Delete `test_local.py` (globs a nonexistent `samples/`, WO-only, bypasses the gate; the golden suites cover its role) or rewrite with `--pipeline` — deletion preferred.

### Missing scenarios by module (priority order)
1. **PO AI-fallback negatives** (with Phase 1): non-dict output (list/str/int/None — today AttributeErrors at the logger f-string into retries/DLQ), injected po_number/email_type/status, XML-wrap + neutralization + linear-time regex, markdown-fence stripping (currently untested in PO).
2. **Bedrock transport errors, both pipelines:** ThrottlingException (assert PO's pre-call metric survived + no partial write + exception propagates into the errors-alarm/DLQ path — this doubles as the only real proof of PO's metric-before-call ordering), missing `content` key, empty content list, non-JSON model text. **Pin WO's no-datapoint-on-throttle behavior with a documenting test** (do not "fix" by reordering — see Phase 8 note).
3. **Handler-level SES auth seam:** today every dispatch test monkeypatches `authenticate_inbound_email=True`; deleting the gate line would pass all 568 tests. Add per pipeline: reject-path with no auth monkeypatch + empty `ALLOWED_DKIM_DOMAINS` (assert zero Bedrock calls, zero writes, no raise; env is read at call time so setenv suffices; FakeS3 still needed — the gate sits after get_object). Accept-path is turnkey for PO (scrubbed Coupa fixtures authenticate); WO needs an SES-stamped fixture synthesized from `WO_SES_HEADER` in test_ses_auth.
4. **web_ui (both, 0% today — auth is the mandatory-review surface):** fail-closed on unset ARN (requires module reload — ARN read at import), fail-closed on Secrets Manager exception, TTL cache refresh (reload fixture resets globals), Bearer/X-Auth-Token/case-insensitivity, wrong-token 401, **401 without table scan**, non-ASCII token (hmac.compare_digest TypeError → 500 today, worth pinning/fixing), hostile-field escaping (regression lock — escaping is currently correct on inspection). PO web_ui lacks `__init__.py` — use the loader, not package imports.
5. **WO merge semantics:** `tests/test_wo_merge.py` moto-backed mirror of test_po_merge (table name `WorkOrders`, not kebab): null-status never clobbers wo_status, created_at immutable via if_not_exists, status→wo_status mapping, None fields absent from SET, record_type only-when-present.
6. **site_extractor characterization (before Phase 6):** extract_site_code shapes incl. KLAL divergence and prefix-match overlong acceptance (`SNY55`), parse_address variants, stream routing (INSERT/MODIFY/REMOVE, pending-review fallback), upsert_site SS-ADD expression.
7. **Small pins:** PO-DC-02 64-char EMF clamp regression test (~10 lines in test_po_derived_wiring); multi-record event failure-isolation test per pipeline (documents the all-or-retry contract); reprocess.py synthetic-event-shape contract test (also pins "raw key, no URL-decoding").

### Fixtures/goldens
Golden .eml + expected-JSON suites are the model — extend, don't replace. Shared `tests/support/` loaders; per-pipeline fixtures stay per-pipeline (real scrubbed samples). WO gains one SES-stamped fixture (synthesized header block, not new scraped mail).

### CI changes
- Add `--cov` with an explicit module list (or add `__init__.py` so `--cov=lambdas` stops silently skipping web_ui/site_extractor); fail-under once web_ui/site_extractor suites exist.
- Add the Phase 0 ast bundle-consistency test to the standard run.
- Add ruff config enabling C901/PLR so complexity ceilings are enforced, not decorative.
- Lint scope currently omits `scripts/` and `test_local.py` — include scripts/ (or delete test_local.py and moot half of it).
- Keep the strict `ci / ci` required check; **no admin-bypass pushes to main while migration PRs are in flight** (bypass_mode=always currently allows a zero-CI production deploy).

---

## 5. Findings appendix

55 confirmed (survived the verification pass, several with sharpening corrections noted in §3/§4), 16 **unverified** (plausible on the evidence given but not independently re-verified — treat as candidates, not commitments). No finding was refuted outright; verification corrections were incorporated into the plan above.

### Duplication / drifted copies
| # | Finding | Verdict | Impact |
|---|---|---|---|
| D1 | AI-fallback hardening (#104) landed in WO only; PO fallback already drifted behind it | confirmed | high |
| D2 | ses_auth.py byte-identical 452-line copy, sync policed by test fixture | confirmed | high |
| D3 | web_ui fail-closed auth gate byte-identical ~61-line block in both handlers | confirmed | high |
| D4 | No lambdas/shared/ despite 4 duplicated modules (handbook location unused; "prescribes" softened to "provides, and the need condition is met") | confirmed | high |
| D5 | EMF ParseMethod emitter near-identical copy, already drifting (actually 3×, incl. derived-agreement emitter) | confirmed | medium |
| D6 | parse_raw_email drifted copy (WO Cc delta is intentional; poor standalone ROI — fold into D2's plumbing) | confirmed | medium |
| D7 | Bedrock-fallback test suites drifted mirroring source drift (WO has 8 tests PO lacks, not 4) | confirmed | medium |
| D8 | template_parser pair: extract only common substrate (_plain_lines etc.), do NOT merge parsers | **unverified** | low |
| D9 | save_new_po / save_revision byte-identical bodies | **unverified** | low |
| D10 | Three hand-rolled DynamoDB SET-expression builders | **unverified** | low |
| D11 | test_local.py + backfill_sites.py entrench single-pipeline layout | **unverified** | low |

### Architecture
| # | Finding | Verdict | Impact |
|---|---|---|---|
| A1 | PO ai_fallback path has no validation-gate module (structural parity gap; unvalidated po_number becomes partition key; sticky-Cancel blast radius) | confirmed | high |
| A2 | Both email-processor handlers are God-modules (WO ~5 concerns, not 7) | confirmed | high |
| A3 | site_extractor is a third, contradictory site-code classifier (all-letter codes can never self-register) | confirmed | high |
| A4 | Module-import boto3 clients force the bespoke loader tax (paid 3×) | confirmed | medium |
| A5 | Trade/site/fiscal rules in two authoritative texts (prompt + derived_fields); keep independent during bake | confirmed | medium |
| A6 | Two test roots, three loading idioms, load-bearing moto import order | confirmed | medium |
| A7 | web_ui interleaves auth/data/HTML rendering | **unverified** | low |

### Code quality / consistency
| # | Finding | Verdict | Impact |
|---|---|---|---|
| Q1 | PO fallback lacks fail-closed gate + injection hardening (consistency view of D1/A1; write path uses non-null-key spray vs WO's whitelist) | confirmed | high |
| Q2 | site_extractor superseded by derived_fields (skip-list sub-claim corrected: real divergence is prefix-match false-accepts + dead all-letter alternation) | confirmed | medium |
| Q3 | _validate_new_po_values 294-line monolith, 60 returns; noqa suppressions currently inert (no ruff config); extract_new_po C901=35 is the worse peer | confirmed | medium |
| Q4 | WO gate misleading reason codes ("malformed_site_code" for bad status — and validate_ai_fallback already returns "invalid_status" for the same failure, so one failure → two codes by path) | confirmed | medium |
| Q5 | Stale 44 MB package/ vendored tree (real cost: synth staging + docker mount + asset-hash divergence, not sys.path shadowing) | confirmed | medium |
| Q6 | web_ui full-table scan + in-memory sort + magic 500 (page-cap option rejected — scan order isn't recency; GSI or accept arbitrary order) | confirmed | medium |
| Q7 | test_local.py rotted (missing samples/, WO-only, bypasses gate) | **unverified** | low |
| Q8 | Type hints absent/degenerate on newest modules | **unverified** | low |
| Q9 | _classify_desc C901=21 priority ladder — defer past bake | **unverified** | low |
| Q10 | WO template_parser function-local imports / sentinel-after-use | **unverified** | low |
| Q11 | backfill_sites.py sys.path hack keeps stale extractor alive | **unverified** | low |

### Tests
| # | Finding | Verdict | Impact |
|---|---|---|---|
| T1 | PO ai_fallback: zero negative tests for hostile model output | confirmed | high |
| T2 | Bedrock transport errors untested both pipelines; metric-ordering divergence unpinned (WO reorder would break its alarm contract — pin, don't move) | confirmed | high |
| T3 | Handler-level SES auth wiring never tested (every dispatch test bypasses) | confirmed | high |
| T4 | web_ui completely untested incl. auth gate (escaping itself verified correct — tests are regression locks) | confirmed | high |
| T5 | Three divergent test-bootstrap loaders + byte-identical fake-Dynamo helpers (only FakeDynamoResource/_stems identical; FakeTable/load_golden intentionally divergent — superset needed) | confirmed | medium |
| T6 | WO save_work_order merge semantics no direct tests | confirmed | medium |
| T7 | Loader choreography: moto import-order invariant enforced only by comments (not breakable by reordering today; maintenance landmine) | confirmed | medium |
| T8 | PO-DC-02 64-char clamp no regression test | **unverified** | low |
| T9 | Multi-record S3 event batch-abort semantics untested | **unverified** | low |
| T10 | test_rule7_bad_status locks in the wrong reason code | **unverified** | low |
| T11 | scripts/ untested and exclusion undocumented | **unverified** | low |

### CDK / infrastructure
| # | Finding | Verdict | Impact |
|---|---|---|---|
| C1 | ~400 (measured 379) identical lines between stacks; extract plain-function cdk/common.py; Construct-wrapping forbidden (logical-ID replacement of RETAIN tables) | confirmed | high |
| C2 | PO fallback-rate alarm omits rejected series; no PO rejected alarm (retune for 57/day — WO's sparse idiom structurally dead at PO volume) | confirmed | high |
| C3 | Alarm helpers duplicated verbatim; per-function alarm boilerplate 13+9 sites (variance to preserve: p99/p95, per-function alarm subsets, wo web_ui has none) | confirmed | medium |
| C4 | Duration statistic drift p99 vs p95, no rationale | **unverified** | low |
| C5 | WorkOrders/WorkOrderComments naming violates kebab-case but replacement-locked → document as legacy, don't rename | **unverified** | low |
| C6 | constructs floor-pinned; stale version comments | **unverified** | low |
| C7 | cdk.Environment omits account; Bedrock ARN hardcodes it | **unverified** | low |
| C8 | No function-ARN CfnOutputs; WO stack has zero outputs | **unverified** | low |

### Contracts & docs
| # | Finding | Verdict | Impact |
|---|---|---|---|
| X1 | README "Data is never corrupted" false for PO fallback path (docs/po-template-parser.md contradicts it via issue #101) | confirmed | high |
| X2 | Three inconsistent site_code definitions; stream field-contract subsection missing (README does list site_extractor as consumer — gap is field-level) | confirmed | high |
| X3 | WO emits ParseOutcome after Bedrock → throttle drops record from alarm denominator, contradicting README ("emit-before" fix rejected — double-counts; use except-and-reraise with bedrock_error reason) | confirmed | medium |
| X4 | Stale quantity/price prompt-type comments; doc self-contradicts §8 vs §10 | confirmed | medium |
| X5 | README omits derived-field suites/derived_fields.py/docs/; 64-char clamp undocumented | confirmed | medium |
| X6 | derived_fields "FAITHFUL v1 port" docstring false (contradicts its own line-100 backtest comments); reword to spec-superset + keep runtime-authority nuance | confirmed | medium |
| X7 | Web UI invocation contract stale ×3; README example guaranteed 401 | confirmed | medium |
| X8 | verified-sites contract: lat/long/notes provenance unknown; INFRA-138 by-state GSI breach live (restore would be a sparse GSI — relevant to the decision) | confirmed | medium |

### Deploy pipeline & bundling (gap dimension)
| # | Finding | Verdict | Impact |
|---|---|---|---|
| P1 | Deploy runs zero tests; cd-cdk pre-flight/health-check dormant (no stack-name); admin bypass allows zero-CI production deploy | confirmed | high |
| P2 | cp allowlist invisible to every gate → production ImportError class (bit PR #105); glob + ast test | confirmed | high |
| P3 | Migration sequencing: guards → root-move → extraction, each deployed/verified (PR-1 corrections: WO zip not byte-identical; pip -r path change) | confirmed | high |
| P4 | Divergent bundling strategies; WO ships tests/ + __pycache__; from_asset ignores .gitignore (hash-poisoning is in WO cp -r and the three non-bundled assets, not PO's allowlist) | confirmed | medium |
| P5 | Broken-bundle detection traffic-dependent; no synthetic canary (even with stack-name set, CFN status check passes a broken bundle — sync invoke + FunctionError check is the load-bearing part) | confirmed | medium |

### DLQ / recovery (gap dimension)
| # | Finding | Verdict | Impact |
|---|---|---|---|
| R1 | No WO recovery tooling — reprocess.py hardcoded PO-only; async-destination DLQ has no console redrive, so scripted re-invoke is the ONLY path | confirmed | high |
| R2 | No DLQ recovery runbook; 14-day-DLQ vs 90-day-S3 windows stated nowhere | confirmed | high |
| R3 | reprocess.py --execute: full-prefix replay not order-safe (Event invocation = concurrent; sorting alone doesn't serialize); same-object replay idempotent | confirmed | medium |
| R4 | Batch-abort → whole-event DLQ message; redrive re-runs succeeded records (safe by idempotency, undocumented; do NOT add per-record swallowing) | confirmed | low |

### Dependency hygiene (gap dimension)
| # | Finding | Verdict | Impact |
|---|---|---|---|
| H1 | Floor-pinned boto3 in Docker bundles → non-reproducible artifacts; vendored copy entirely unnecessary (runtime boto3 suffices) | confirmed | high |
| H2 | Target-state manifest table; layer-vs-shared-dir Dependabot implication (shared/ source dir with no manifest needs no new entry) | confirmed | medium |
| H3 | po/web_ui + site_extractor: no requirements.txt, invisible to Dependabot (cite cdk-project-layout.md, not lambda-template's SAM rationale) | confirmed | low |
| H4 | wo/web_ui requirements.txt is a dead manifest (10 no-op Dependabot PRs merged; file ships as dead weight in the zip) | confirmed | low |
| H5 | tests/requirements.txt uncovered + floor pin (pin first or the dependabot entry is a no-op) | confirmed | low |

---

## 6. Explicitly out of scope / rejected

| Rejected | Why |
|---|---|
| **Merging the two template parsers** | 15.4% similarity; extractors and validation rules are genuinely domain-specific (Coupa PO vs Hexagon WO). Only the byte-identical low-level helpers are extraction candidates, and even that is low-impact/unverified (D8). |
| **CDK Construct-subclass composition** | Changes every child logical ID → CloudFormation replacement of RETAIN tables and create-failure on named buckets. Plain functions with unchanged scope/ids, gated on zero `cdk diff`, achieve the dedup safely. |
| **Lambda layer for shared code** | Creates a cross-stack export (layer owned by one stack, consumed by the other) that blocks independent updates under `cdk deploy --all`. Asset-root widening keeps each function self-contained. Revisit only if shared code grows real third-party deps. |
| **Renaming WorkOrders/WorkOrderComments/non-prefixed resources to kebab-case** | table_name/function_name changes force resource replacement (production data; cross-stack `fromTableName` consumers). Correct move is a handbook Legacy-section entry; revisit at the INFRA-6 CMK table touch. |
| **Reordering WO's emit_parse_metric before the Bedrock call ("PO parity")** | Would double-count gate-rejected emails against wo_stack's fallback-rate MathExpression and violate the documented "rejected emits nothing else" contract. Fix is except-and-reraise with a `bedrock_error` reason code + a pinning test. |
| **Moving PO's pre-Bedrock metric emission after the gate** | The pre-call emit is deliberate (Bedrock errors still record ai_fallback) and load-bearing for the alarm; ai_fallback_rejected is additive instead. |
| **Per-record exception swallowing in handler loops** | Would remove the retry→DLQ capture and weaken fail-closed behavior. If isolation is ever wanted: collect failures, re-raise at end. Documented in the runbook instead. |
| **Dedupe/skip logic inside handlers for replay safety** | Weakens the deliberate always-upsert semantics; replay safety handled in the reprocess script (targeted-by-default) and runbook. |
| **Generating EXTRACTION_PROMPT from derived_fields rule tables** | Defeats the shadow bake's purpose (comparing two independent implementations). Appropriate only after Python becomes authoritative and the prompt sections are deleted. |
| **Any derived_fields.py behavior change before the bake concludes** | Shadow-telemetry constraint: the module is under agreement measurement; restructuring (incl. the _classify_desc ladder refactor, Q9) confounds the telemetry. Docstring fix (X6) is text-only and allowed. |
| **Adding alarms to wo-web-ui as a side effect of the alarm helper** | New paging surface; propose separately and deliberately, never silently via refactor. |
| **Consolidating the fallback-rate alarms into one parameter-free helper** | PO's 6h/≥8-floor tuning is a deliberate, documented retune for ~57 emails/day vs WO's 15-min/≥10; helper parameterizes, per-stack values stay. |
| **fromTableName→CfnOutput coupling changes for slack-bot consumers** | Cross-repo; out of this refactor. CfnOutputs are added (C8) but consumer migration is seahaven-slack-bot's change. |
| **Full rewrite / repo split** | Nothing measured supports it: the tested tree is healthy (93%, fast suite, clean lint); every problem is addressable by consolidation with production continuity. |

---
*Cross-cutting compliance notes for execution: Phases 0, 1, 3 touch untrusted-input/auth surfaces → `/sh-security-review` mandatory before push. Phase 4 moves IAM policy construction and Phase 0 touches handler event contracts → cross-family `cross_review.py` review mandatory. Every PR: `ruff check` + `ruff format --check` locally before push; `gh pr checks` green before merge; deploy-then-merge for all deploy-affecting phases. README updates land in the same commits as the changes they describe; the Confluence "AWS Architecture Map" needs updating when Phase 4/7 alter the resource inventory (new outputs, runbook).*