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).