diff --git a/.claude/workflows/phase-8-test-consolidation.js b/.claude/workflows/phase-8-test-consolidation.js new file mode 100644 index 0000000..b344c99 --- /dev/null +++ b/.claude/workflows/phase-8-test-consolidation.js @@ -0,0 +1,712 @@ +export const meta = { + name: 'phase-8-test-consolidation', + description: 'Phase 8 of the procurement-ingest refactor (docs/refactor-evaluation.md §4): test-root consolidation — keep BOTH roots but fix the LOADING via ONE new repo-root conftest.py (dummy AWS env + moto BUILTIN_HANDLERS before any handler import + single load_lambda_module keeping one sys.modules save/restore); a shared tests/support/ package (superset FakeTable, FakeDynamoResource, load_email, load_golden with parse_float=Decimal); rewrite _wo_parser_support.py off the bare import strategy; move test_po_merge/test_pad_zip into the PO tests dir (test_parse_raw_email/test_ses_auth stay root); delete test_local.py; add the missing scenarios (PO+WO Bedrock transport errors, handler SES-auth reject seam, web_ui both, WO merge semantics, small pins); CI gains --cov with an explicit module list (or __init__.py), a fail-under, the Phase 0 AST bundle test, an enforced ruff C901/PLR config incl scripts/. The ONLY product-code change is the WO handler Bedrock-error metric-wrap (except-and-reraise, no double-count) + invalid_status reason-code fix, and the PO _validate_new_po_values per-rule split. LAST PR — validates the new module boundaries, so it hard-gates on Phases 3 AND 5 merged. Committed locally, never pushed (push gated on /sh-security-review in the main loop for the WO metric change).', + phases: [ + { title: 'Setup', detail: 'verify Phases 3 (lambdas/shared/) AND 5 (decomposed handler siblings) on base, branch feature/phase-8-test-consolidation', model: 'haiku' }, + { title: 'Recon', detail: '4 mappers: the three loaders + FakeTable/support duplication, test inventory + coverage gaps, WO metric/reason-code + PO validation-split surface, CI/lint/ruff/README drift' }, + { title: 'Spec', detail: 'serial fable spec: pin root conftest, support package, loader rewrites, file moves/deletions, new scenarios, WO metric-wrap + reason-code, PO validation split, ruff config, CI changes, README, disjoint ownership' }, + { title: 'Implement', detail: 'opus: all tests + support + root conftest; opus: WO handler + PO template_parser product-code + newly-surfaced lint fixes; sonnet: ruff/pytest/CI config + README — disjoint files', model: 'opus' }, + { title: 'Verify', detail: 'mechanical gates (full suite green under the new loader, goldens UNCHANGED, ruff green incl scripts/, both cdk synth, --cov covers web_ui/site_extractor, WO no-double-count pin passes) + 3 fable lenses (loader-integrity, WO-metric-contract, coverage-honesty)' }, + { title: 'Fix', detail: 'opus fixer, full re-verify, max 3 rounds', model: 'opus' }, + { title: 'Package', detail: 'single commit via -F (no push)', model: 'sonnet' }, + ], +} + +// ---------------------------------------------------------------- constants + +const REPO = '/Users/adammoussa/Documents/repositories/seahaven/procurement-ingest' +const BRANCH = 'feature/phase-8-test-consolidation' +let _args = args +if (typeof _args === 'string') { + try { _args = JSON.parse(_args) } catch (e) { _args = null } +} +const BASE = (_args && _args.base) || 'main' + +const CONSTRAINTS = ` +PINNED BEHAVIORAL CONSTRAINTS (docs/refactor-evaluation.md Phase 8 + §4 — violating any is a build failure): +1. KEEP BOTH TEST ROOTS; fix the LOADING, not the split. ONE loader: a NEW + repo-root conftest.py (tests/conftest.py does NOT load for a standalone + 'pytest lambdas/po' run, so it cannot carry session invariants) that: + sets the dummy AWS env; registers moto BUILTIN_HANDLERS BEFORE any handler + import (carry the explanatory comment currently buried at + _po_parser_support.py:33 verbatim — boto3 sessions only pick up moto's + stubber hook if created AFTER registration); and exposes a single + load_lambda_module(pipeline, name) keeping ONE copy of the sys.modules + save/restore dance (tests/conftest.py:70-85). That dance does NOT shrink to + nothing — template_parser is still a duplicated bare name across pipelines + and still needs per-exec sibling binding. +2. Rewrite _wo_parser_support.py OFF the bare 'import handler' / + 'from handler import parse_raw_email' strategy (it is the source of the + bare-name sys.modules collision the other two loaders defend against — see + _wo_parser_support.py:17-18,39). Add a shared tests/support/ package with: + a SUPERSET FakeTable (PO's update_item recording + WO's put_item and the + keyed single-row store from _wo_parser_support.py:51-67), FakeDynamoResource, + load_email, load_golden — KEEPING PO's parse_float=Decimal in load_golden + (load-bearing: exact money comparison at PO magnitudes; + _po_parser_support.py load_golden). PO-only helpers keep Decimal; WO's + load_golden has no parse_float today — the superset must not regress PO. +3. FILE MOVES: tests/test_po_merge.py and tests/test_pad_zip.py -> + lambdas/po/email_processor/tests/ (PO-specific, belong beside the code). + tests/test_parse_raw_email.py and tests/test_ses_auth.py STAY at root + (genuinely cross-pipeline, parameterized over BOTH handlers). git mv so + history follows; fix their imports to the new support package. +4. DELETE tests/../test_local.py (repo root: globs a nonexistent samples/, + WO-only, imports a handler at collection so it bypasses the loader gate; + the golden suites cover its role). Deletion is PREFERRED over a --pipeline + rewrite. Its pytest.ini exclusion comment goes with it. +5. ADD the missing scenarios by module (§4 priority order), using the new + single loader + support package for EVERY new test: + (a) PO+WO Bedrock transport errors: ThrottlingException, missing 'content' + key, empty content list, non-JSON model text. Assert PO's pre-call + ai_fallback metric survived + no partial write + exception propagates. + PIN WO's no-datapoint-on-throttle behavior with a DOCUMENTING test — + do NOT "fix" it by reordering (constraint 7 owns the real fix). + (b) Handler-level SES-auth reject seam, per pipeline: NO auth monkeypatch + + empty ALLOWED_DKIM_DOMAINS -> assert ZERO Bedrock calls, ZERO writes, + NO raise (env read at call time; FakeS3 still needed, gate sits after + get_object). Today deleting the gate line passes all tests — this closes + that hole. + (c) web_ui BOTH functions (0% today; auth is the mandatory-review surface): + fail-closed on unset ARN (module reload — ARN read at import), + fail-closed on a Secrets-Manager exception, TTL cache refresh, + Bearer / X-Auth-Token / header case-insensitivity, wrong-token 401, + 401 WITHOUT a table scan, non-ASCII token, hostile-field escaping + regression lock. PO web_ui lacks __init__.py — use the loader, not + package imports. + (d) WO merge semantics: a 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. + (e) Small pins: PO-DC-02 64-char EMF clamp regression, per-pipeline + multi-record failure-isolation (all-or-retry contract), reprocess.py + synthetic-event-shape contract IF Phase 7 has not already added it + (recon confirms — do not duplicate). +6. CI CHANGES (in the same PR): add --cov with an EXPLICIT module list (OR add + __init__.py so --cov=lambdas stops silently skipping web_ui/site_extractor + for the missing package marker) — pick ONE, state why; add a fail-under + ONCE the web_ui/site_extractor suites exist; keep the Phase 0 AST + bundle-consistency test in the standard pytest run; ADD a ruff config + (pyproject.toml or ruff.toml) that ENABLES C901/PLR so the complexity + ceilings are ENFORCED not decorative; include scripts/ in the lint scope; + keep the strict 'ci / ci' required check; NO admin-bypass pushes. +7. WO HANDLER PRODUCT-CODE CHANGES (the ONLY product-code deltas besides the PO + split): (a) invalid_status reason-code fix — GREP the dashboards/metric + filters for 'malformed_site_code' FIRST and report before renaming (one + status failure currently yields two codes by path). (b) WO Bedrock-error + metric fix: WRAP the Bedrock call so ai_fallback + bedrock_error are emitted + in an except-and-RERAISE. This is NOT a naive reorder — a reorder emits the + metric unconditionally pre-call and DOUBLE-COUNTS gate-rejected emails + against wo_stack's "a rejected email emits nothing else" alarm contract. + The handler EVENT/RETURN CONTRACT is unchanged (no signature change). PIN + the no-double-count with a test that computes the emitted series by hand. +8. PO _validate_new_po_values PER-RULE SPLIT (template_parser.py): break the + monolith into per-rule helpers; the V4 anchor-frame dataclass must CARRY + summary_matches / price so V13 can consume them; include extract_new_po + (C901=35) in the split scope. The NEW ruff PLR/C901 config makes the + previously-INERT 'noqa: PLR09xx' suppressions LIVE — every newly-surfaced + violation across the tree must be FIXED or noqa'd-with-written-justification + IN THIS SAME PR. EXCEPTION: files under the shadow-bake freeze + (derived_fields.py) are UNTOUCHABLE — surface their violations via a + per-file ignore in the ruff config (with a justification comment), NEVER an + in-file edit. Docstring/README drift batch (findings 33-38) lands here too. +9. UNTOUCHABLE (git diff ${BASE}...HEAD must be empty for each): every product + module EXCEPT lambdas/wo/email_processor/handler.py (constraint 7) and + lambdas/po/email_processor/template_parser.py (constraint 8). Specifically + derived_fields.py (shadow bake), both ses_auth, both validate_ai_fallback + gates, lambdas/shared/*, all CDK stacks, site_extractor. GOLDEN fixtures + (tests/**/expected/*.json and the .eml corpora) are byte-frozen — a test + that only passes after a golden edit is a FAIL. New tests must NOT edit + existing goldens; WO's one SES-stamped fixture (synthesized header block) + is the only new fixture allowed. +10. This is the LAST phase: the resulting suite / coverage fail-under / ruff + config become the NEW CI floor. Do not weaken any existing gate to make a + new one pass. +` + +const PREAMBLE = ` +You are one of several agents building refactor Phase 8 in the git repo at +${REPO} on branch ${BRANCH} (already checked out — do NOT switch branches, +do NOT create branches, do NOT commit, NEVER push, do NOT run cdk deploy or +touch AWS resources beyond read-only calls). +Authoritative spec: docs/refactor-evaluation.md, section "Phase 8 — +Test-root consolidation" and the whole of "§4 Test-hardening plan". The doc +wins on any conflict. +Work ONLY in the files you are told you own; other agents are concurrently +editing other files in this same working tree. +${CONSTRAINTS} +Your final message is consumed by an orchestrator script, not a human — +return only the structured data requested. +` + +// ------------------------------------------------------------------ schemas + +const RECON = { + type: 'object', + required: ['summary', 'facts'], + properties: { + summary: { type: 'string' }, + facts: { type: 'array', items: { type: 'string' } }, + blockers: { type: 'array', items: { type: 'string' } }, + }, +} + +const SPEC = { + type: 'object', + required: ['rootConftest', 'supportPackage', 'loaderRewrites', 'fileMoves', 'newScenarios', 'productCodeEdits', 'ruffConfig', 'ciChanges', 'readmeDocs', 'ownership', 'notes'], + properties: { + rootConftest: { type: 'string', description: 'the complete new repo-root conftest.py design: dummy AWS env block, the moto BUILTIN_HANDLERS-before-handler-import registration with the carried :33 comment, and the single load_lambda_module(pipeline, name) signature keeping ONE sys.modules save/restore (state exactly why it does not shrink to nothing — template_parser bare name). How tests/conftest.py, _po_parser_support.py and _wo_parser_support.py reconcile against it (deleted / shimmed / re-pointed).' }, + supportPackage: { type: 'string', description: 'tests/support/ package contents: the SUPERSET FakeTable field-by-field (PO update_item record + WO puts/keyed store), FakeDynamoResource, load_email, load_golden — explicitly pinning parse_float=Decimal is kept and that WO callers gain it without regressing (are WO goldens integer-only?). __init__.py exports.' }, + loaderRewrites: { type: 'string', description: 'exact per-file edits to retire the bare-import strategy in _wo_parser_support.py and rewire both support files + tests/conftest.py onto load_lambda_module; the moto-before-handler ordering must survive; which fixtures (email_handler, ses_auth, po_handler, wo_handler) move where.' }, + fileMoves: { type: 'string', description: 'the git mv list (test_po_merge.py, test_pad_zip.py -> PO tests dir), the stay-at-root set (test_parse_raw_email.py, test_ses_auth.py), the test_local.py deletion + its pytest.ini comment removal, and the import rewrites each moved file needs.' }, + newScenarios: { type: 'string', description: 'per-module new test files (paths) and the scenarios each covers per constraint 5 (a-e): Bedrock transport errors both pipelines, SES-auth reject seam both pipelines, web_ui both functions, WO merge semantics, the small pins — and whether the reprocess synthetic-event pin already exists from Phase 7.' }, + productCodeEdits: { type: 'string', description: 'the WO handler edit (invalid_status reason-code fix + Bedrock-error except-and-reraise metric-wrap with the exact emitted series proving NO double-count vs wo_stack alarm contract; the malformed_site_code dashboard-grep result) and the PO _validate_new_po_values / extract_new_po per-rule split (V4 dataclass carries summary_matches/price for V13). file:line for each.' }, + ruffConfig: { type: 'string', description: 'the exact new ruff config (pyproject.toml or ruff.toml): which rule families (C901/PLR), the complexity ceilings (extract_new_po C901=35), lint scope incl scripts/, and the per-file-ignore for derived_fields.py with justification. The full list of newly-surfaced violations and their fix-or-noqa disposition.' }, + ciChanges: { type: 'string', description: 'the exact .github/workflows/ci.yaml edits: --cov approach (explicit module list vs __init__.py, with the reason), where the fail-under lands and its value, keeping the AST bundle test + strict ci/ci check, scripts/ lint scope, no admin bypass.' }, + readmeDocs: { type: 'string', description: 'README + docstring drift batch (findings 33-38): exact sections/lines to fix, matched to existing style.' }, + ownership: { type: 'string', description: 'the DISJOINT file-ownership map for the 3 parallel implement agents (tests+support+root-conftest / product-code+newly-surfaced-lint / config+CI+README) — no path owned by two agents; note the ruff-config -> product-lint dependency and how the poll resolves it.' }, + notes: { type: 'string' }, + }, +} + +const IMPL = { + type: 'object', + required: ['filesChanged', 'summary', 'checksRun'], + properties: { + filesChanged: { type: 'array', items: { type: 'string' } }, + summary: { type: 'string' }, + checksRun: { type: 'string' }, + blockers: { type: 'array', items: { type: 'string' } }, + }, +} + +const CHECKS = { + type: 'object', + required: ['passed', 'details'], + properties: { + passed: { type: 'boolean' }, + details: { type: 'string' }, + scopeViolations: { type: 'array', items: { type: 'string' } }, + }, +} + +const FINDINGS = { + type: 'object', + required: ['findings'], + properties: { + findings: { + type: 'array', + items: { + type: 'object', + required: ['title', 'severity', 'confirmed', 'evidence', 'fix'], + properties: { + title: { type: 'string' }, + severity: { enum: ['critical', 'high', 'medium', 'low'] }, + confirmed: { type: 'boolean' }, + evidence: { type: 'string' }, + fix: { type: 'string' }, + }, + }, + }, + }, +} + +// ------------------------------------------------------------------- setup + +phase('Setup') +const setup = await agent(` +In ${REPO}: +1. SEQUENCING HARD-GATE — this is the LAST PR and it validates the NEW module + boundaries, so Phases 3 AND 5 must both be on ${BASE} (shared/ from Phase 3 + is what the loader now resolves against; the decomposed handler siblings + from Phase 5 are the boundaries these suites exercise). git fetch origin, + then pick the base ref: origin/${BASE} if that remote ref exists, otherwise + the local branch ${BASE} (a stacked local-only base is expected and fine). + Verify on the base ref: + (a) Phase 3: lambdas/shared/ exists with its modules + (git ls-tree -- lambdas/shared/ — must list ses_auth.py etc.); + (b) Phase 5: the decomposed PO handler siblings exist + (git ls-tree -- lambdas/po/email_processor/ must include + extraction.py, enrichment.py, telemetry.py, persistence.py, prompts.py). + If EITHER is missing, STOP with a blocker naming the unmet phase and do + nothing else. +2. Verify clean working tree (untracked .coverage / .claude/ / the local + 44 MB lambdas/po/email_processor/package/ dir are fine; any OTHER dirt = + blocker, never stash or discard). +3. git checkout ${BASE}; then git pull --ff-only ONLY if the branch has an + upstream (a local-only base skips the pull — not a blocker); then + git checkout -b ${BRANCH} +4. gh pr list --state open --json number,title,headRefName (overlap check). +Return facts: HEAD sha, per-phase gate evidence (the two git ls-tree outputs), +open PRs, blockers. +`, { label: 'setup:branch', model: 'haiku', schema: RECON }) + +if (!setup || (setup.blockers && setup.blockers.length)) { + return { aborted: 'setup blockers', blockers: setup ? setup.blockers : ['setup agent died'], facts: setup ? setup.facts : [] } +} +log(`Branch ${BRANCH} ready off ${BASE}. ${setup.summary}`) + +// ------------------------------------------------------------------- recon + +phase('Recon') +const recon = await parallel([ + () => agent(`${PREAMBLE} +Read-only recon of the THREE loading idioms + the fake-Dynamo helpers this +phase collapses: +1. tests/conftest.py — quote the importlib loader, the _SIBLING_MODULES tuple + (ses_auth/template_parser/derived_fields), and the sys.modules save/restore + dance (lines ~70-85). Note that after Phase 5 the PO sibling set changed + (extraction/enrichment/telemetry/persistence/prompts) — quote the CURRENT + sibling list on this branch's base, it may already differ from the audit. +2. _po_parser_support.py — the independent importlib reimplementation, the + load-bearing moto-before-handler comment at line 33 (quote it verbatim, it + must be carried into the new root conftest), load_golden's parse_float=Decimal + (quote it), and its FakeTable (update_item only). +3. _wo_parser_support.py — the bare sys.path 'import handler' / + 'from handler import parse_raw_email' strategy (lines 17-18, 39), and its + FakeTable (put_item + keyed store, lines 51-67) — the superset target. +4. pytest.ini — the testpaths (three roots) and the test_local.py exclusion + comment. Confirm test_local.py exists at repo root and what it imports at + collection. +Diff the two FakeTable classes field-by-field to define the superset. Confirm +whether WO's load_golden uses parse_float (it does not today) and whether any +WO golden is non-integer (would break under Decimal — pin it). +20-30 precise facts.`, + { label: 'recon:loaders-support', model: 'sonnet', phase: 'Recon', schema: RECON }), + + () => agent(`${PREAMBLE} +Read-only recon of the test INVENTORY + coverage gaps this phase must fill: +- Enumerate every test_*.py across all three roots (tests/, + lambdas/po/email_processor/tests/, lambdas/wo/email_processor/tests/) with a + one-line purpose each. Flag test_po_merge.py + test_pad_zip.py at root (move + targets) and test_parse_raw_email.py + test_ses_auth.py at root (stay). +- For the MISSING scenarios (§4), confirm the current gap and the seam each new + test hooks: (a) PO+WO Bedrock transport-error handling — quote where each + handler invokes Bedrock and where ai_fallback is emitted relative to the call + (PO pre-call; WO post-gate); (b) the SES-auth gate call site in each handler + (authenticate_inbound_email) and how existing dispatch tests monkeypatch it; + (c) web_ui — BOTH functions' auth entry points, whether they have any tests + or __init__.py today (they do not), how the ARN/Secrets-Manager fail-closed + path reads config (import-time vs call-time), the table-scan path a 401 must + avoid; (d) WO save_work_order merge semantics (table 'WorkOrders'); (e) the + small pins — the 64-char EMF clamp site, multi-record event loop, and whether + scripts/reprocess.py already has a synthetic-event contract test from Phase 7 + (git log / grep — do NOT duplicate it). +- Confirm the golden corpora locations (expected/*.json, .eml dirs) that are + byte-frozen. +25-35 facts.`, + { label: 'recon:inventory-gaps', model: 'sonnet', phase: 'Recon', schema: RECON }), + + () => agent(`${PREAMBLE} +Read-only recon of the TWO product-code change surfaces (constraints 7-8): +1. WO handler (lambdas/wo/email_processor/handler.py): quote with file:line the + Bedrock invoke, the emit_parse_metric / ai_fallback emission, and the gate + that returns 'invalid_status' vs 'malformed_site_code'. Establish the CURRENT + emitted-metric series for a gate-rejected email and for a Bedrock-error email + — enough to prove the except-and-reraise wrap does NOT double-count. THEN + grep the whole repo (cdk/*.py, dashboards, metric filters, docs) for + 'malformed_site_code' and 'invalid_status' and list every consumer — the + reason-code rename must not silently break a dashboard/alarm. Quote the + wo_stack "a rejected email emits nothing else" alarm/MathExpression it must + not violate. +2. PO template_parser.py: quote _validate_new_po_values (the ~294-line monolith, + its 'noqa: PLR09xx' suppressions) and extract_new_po (C901=35). Identify the + V4 anchor-frame dataclass and what fields it carries today vs the + summary_matches/price V13 needs. Confirm both currently pass lint ONLY + because no ruff config selects PLR/C901. +Return the exact emitted-series tables, the malformed_site_code consumer list, +and the validation-split surface. 20-30 facts.`, + { label: 'recon:product-code', model: 'sonnet', phase: 'Recon', schema: RECON }), + + () => agent(`${PREAMBLE} +Read-only recon of the CI / lint / coverage / README surface: +- .github/workflows/ci.yaml — quote the pytest invocation (does it pass --cov?), + the ruff steps, the required-check name (ci / ci), any admin-bypass config. + Confirm the Phase 0 AST bundle-consistency test (tests/test_bundle_consistency.py) + is or is not already in the standard run. +- Coverage honesty: confirm there is NO ruff config file today (so PLR/C901 are + unselected) and confirm web_ui/site_extractor lack __init__.py (so a naive + --cov=lambdas SILENTLY skips them). Prove the skip by reading the tree, not by + running. +- scripts/ — list the scripts and confirm they are outside the current lint + scope. +- README.md + docstrings — locate the drift items (findings 33-38 / X-series): + the false "Data is never corrupted" style claims, stale web_ui invocation + contract, missing derived-field/coverage docs, and any test-layout description + that this phase changes. Quote line numbers. +15-25 facts.`, + { label: 'recon:ci-lint-readme', model: 'haiku', phase: 'Recon', schema: RECON }), +]) + +const reconOk = recon.filter(Boolean) +const reconBlockers = reconOk.flatMap(r => r.blockers || []) +log(`Recon complete: ${reconOk.length}/4 mappers, ${reconBlockers.length} blockers (all resolved in the main loop — see RESOLUTIONS)`) + +// Main-loop resolutions for the first run's recon blockers — verified facts, +// checked directly against WO goldens, live CloudWatch, and the reusable CI +// workflow source. The remaining recon "blockers" were this phase's own work +// items misreported as blockers; none abort the run. +const RESOLUTIONS = ` +## MAIN-LOOP RESOLUTIONS (verified AFTER recon flagged blockers — authoritative facts, supersede any recon hedge) +1. WO goldens under parse_float=Decimal: SAFE. All 55 golden files under + lambdas/wo/email_processor/tests/fixtures/expected/ were scanned + programmatically — ZERO float-typed JSON number values exist (decimal-looking + strings appear only inside comment_text STRING values, which parse_float never + touches). The superset load_golden keeping parse_float=Decimal cannot regress WO. +2. malformed_site_code consumers: NONE outside the repo. Live scan of the + deployment account 328440206208 (hosts both po-ingest and WorkorderIngestStack): + 0 CloudWatch dashboards, 0 saved Logs Insights query definitions, 18 metric + filters (no match), 146 alarms (no match). seahaven-prod (011934824531) also + clean. Combined with recon's repo-scope grep, the invalid_status reason-code + rename has NO external consumer to break. Still note the rename in the commit + body per the deploy-then-merge outstanding gate. +3. Reusable CI workflow (Sea-Haven-Industries/.github ci-python-sam.yaml + @fd60e4c904, the pinned ref in ci.yaml): inputs.source-dirs threads ONLY into + 'ruff check \${{ inputs.source-dirs }}' and 'ruff format --check' — so adding + 'scripts' to source-dirs in THIS repo's ci.yaml genuinely puts scripts/ in lint + scope. Tests run as bare 'pytest' with NO args — so --cov and --cov-fail-under + MUST land via pytest.ini addopts (bare pytest picks addopts up); ci.yaml cannot + pass pytest flags. Dependency install is 'pip install pytest' plus + 'pip install -r' for EVERY requirements.txt found outside ./.aws-sam — so + pytest-cov (and any other new test dep) must be added to a requirements.txt the + find loop reaches (e.g. a tests/requirements.txt; create it if absent). CI's + ruff is unpinned 'pip install ruff' — the new config must be valid on current ruff. +4. The remaining recon 'blockers' (README drift X1-X8, coverage honesty, absent + ruff config, test_local.py still present) are THIS PHASE'S OWN WORK ITEMS, not + blockers — implement them per the constraints. +` +const pack = reconOk.map(r => `## ${r.summary}\n${r.facts.join('\n')}`).join('\n\n') + '\n\n' + RESOLUTIONS + +// -------------------------------------------------------------------- spec + +phase('Spec') +const spec = await agent(`${PREAMBLE} +You are the SPEC agent — the single authority that pins every contested +decision BEFORE parallel implementation (parallel leaves cannot see each +other's choices). Using the recon pack below plus your own reads of the actual +files, produce the binding implementation spec: +- rootConftest: the complete new repo-root conftest.py — the dummy AWS env, the + moto BUILTIN_HANDLERS-before-handler-import registration WITH the carried :33 + comment, and the single load_lambda_module(pipeline, name) keeping ONE + sys.modules save/restore. State explicitly why the dance does not shrink to + nothing (template_parser bare name) and how the three existing loaders + reconcile (which are deleted, which become thin shims, which re-point). +- supportPackage: tests/support/ — the SUPERSET FakeTable, FakeDynamoResource, + load_email, load_golden. PIN parse_float=Decimal kept in load_golden and + prove WO goldens survive it (recon's non-integer check). +- loaderRewrites: retire the bare-import strategy in _wo_parser_support.py; + rewire both support files + tests/conftest.py onto the new loader; moto + ordering survives; where each fixture lands. +- fileMoves: the git mv set, the stay-at-root set, the test_local.py deletion + + pytest.ini comment removal, per-file import rewrites. +- newScenarios: every new test file path + the scenarios per constraint 5 (a-e). + De-duplicate the reprocess synthetic-event pin against Phase 7 per recon. +- productCodeEdits: the WO handler invalid_status reason-code fix + the + Bedrock-error except-and-reraise metric-wrap with the exact emitted series + (compute by hand: gate-reject emits nothing else; Bedrock-error emits + ai_fallback + bedrock_error ONCE — NO double-count), plus the + malformed_site_code consumer disposition. The PO _validate_new_po_values / + extract_new_po per-rule split (V4 dataclass carries summary_matches/price). + file:line for each; nothing else in these two files changes. +- ruffConfig: the exact config, rule families, ceilings (extract_new_po + C901=35), scripts/ scope, the derived_fields.py per-file-ignore with + justification, and the FULL disposition of every newly-surfaced violation. +- ciChanges: the exact ci.yaml edits — --cov approach (explicit list vs + __init__.py, with reason), fail-under value + placement, AST test + strict + check kept, scripts/ in lint scope, no admin bypass. +- readmeDocs: the drift batch fixes matched to existing style. +- ownership: the DISJOINT 3-agent ownership map with NO shared path, and how + the ruff-config -> product-lint poll dependency resolves. +Recon pack:\n${pack}`, + { label: 'spec:pin-consolidation', phase: 'Spec', schema: SPEC }) + +if (!spec) return { aborted: 'spec agent died — rerun workflow', reconBlockers } +const specBlock = `BINDING SPEC (from the spec agent — implement EXACTLY this):\n${JSON.stringify(spec, null, 2)}` +log('Spec pinned: root conftest, support package, loader rewrites, file moves, new scenarios, product-code edits, ruff config, CI changes, README, ownership') + +// --------------------------------------------------------------- implement + +phase('Implement') +const impl = await parallel([ + () => agent(`${PREAMBLE} +YOU OWN: ALL test + support files — the repo-root conftest.py (NEW), +tests/ (including tests/support/ the new package, tests/conftest.py, +tests/test_bundle_consistency.py, the stay-at-root test_parse_raw_email.py / +test_ses_auth.py, and the test_local.py DELETION), +lambdas/po/email_processor/tests/ and lambdas/wo/email_processor/tests/. +You do NOT touch product code, cdk/, ci.yaml, ruff config, pytest.ini, or +README (other agents own those; pytest.ini's test_local.py comment is the +config agent's edit — coordinate only by not touching it). +Task per spec.rootConftest + spec.supportPackage + spec.loaderRewrites + +spec.fileMoves + spec.newScenarios: +1. Write the new repo-root conftest.py with the single load_lambda_module, + carrying the :33 moto comment VERBATIM and keeping ONE sys.modules + save/restore (it does NOT shrink to nothing). +2. Create tests/support/ (superset FakeTable, FakeDynamoResource, load_email, + load_golden with parse_float=Decimal). Retire _wo_parser_support.py's + bare-import strategy; rewire _po_parser_support.py + tests/conftest.py. +3. git mv test_po_merge.py + test_pad_zip.py into the PO tests dir; DELETE + test_local.py; fix all import lines. +4. Add every new scenario suite per constraint 5 (a-e) using ONLY the new + loader + support package. Do NOT edit any existing golden (constraint 9) — + a test that only passes after a golden edit is a bug in the test. WO's one + SES-stamped fixture is the only new fixture allowed. +Run before returning: pytest -q at repo root (all roots green against the OTHER +agents' edits — they land in parallel; the WO metric-wrap test and PO split may +be mid-flight, so poll by re-running up to ~10 min before reporting a blocker), +and confirm goldens are byte-unchanged (git diff --stat on expected/ dirs is +empty). +${specBlock}`, + { label: 'impl:tests-support', model: 'opus', phase: 'Implement', schema: IMPL }), + + () => agent(`${PREAMBLE} +YOU OWN: lambdas/wo/email_processor/handler.py and +lambdas/po/email_processor/template_parser.py ONLY, PLUS you are the designated +fixer for any NEWLY-SURFACED ruff PLR/C901 violation across product code and +scripts/ (fix-or-noqa-with-justification, NEVER a behavior change). You do NOT +touch tests, cdk/, ci.yaml, README, or derived_fields.py (its violations are +handled via the config agent's per-file-ignore, NOT an edit — constraint 8). +Task per spec.productCodeEdits: +1. WO handler: apply the invalid_status reason-code fix (only after confirming + spec captured the malformed_site_code consumer disposition) and the + Bedrock-error except-and-RERAISE metric-wrap. The wrap emits + ai_fallback + bedrock_error ONCE on a Bedrock error and NOTHING extra on a + gate reject — do the hand series-count in your summary. Handler event/return + contract UNCHANGED. +2. PO template_parser: the _validate_new_po_values / extract_new_po per-rule + split; the V4 anchor-frame dataclass carries summary_matches/price for V13. +3. The ruff config lands in a sibling agent's file. POLL for it (re-check every + ~60s up to ~10 min); once present, run 'ruff check .' and resolve + EVERY newly-surfaced violation in the files you own (and scripts/) — fix or + noqa with a written justification. If a violation lands in a file you do NOT + own and is not derived_fields.py, report it as a blocker for the fix loop. +Run before returning: ruff check + ruff format --check on the files you touched, +and a targeted pytest of the WO Bedrock-fallback + PO validation-gate suites +(poll for the test agent's new files up to ~10 min). +${specBlock}`, + { label: 'impl:product-code', model: 'opus', phase: 'Implement', schema: IMPL }), + + () => agent(`${PREAMBLE} +YOU OWN: the ruff config file (pyproject.toml or ruff.toml — spec picks which), +pytest.ini, .github/workflows/ci.yaml, and README.md ONLY. You do NOT touch +tests or product code. +Task per spec.ruffConfig + spec.ciChanges + spec.readmeDocs: +1. Create the ruff config ENABLING C901/PLR with the pinned ceilings + (extract_new_po C901=35), scripts/ in scope, and the derived_fields.py + per-file-ignore with a justification comment (constraint 8). Write this + FIRST so the product-code agent can poll for it. +2. pytest.ini: remove the test_local.py exclusion comment (the file is being + deleted by the test agent); keep the three testpaths roots. +3. ci.yaml: add --cov with the spec's approach (explicit module list OR + __init__.py note), a fail-under, keep the AST bundle-consistency test in the + standard run, add scripts/ to the lint scope, keep the strict 'ci / ci' + required check, no admin bypass. +4. README + docstring drift batch (findings 33-38): the false "Data is never + corrupted" claims, stale web_ui invocation contract, the new test layout + (single loader, tests/support/, moved files), coverage/ruff-floor note. +Run before returning: ruff check . to confirm the config is +valid and to enumerate what it surfaces (report the list for the product-code +agent), and a yaml lint / dry parse of ci.yaml. Do NOT run the full suite (the +test agent owns that gate). +${specBlock}`, + { label: 'impl:config-ci-readme', model: 'sonnet', phase: 'Implement', schema: IMPL }), +]) + +const implOk = impl.filter(Boolean) +const implBlockers = implOk.flatMap(r => r.blockers || []) +log(`Implement complete: ${implOk.length}/3 agents, blockers: ${implBlockers.length}`) + +// ---------------------------------------------------- verify + fix loop + +const EXPECTED_SCOPE = [ + 'conftest.py', + 'tests/', + 'lambdas/po/email_processor/tests/', + 'lambdas/wo/email_processor/tests/', + 'lambdas/wo/email_processor/handler.py', + 'lambdas/po/email_processor/template_parser.py', + 'pyproject.toml', + 'ruff.toml', + 'pytest.ini', + '.github/workflows/ci.yaml', + 'README.md', +] + +const mechanicalPrompt = `${PREAMBLE} +Independent re-verification — trust nothing self-reported. Run ALL gates, +quoting failures verbatim: +1. pytest -q at repo root — all three roots collected and GREEN under the NEW + single loader. Then prove the loader actually runs: pytest lambdas/po alone + and pytest lambdas/wo alone each collect and pass (the new root conftest + must load for a standalone run — the whole reason it moved to repo root). +2. GOLDENS UNCHANGED: git diff ${BASE}...HEAD -- '**/expected/*.json' and the + .eml corpora must be empty (constraint 9). Any golden edit = FAIL. +3. ruff check . && ruff format --check . under the NEW config, WITH scripts/ in + scope. Zero violations (every surfaced one fixed or noqa'd-with-justification; + derived_fields.py handled via per-file-ignore, its file byte-unchanged: + git diff ${BASE}...HEAD -- lambdas/po/email_processor/derived_fields.py empty). +4. cd cdk && npx cdk synth po-ingest -q && npx cdk synth workorder-ingest -q + (artifact-id selectors) — both synth clean (no CDK change this phase, but a + broken import would surface here). +5. COVERAGE HONESTY: run the CI --cov invocation and CONFIRM the coverage report + lists lambdas/po/web_ui, lambdas/wo/web_ui and lambdas/po/site_extractor with + NON-zero, NON-omitted lines (prove they are no longer silently skipped for a + missing __init__.py). Confirm the fail-under is present and would actually + fail if tripped (has teeth). +6. WO NO-DOUBLE-COUNT PIN: run the specific test that computes the WO + Bedrock-error emitted series by hand and assert it passes; then git grep for + the reason-code rename and confirm no dashboard/metric-filter consumer of + 'malformed_site_code' was left dangling. +7. UNTOUCHABLES (each must output NOTHING): git diff ${BASE}...HEAD for + derived_fields.py, both ses_auth, both validate_ai_fallback gates, + lambdas/shared/, all cdk/*.py, site_extractor. The ONLY product diffs allowed + are lambdas/wo/email_processor/handler.py and + lambdas/po/email_processor/template_parser.py. +8. FILE MOVES + DELETION: test_po_merge.py + test_pad_zip.py are gone from + tests/ and present under lambdas/po/email_processor/tests/; test_local.py is + deleted; test_parse_raw_email.py + test_ses_auth.py still at root and green. +9. AST bundle-consistency test present in the standard run and green. +10. git status --porcelain scope check: every modified/added/deleted path under + ${EXPECTED_SCOPE.join(', ')} plus tests/support/ (untracked + .coverage/.claude/package/ tolerated). +passed=true only if all green. YOU MAY NOT edit files.` + +const lenses = [ + { key: 'loader-integrity', prompt: `${PREAMBLE} +ADVERSARIAL REVIEW — loader-integrity lens. ONE loader replaced three; attack +it. (1) Prove there is exactly ONE load path now and that the moto +BUILTIN_HANDLERS registration still happens BEFORE any handler import in EVERY +entry order (session start AND a standalone 'pytest lambdas/wo' run) — if a +handler's module-level boto3 client can be created before moto registers, the +moto-backed suites silently hit real AWS. (2) sys.modules isolation for +template_parser: run TWO cross-pipeline tests back to back (a PO golden then a +WO golden, and the reverse) and prove the save/restore still binds each handler +to its OWN template_parser — the dance did NOT shrink to nothing. (3) The file +moves (test_po_merge/test_pad_zip into the PO dir) must not break collection or +silently drop a test — count tests before/after. (4) Did retiring +_wo_parser_support.py's bare import leave any stale 'import handler' / +sys.path.insert that could re-introduce the collision? (5) load_golden's +parse_float=Decimal survived for PO (exact money) and did not corrupt WO. +confirmed=true only with file:line or a reproduced-failure.` }, + { key: 'wo-metric-contract', prompt: `${PREAMBLE} +ADVERSARIAL REVIEW — WO-metric-contract lens. The WO Bedrock-error metric-wrap +is the one behavior change; prove it does NOT break wo_stack's alarm contract. +Read the wrapped handler diff (git diff ${BASE}) and wo_stack's fallback-rate +MathExpression + the "a rejected email emits nothing else" alarm. By HAND, +enumerate the emitted metric series for: (a) a normal template parse, (b) a +gate-rejected email, (c) a Bedrock error (Throttling / missing-content / +empty-content / non-JSON). Prove NO series is double-counted — specifically that +the except-and-reraise emits ai_fallback + bedrock_error EXACTLY ONCE on a +Bedrock error and that a gate-rejected email still emits ONLY its rejected +datapoint and nothing else (a naive reorder would double-count — confirm this +was NOT done). Then the reason-code fix: confirm invalid_status no longer +collides with malformed_site_code and that no dashboard/metric-filter consumer +of the old code is left dangling. Confirm the handler event/return contract is +unchanged (no signature drift). confirmed=true only with the hand-computed +series tables as evidence.` }, + { key: 'coverage-honesty', prompt: `${PREAMBLE} +ADVERSARIAL REVIEW — coverage-honesty lens. The headline coverage historically +overstated reality because --cov=lambdas silently skipped web_ui/site_extractor +(missing __init__.py). Prove that is FIXED: run the new CI --cov invocation and +show lambdas/po/web_ui, lambdas/wo/web_ui, lambdas/po/site_extractor each appear +in the report with real measured lines (not omitted, not 0-of-0). Prove the +fail-under has TEETH — temporarily lower a threshold or add a trivially-uncovered +line on a scratch copy and show the job would fail (then discard). Prove the new +web_ui suites exercise the REAL auth module (fail-closed on unset ARN + on a +Secrets-Manager exception, 401 WITHOUT a table scan, non-ASCII token) and not a +mock that would pass with the gate deleted. Finally, confirm the ruff C901/PLR +config genuinely bites (extract_new_po at C901=35 is enforced, the previously +inert noqas are now live and justified, scripts/ is in scope, and +derived_fields.py is excluded via config not an edit). confirmed=true only with +command output as evidence.` }, +] + +let round = 0 +let checks = null +let confirmed = [] +while (round < 3) { + phase('Verify') + const results = await parallel([ + () => agent(mechanicalPrompt, { label: `verify:mechanical-r${round}`, model: 'sonnet', phase: 'Verify', schema: CHECKS }), + ...lenses.map(l => () => + agent(l.prompt, { label: `verify:${l.key}-r${round}`, phase: 'Verify', schema: FINDINGS })), + ]) + checks = results[0] + confirmed = results.slice(1).filter(Boolean) + .flatMap(r => r.findings || []) + .filter(f => f.confirmed && f.severity !== 'low') + const green = checks && checks.passed + log(`Verify round ${round}: mechanical ${checks && checks.passed ? 'GREEN' : 'RED'}, confirmed findings: ${confirmed.length}`) + if (green && confirmed.length === 0) break + + round += 1 + if (round >= 3) break + phase('Fix') + await agent(`${PREAMBLE} +You are the fix agent — you may edit files under: ${EXPECTED_SCOPE.join(', ')} +and tests/support/. Fix EVERY item below minimally; the binding spec and the +10 pinned constraints still hold (a finding that conflicts with a constraint is +reported, not "fixed" — the constraint wins, esp. constraint 9's untouchable +goldens + derived_fields freeze, and constraint 7's no-double-count metric +wrap: do NOT convert it to a naive reorder to silence a finding). Re-run the +specific failing gate/test per fix. +MECHANICAL:\n${checks ? checks.details : '(agent died — rerun all gates)'} +CONFIRMED FINDINGS:\n${JSON.stringify(confirmed, null, 2)} +${specBlock}`, + { label: `fix:round-${round}`, model: 'opus', phase: 'Fix', schema: IMPL }) +} + +const verifyClean = checks && checks.passed && confirmed.length === 0 +if (!verifyClean) { + return { + status: 'NEEDS ATTENTION — verify not clean after 3 rounds; branch left uncommitted', + branch: BRANCH, + mechanical: checks, + unresolvedFindings: confirmed, + implBlockers, + reconBlockers, + spec, + } +} + +// ----------------------------------------------------------------- package + +phase('Package') +const commit = await agent(`${PREAMBLE.replace('do NOT commit, ', '')} +YOU are the commit agent: +1. Read ~/Documents/repositories/seahaven/engineering-handbook/commit-messages.md + and follow it exactly. +2. git add only paths under: ${EXPECTED_SCOPE.join(', ')}, tests/support/, and + .claude/workflows/phase-8-test-consolidation.js. NOT .coverage, NOT package/. + Verify the staged set with git status — the test_local.py DELETION and the + git mv of test_po_merge.py / test_pad_zip.py MUST be staged too. +3. ONE commit; write the message to /tmp/phase8-commit-msg.txt and use + git commit -F /tmp/phase8-commit-msg.txt (backticks in -m get eaten by zsh). + Suggested subject: + "test: consolidate test roots — one repo-root loader, shared support package, missing-scenario suites, enforced ruff/coverage floor (refactor phase 8)" + Body: the single repo-root conftest loader + moto-before-handler carry, the + tests/support/ superset (parse_float=Decimal kept), the file moves + + test_local.py deletion, the new scenario suites (Bedrock transport, SES-auth + seam, web_ui both, WO merge, small pins), the WO Bedrock-error metric-wrap + (except-and-reraise, no double-count) + invalid_status reason-code fix, the + PO _validate_new_po_values split, the enforced ruff C901/PLR config, and the + CI --cov + fail-under floor. NO AI attribution / Co-Authored-By lines. +4. Do NOT push. Return commit sha + shortstat in summary.`, + { label: 'package:commit', model: 'sonnet', phase: 'Package', schema: IMPL }) + +return { + status: 'BUILT — committed locally, NOT pushed', + branch: BRANCH, + base: BASE, + commit: commit ? commit.summary : 'commit agent died — commit manually', + spec: { rootConftest: spec.rootConftest, productCodeEdits: spec.productCodeEdits, ruffConfig: spec.ruffConfig, ciChanges: spec.ciChanges, ownership: spec.ownership }, + implementation: implOk.map(r => r.summary), + filesChanged: implOk.flatMap(r => r.filesChanged), + verifyRounds: round + 1, + blockers: implBlockers.concat(reconBlockers), + outstandingGates: [ + '/sh-security-review RECOMMENDED before push — the WO Bedrock-error metric-wrap is a behavior change on the untrusted-input processing path, and web_ui auth is the mandatory-review surface (here only TESTS are added for it). Flag it for the WO metric change specifically; re-run after any post-review fix.', + 'cross-family cross_review.py NOT required — no IAM/policy change and no handler-signature change (the WO metric-wrap keeps the event/return contract). Opt-in only if judgment says so.', + 'deploy-then-merge for the WO handler metric/reason-code change: deploy from branch, smoke green, one live WO email, verify the ParseOutcome series still matches the alarm contract (NO double-count), and grep the dashboards for malformed_site_code BEFORE the reason-code rename ships.', + 'LAST PHASE: the new suite / coverage fail-under / enforced ruff C901/PLR config become the new CI floor — do not weaken any existing gate to land a later change.', + ], +} diff --git a/.coveragerc b/.coveragerc new file mode 100644 index 0000000..4246bb0 --- /dev/null +++ b/.coveragerc @@ -0,0 +1,4 @@ +[run] +omit = + */tests/* + */cdk.out/* diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f517699..3f816d8 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -7,7 +7,7 @@ jobs: ci: uses: Sea-Haven-Industries/.github/.github/workflows/ci-python-sam.yaml@fd60e4c9041784f666ac0fdefb9bec3c7fbf5143 # main with: - source-dirs: "lambdas cdk tests" + source-dirs: "lambdas cdk tests scripts" run-tests: true run-cdk-synth: true run-sam-validate: false diff --git a/.gitignore b/.gitignore index 14746c7..1bf4d63 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,4 @@ cdk.out/ lambdas/*/*/package/ .idea/ # JetBrains user settings directory +.coverage diff --git a/README.md b/README.md index 5d21c94..0067b50 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,7 @@ Coupa PO emails are received at `amazon_po@int.seahaven.com`, parsed **determini **Deterministic template parser.** `template_parser.try_deterministic_parse()` classifies by exact subject regex, extracts the nested contract (`supplier{}`, `ship_to{}` — 8 keys, `line_items[]` — 10 keys per item), and returns a parsed result **only if** it passes a fail-closed validation gate: recursive exact key-set at every nesting level; `email_type` emitted **only** on the exact new-PO subject *and* a confirmed-safe `Status` (never defaulted — a cancellation misrouted as `new_po` would defeat the sticky-`Cancelled` guard); `po_number` shape + byte-equality with the subject, the body `PO ID`, the `Amazon Purchase Order #` heading, and the `orders/` URL; duplicate-label anchor integrity (`Supplier`/`Shipping`/`Total` each appear twice, the first `Shipping` must be the literal `None` placeholder); money fidelity (every `Decimal` re-serializes byte-identically to its source token with a digit/comma border check — the thousands-separator-truncation kill switch — plus `sum(line amounts) == total`, proven against **both** `Total` blocks); bullet-metadata label discipline (closed label set, assigned by leading label, never ordinal — immune to the optional `Part Number` segment); USPS address shape on the raw pre-enrichment value; and rejection of any unparseable sentinel or residual `\r`/`\xa0` artifact. Multi-line-item (0.18%) and non-USD (0 observed) new-POs, comment emails, and anything else falls back to the AI extractor. The AI path is gated too: the untrusted email reaches Bedrock inside a neutralized `` data block (forged tag lookalikes in the body are defanged) with `temperature=0`, and the raw model output must pass the fail-closed `validate_ai_fallback()` gate — the PO-specific nested contract (exact key-set at every level, with missing keys normalized rather than rejected), a `po_number` shape check hardened against fullwidth-digit and trailing-artifact injection (the same regex family protecting the DynamoDB partition key the handler builds from it), an `email_type` allow-list enforced *before* dispatch so a miss can never fall into the `new_po` default branch, and `Decimal`/`int`/`None` money typing (PO decodes with `parse_float=Decimal`) — before any DynamoDB write. **The template path is gate-enforced end-to-end; the AI-fallback path is validated and fail-closed — output that fails the gate is skipped, never written (see `ai_fallback_rejected` below), so a malformed or injected email raises the fallback rate rather than corrupting a record.** Every record emits one CloudWatch EMF metric (see below). -**Derived fields (`site_code`, `trade`, `fiscal_year`).** These three are **classified**, not verbatim-extracted, so neither parse path emits them directly — instead a deterministic Python classifier (`derived_fields.derive_all()`) computes them inside the shared `enrich_parsed()` post-stage, which runs identically on both paths. `trade` is a closed label set (the same one the AI prompt enumerates): `Plumbing - PM`, `Plumbing - Reactive`, `Electrical`, `HVAC`, `Dock Doors`, `Doors`, `Signage`, `Carpentry`, `Fencing/Gates`, `Conveyance/MHE`, `Painting`, `Flooring`, `Janitorial`, `Fire/Life Safety`, `Landscaping/Yard`, `Roofing`, `Security/Locksmith`, `Snow Removal`, `PO Uplift`, `General Building - Emergency`, `General Building - Handyman`, `General Building - Project`, `General Building`. Authority differs by path: +**Derived fields (`site_code`, `trade`, `fiscal_year`).** These three are **classified**, not verbatim-extracted, so neither parse path emits them directly — instead a deterministic Python classifier (`derived_fields.derive_all()`) computes them inside the shared `enrich_parsed()` post-stage, which runs identically on both paths. `derived_fields.py` is byte-frozen this phase (shadow-bake freeze, per the constraint-9 module-diff gate); note for accuracy that its in-file docstring's "FAITHFUL v1 port" self-description is aspirational, not descriptive — the module is a deliberate **spec-superset** of the original `EXTRACTION_PROMPT` rule text (its own inline backtest-tuning comments diverge from those three prompt sections), with the LLM staying runtime-authoritative on the AI-fallback path during the bake regardless. The docstring reword is deferred to the first post-bake PR, since even a text-only edit is barred by this phase's machine-checked empty-diff gate on the file. `trade` is a closed label set (the same one the AI prompt enumerates): `Plumbing - PM`, `Plumbing - Reactive`, `Electrical`, `HVAC`, `Dock Doors`, `Doors`, `Signage`, `Carpentry`, `Fencing/Gates`, `Conveyance/MHE`, `Painting`, `Flooring`, `Janitorial`, `Fire/Life Safety`, `Landscaping/Yard`, `Roofing`, `Security/Locksmith`, `Snow Removal`, `PO Uplift`, `General Building - Emergency`, `General Building - Handyman`, `General Building - Project`, `General Building`. Authority differs by path: - **Template path** — the parser leaves all three `null`, so Python is authoritative: it fills them. - **AI-fallback path** — the LLM value stays **authoritative during the bake** (Python fills only a gap the LLM left `null`, never overwrites), and Python additionally runs in **shadow mode**: `enrich_parsed()` emits one `DerivedFieldAgreement` EMF record per field comparing the two. @@ -54,7 +54,7 @@ Or aggregate overall agreement per field: `| filter ispresent(DerivedFieldAgreem |---|---|---| | `po-email-processor` | S3 ObjectCreated | Claude extraction + DynamoDB write | | `po-ingest-site-extractor` | DynamoDB Streams | Site code/address extraction -> `verified-sites` | -| `po-web-ui` | Manual invoke | HTML dashboard (public Function URL removed 2026-06-08, INFRA-74) | +| `po-web-ui` | Manual invoke (authenticated — see Setup §6) | HTML dashboard (public Function URL removed 2026-06-08, INFRA-74) | **Tables:** - `purchase-orders` (PK: `po_number`, Streams: NEW_IMAGE) — shared with seahaven-slack-bot (read-only; see Shared Resources) @@ -80,7 +80,7 @@ Amazon APM work order emails (from Hexagon EAM / HxGN SmartCloud) are received a | Function | Trigger | Purpose | |---|---|---| | `workorder-email-processor` | S3 ObjectCreated | Claude extraction + DynamoDB write | -| `workorder-web-ui` | Manual invoke | HTML dashboard (public Function URL removed 2026-06-08, INFRA-74) | +| `workorder-web-ui` | Manual invoke (authenticated — see Setup §6) | HTML dashboard (public Function URL removed 2026-06-08, INFRA-74) | **Tables:** - `WorkOrders` (PK: `work_order_id`) — `site-code-index` and `status-index` GSIs removed 2026-06-03 (audit M-20) @@ -154,7 +154,9 @@ The `-duration` and `-throttles` alarms for `po-email-processor` and `wo **Parse-outcome metric + fallback-rate alarm (workorder-ingest):** the WO processor writes one CloudWatch **EMF** line per email to namespace `Seahaven/WorkorderIngest`, metric `ParseOutcome` (Unit Count, value 1), dimensioned by `ParseMethod` (`template` | `ai_fallback` | `ai_fallback_rejected`) and `TemplateId` (`update_plaintext` | `assign_html` | `unknown`). `ai_fallback_rejected` counts AI-fallback output that failed the fail-closed `validate_ai_fallback()` gate (schema/enum/date contract on raw Bedrock output — prompt-injection defence) and was dropped without a DynamoDB write. Non-dimension EMF properties `ReasonCode` and `work_order_id` are queryable in Logs Insights but not promoted to metrics (kept low-cardinality). EMF is used instead of `PutMetricData` so there is no extra sync call / latency / IAM grant on the async hot path (the role already has `logs:PutLogEvents`). The alarm `workorder-email-processor-template-fallback-rate` fires when the AI-fallback share of parses — rejected fallback parses included, so a drift outage whose AI output also fails the gate cannot lower the observed rate while dropping mail — exceeds **15%** sustained (a `MathExpression` with `FILL(...,0)` and a ≥10-sample volume floor over 15-minute periods, eval 3 / datapoints 2) — catching Hexagon template-drift coverage collapse while the volume floor + `FILL` prevent low-volume false pages / `INSUFFICIENT_DATA`. ALARM-only `SnsAction` to `site-alerts`, no OK action, `NOT_BREACHING`. The 15-minute period is a deliberate deviation from the 5-minute house style to accumulate a stable denominator at the low ~760/day volume. A second alarm, `workorder-email-processor-ai-fallback-rejected`, pages on the rejected series itself (≥1 rejection per 5-min period, 2 of the last 6 periods — the sender-auth-rejected sparse-arrival idiom) because a gate rejection drops mail without error/retry/DLQ and would otherwise be silent. -**Parse-outcome metric + fallback-rate alarm (po-ingest):** the PO processor emits the same EMF shape to namespace `Seahaven/PoIngest`, metric `ParseOutcome`, dimensioned by `ParseMethod` (`template` | `ai_fallback` | `ai_fallback_rejected`) and `TemplateId` (`coupa_new_po` | `coupa_cancellation` | `unknown`), with `ReasonCode` (the fail-closed gate reason) and `po_number` as Logs-Insights ride-alongs. `ai_fallback_rejected` counts AI-fallback output that failed the fail-closed `validate_ai_fallback()` gate (nested key-set contract, `po_number` shape, `email_type` allow-list, `Decimal` money typing) and was dropped without a DynamoDB write. +**WO Bedrock transport-error metric (Phase 8).** A Bedrock-side transport error (throttling, malformed response, non-JSON model text) during the AI-fallback attempt previously emitted **zero** `ParseOutcome` datapoints — the only emit sites were post-gate. `handler.py` now wraps the `extract_with_bedrock` call in a try/except that emits exactly one `ParseMethod=ai_fallback` / `ReasonCode=bedrock_error` datapoint and then re-raises (the exception still propagates into the errors alarm / DLQ path unchanged). This is an except-and-reraise, not a reorder: a gate-rejected email (Bedrock *returns* successfully, `validate_ai_fallback()` then rejects it) still emits only the single `ai_fallback_rejected` datapoint and nothing else — the except branch never fires because Bedrock did not raise — so the `workorder-email-processor-ai-fallback-rejected` "a rejected email emits nothing else" alarm contract holds with no double-count. + +**Parse-outcome metric + fallback-rate alarm (po-ingest):** the PO processor emits the same EMF shape to namespace `Seahaven/PoIngest`, metric `ParseOutcome`, dimensioned by `ParseMethod` (`template` | `ai_fallback` | `ai_fallback_rejected`) and `TemplateId` (`coupa_new_po` | `coupa_cancellation` | `unknown`), with `ReasonCode` (the fail-closed gate reason) and `po_number` as Logs-Insights ride-alongs. `ai_fallback_rejected` counts AI-fallback output that failed the fail-closed `validate_ai_fallback()` gate (nested key-set contract, `po_number` shape, `email_type` allow-list, `Decimal` money typing) and was dropped without a DynamoDB write. The `ai_fallback_rejected` emission's `po_number` ride-along is clamped to 64 chars (`handler.py:118`, pinned by the PO-DC-02 regression test) — `telemetry.py`'s `DerivedFieldAgreement` `PythonValue`/`LlmValue` properties clamp the same way. **Deliberate double-count:** unlike WO, PO emits `ParseMethod=ai_fallback` *before* the Bedrock call (so a Bedrock-side error still records the outcome) — a rejected email therefore always emits **both** an `ai_fallback` datapoint (pre-call) and an `ai_fallback_rejected` datapoint (post-gate), never just the latter. This is intentional and load-bearing, not a bug; the fallback-rate math below treats `fb` as already inclusive of every rejection. @@ -284,6 +286,8 @@ Both currently use default DynamoDB encryption — they are **not** yet on the s **Consumer (read-only) — data contract:** `seahaven-slack-bot` imports both tables via `Table.fromTableName(...)` (`grantReadData`) and reads them from two Lambdas: `workorder-sync` (daily full-table scan into the Bedrock knowledge base) and `wo-po-lookup` (the Bedrock agent's direct WO lookup action group). The bot depends on the PK/SK schema above and these attributes: on `WorkOrders` — `description`, `wo_status`, `customer`, `site_code`, `building`, `address`, `severity`, `priority`, `assigned_to`, `date_reported`, `scheduled_start`, `due_date`, `updated_at`; on `WorkOrderComments` — `created_at` (used to sort comments), `commenter`, `text`. Any change to table name, key schema, these attribute names, or the encryption configuration (e.g. the INFRA-6 CMK migration) must be coordinated with `seahaven-slack-bot` before it ships, or the Bedrock agent breaks at runtime (not at deploy — the tables are imported by name, so there is no compile-time link). +**Stream field contract (`site_code`).** Three definitions of "is this a valid site code" have existed in this repo at once. The canonical shape is `derived_fields._STRICT_CODE_RE` (`[A-Z][A-Z0-9]{2,4}`, `fullmatch`) plus its skip-list semantics (a code-shaped token is only a real site code if it is *not* skip-listed, e.g. `LLC`/`INC`/`CORP`/`LTD`/`ATTN`), used by `derive_site_code()` (the `enrich_parsed()` classifier, see the **Derived fields** note in the Purchase Orders flow above). Honestly noted, not papered over: `lambdas/po/site_extractor/handler.py`'s own `SITE_CODE_PATTERN` (`[A-Z]{2,4}\d{1,2}`, prefix-anchored `.match`, digit-requiring) still diverges from the canonical shape as of this phase — it rejects valid all-letter codes like `KLAL` (which surface as permanent `pending-site-review` rows) and accepts overlong junk like `DLI6X`/`SNY55` that the canonical `fullmatch` would not. Reconciling `po-ingest-site-extractor`'s direct-field validation onto the canonical `derived_fields` shape is Phase 6 scope, tracked separately — this paragraph will be updated when it lands. + ### `verified-sites` table (owned here) Owned by this repo's `po-ingest` stack (`cdk/po_stack.py`). PK `siteCode` (S); default DynamoDB encryption (NOT the shared CMK). @@ -328,24 +332,34 @@ Branch protection on `main` — all changes through PR. aws secretsmanager delete-secret --secret-id po-ingest/anthropic-api-key --force-delete-without-recovery aws secretsmanager delete-secret --secret-id workorder-ingest/anthropic-api-key --force-delete-without-recovery ``` -6. Dashboards: `po-web-ui` and `workorder-web-ui` have no public endpoint (the Function URLs were removed 2026-06-08, INFRA-74). Invoke them through an authenticated path that forwards the `X-Auth-Token` header, e.g. `aws lambda invoke --function-name po-web-ui /tmp/out.json`. +6. Dashboards: `po-web-ui` and `workorder-web-ui` have no public endpoint (the Function URLs were removed 2026-06-08, INFRA-74). A bare `aws lambda invoke --function-name po-web-ui /tmp/out.json` with no headers in the event is guaranteed a `401` — the handler fails closed (see [Web UI auth](#security)). Fetch the token and forward it in the event's `headers`: + ```bash + TOKEN=$(aws secretsmanager get-secret-value --secret-id procurement-ingest/web-ui-auth-token --query SecretString --output text) + aws lambda invoke --function-name po-web-ui --payload "{\"headers\":{\"x-auth-token\":\"$TOKEN\"}}" /tmp/out.json + ``` ## Tests Offline unit tests (no AWS, no network) run via pytest from the repo root: ```bash -pip install pytest +pip install -r tests/requirements.txt pytest ``` +**Test-root consolidation (Phase 8).** Both test roots are kept (`tests/` for genuinely cross-pipeline suites, plus each pipeline's own `lambdas/*/email_processor/tests/`), but loading is now single-sourced. A new repo-root `conftest.py` — not `tests/conftest.py`, which is never an ancestor of the pipeline test roots and so cannot load for a standalone `pytest lambdas/po/email_processor/tests` run — sets the dummy AWS env, imports `moto` before any handler import (registers moto's botocore stubber hook so boto3 sessions created afterwards are stubbable), and exposes the one `load_lambda_module(pipeline, name)` loader (implemented in `tests/support/loader.py`) that every handler exec in the suite goes through, including its `sys.modules` save/restore dance for the per-pipeline duplicated bare names (`template_parser`, `persistence`, etc. — `derived_fields`/`prompts`/`telemetry`/`extraction`/`enrichment` too). `tests/support/` is a small shared package (`FakeTable`, `FakeDynamoResource`, `FakeS3`, `load_email`, `load_golden`) used by both pipelines' `_po_parser_support.py`/`_wo_parser_support.py` shims — `load_golden` decodes JSON money with `parse_float=Decimal` unconditionally (load-bearing for PO's exact-money goldens; proven safe for WO, whose 55 golden fixtures contain zero float-typed JSON numbers). `test_local.py` (a root-level manual script that imported a handler at collection time, bypassing the loader gate, and globbed a nonexistent `samples/` dir) is deleted — the golden suites cover its role. + Coverage: -- `tests/` — shared handler + cross-pipeline tests: `parse_raw_email` and `pad_zip`, the PO merge-write semantics (`test_po_merge.py`, #97, moto-backed), the fail-closed sender-authentication parser (`test_ses_auth.py`, INFRA-107), and the Phase 0 CDK-bundling/handler-import AST consistency check (`test_bundle_consistency.py` — see [Deploy-Pipeline Guards](#deploy-pipeline-guards-phase-0)). -- `lambdas/wo/email_processor/tests/` — the deterministic WO parser suite: golden-file tests over 55 real scrubbed `.eml` fixtures (`test_parser.py`), fail-closed validation-gate rules and adversarial/injection cases (`test_validation_gate.py`), the issue #23 `comment_id` idempotency invariants (`test_comment_id.py`), the Bedrock-fallback dispatch/EMF-metric behavior with a mocked `invoke_model` (`test_bedrock_fallback.py`, which also carries the Phase 5 behavior pins — WO's mutually-exclusive emit and the `[0-9]+` key guard ahead of both saves), and the Phase 0 direct-invoke healthcheck contract (`test_healthcheck.py`). Golden JSON lives under `tests/fixtures/expected/`. After Phase 5 the suite patches accessors on the owning siblings (`extraction.bedrock`, `persistence.dynamodb`, `persistence.datetime`, `persistence.save_*`) rather than on `handler`. -- `lambdas/po/email_processor/tests/` — the deterministic PO parser suite: golden-file tests over real scrubbed `.eml` fixtures (17 single-line new-PO + 8 cancellations, exact `Decimal`-aware golden comparison via `parse_float=Decimal`), fail-closed validation-gate coverage for **every** gate reason code (fixture-driven for body-level triggers under `fixtures/adversarial/`, direct `validate()` unit tests for candidate-level mutations), real multi-line and comment/non-Coupa fallback fixtures under `fixtures/ai-fallback/`, dual line-ending (CRLF/LF) parse-identity, two-path `enrich_parsed`/`save_new_po` parity (the site-extractor stream-contract guard), fixture hygiene (`ses_auth` pass + scrub-marker leak sweep), the Bedrock-fallback dispatch/EMF-metric behavior plus the Phase 5 behavior pins (`test_po_bedrock_fallback.py` — the pre-Bedrock `ai_fallback` emit and the additive rejected double-count), the `_save_merge` collapse parity to both `save_new_po`/`save_revision` (`test_po_save_merge_parity.py`), the `ai_fallback`-only shadow telemetry after the `enrich_parsed` move (`test_po_derived_wiring.py`), and the Phase 0 direct-invoke healthcheck contract (`test_po_healthcheck.py`). After Phase 5 the suite patches accessors/constants on the owning siblings (`po_extraction.bedrock`/`.BEDROCK_MODEL_ID`/`._EMAIL_TAG_RE`, `po_persistence.dynamodb`/`.PO_TABLE`, `po_enrichment.derive_all`/`.pad_zip`, `po_telemetry.DERIVED_METRIC_NAME`) rather than on `po_handler`; re-exported names it calls (`save_*`, `extract_with_claude`, `enrich_parsed`, `_emit_parse_method_metric`, `EXTRACTION_PROMPT`) stay on `po_handler`. +- `tests/` — shared handler + cross-pipeline tests: `parse_raw_email` (`test_parse_raw_email.py`), the fail-closed sender-authentication parser (`test_ses_auth.py`, INFRA-107), the Phase 0 CDK-bundling/handler-import AST consistency check (`test_bundle_consistency.py` — see [Deploy-Pipeline Guards](#deploy-pipeline-guards-phase-0)), the `scripts/reprocess.py` synthetic S3-event-shape contract (`test_reprocess_contract.py`, Phase 7), and, new in Phase 8: the handler-level SES-auth reject seam per pipeline (`test_handler_auth_seam.py` — no auth monkeypatch + an empty `ALLOWED_DKIM_DOMAINS` must yield zero Bedrock calls, zero writes, no raise), and both web_ui functions' first coverage — fail-closed auth (`test_web_ui_auth.py`) and the handlers themselves, including a 401-without-a-table-scan assertion and a hostile-field escaping regression lock (`test_web_ui_handlers.py`). +- `lambdas/wo/email_processor/tests/` — the deterministic WO parser suite: golden-file tests over 55 real scrubbed `.eml` fixtures (`test_parser.py`), fail-closed validation-gate rules and adversarial/injection cases (`test_validation_gate.py`), the issue #23 `comment_id` idempotency invariants (`test_comment_id.py`), the Bedrock-fallback dispatch/EMF-metric behavior with a mocked `invoke_model` (`test_bedrock_fallback.py`, which also carries the Phase 5 behavior pins — WO's mutually-exclusive emit and the `[0-9]+` key guard ahead of both saves), and the Phase 0 direct-invoke healthcheck contract (`test_healthcheck.py`). Golden JSON lives under `tests/fixtures/expected/`. New in Phase 8: `test_wo_bedrock_transport.py` (transport errors — throttle, missing `content`, empty content, non-JSON model text — plus the hand-computed no-double-count pin for the `handler.py` except-and-reraise metric wrap, and the multi-record failure-isolation pin) and `test_wo_merge.py` (a moto-backed mirror of `test_po_merge.py` against the `WorkOrders` table — null-status never clobbers `wo_status`, `created_at` immutable, `status`→`wo_status` mapping, `None` fields absent from `SET`, `record_type` only-when-present). After Phase 5 the suite patches accessors on the owning siblings (`extraction.bedrock`, `persistence.dynamodb`, `persistence.datetime`, `persistence.save_*`) rather than on `handler`. +- `lambdas/po/email_processor/tests/` — the deterministic PO parser suite: golden-file tests over real scrubbed `.eml` fixtures (17 single-line new-PO + 8 cancellations, exact `Decimal`-aware golden comparison via `parse_float=Decimal`), fail-closed validation-gate coverage for **every** gate reason code (fixture-driven for body-level triggers under `fixtures/adversarial/`, direct `validate()` unit tests for candidate-level mutations), real multi-line and comment/non-Coupa fallback fixtures under `fixtures/ai-fallback/`, dual line-ending (CRLF/LF) parse-identity, two-path `enrich_parsed`/`save_new_po` parity (the site-extractor stream-contract guard), fixture hygiene (`ses_auth` pass + scrub-marker leak sweep), the Bedrock-fallback dispatch/EMF-metric behavior plus the Phase 5 behavior pins (`test_po_bedrock_fallback.py` — the pre-Bedrock `ai_fallback` emit and the additive rejected double-count), the `_save_merge` collapse parity to both `save_new_po`/`save_revision` (`test_po_save_merge_parity.py`), the `ai_fallback`-only shadow telemetry after the `enrich_parsed` move (`test_po_derived_wiring.py`, which gains the PO-DC-02 64-char `po_number` clamp regression pin in Phase 8), and the Phase 0 direct-invoke healthcheck contract (`test_po_healthcheck.py`). New in Phase 8: `test_po_bedrock_transport.py` (the same four transport-error cases as WO, asserting the pre-call `ai_fallback` metric survives and no partial write occurs, plus the multi-record failure-isolation pin) and, moved here from `tests/`: `test_po_merge.py` (#97, moto-backed merge-write semantics) and `test_pad_zip.py` (zip-code padding). After Phase 5 the suite patches accessors/constants on the owning siblings (`po_extraction.bedrock`/`.BEDROCK_MODEL_ID`/`._EMAIL_TAG_RE`, `po_persistence.dynamodb`/`.PO_TABLE`, `po_enrichment.derive_all`/`.pad_zip`, `po_telemetry.DERIVED_METRIC_NAME`) rather than on `po_handler`; re-exported names it calls (`save_*`, `extract_with_claude`, `enrich_parsed`, `_emit_parse_method_metric`, `EXTRACTION_PROMPT`) stay on `po_handler`. All three roots are discovered by `pytest.ini` (`testpaths`). +**Coverage floor (Phase 8).** `pytest.ini`'s `addopts` runs `pytest-cov` with an explicit `--cov` path per first-party package (`lambdas/po/email_processor`, `lambdas/wo/email_processor`, `lambdas/po/web_ui`, `lambdas/wo/web_ui`, `lambdas/po/site_extractor`, `lambdas/shared`) rather than relying on an `__init__.py` package marker — none of these dirs have one, and adding one would perturb the CDK bundling asset-hash fingerprint for zero runtime benefit; `pytest-cov`'s path form measures by source file regardless of package markers. `site_extractor` is deliberately included even though it measures 0% until Phase 6 lands — coverage honesty, not a silent skip. `.coveragerc` omits `*/tests/*` and `*/cdk.out/*` so the pipeline test dirs (which sit inside their own `--cov` path) don't dilute the number. `--cov-fail-under` is the new permanent CI floor (constraint 10 — never ratcheted down). + +**Ruff C901/PLR floor (Phase 8).** `ruff.toml` adds `extend-select = ["C901", "PLR"]` with `max-complexity = 12`, making the tree's pre-existing `# noqa: PLR09xx` suppressions load-bearing instead of inert (no config previously enabled the rules they suppressed). `scripts/` is in the lint scope. Findings that could not be split or were out of this phase's file-ownership were resolved with a per-file `[lint.per-file-ignores]` entry carrying a written justification — most notably `derived_fields.py` (shadow-bake freeze: even an in-file `noqa` comment is a barred edit) and the other Phase 3/5/6/7 frozen modules (`enrichment.py`, WO `template_parser.py`, both `ses_auth.py`/`emf.py`, `site_extractor/handler.py`, `scripts/reprocess.py`). PO's `template_parser.py` — this phase's one in-scope split target — clears the ceiling by decomposing `extract_new_po` and `_validate_new_po_values` into per-rule helpers instead of an ignore. + ## Scripts **Reprocess emails** (re-invoke an email-processor with a synthetic S3 event). `reprocess.py` is now **pipeline-general**: `--pipeline po|wo` selects the function + raw-email bucket. **Targeted replay** (`--key` one object, `--prefix`, or `--since` a `LastModified` timestamp) is the default, preferred mode; the full-prefix sweep is demoted behind an explicit `--all`. Every mode is dry-run unless `--execute`. See the [DLQ recovery runbook](docs/runbook-dlq-recovery.md) for the targeted single-key re-invoke flow. @@ -393,34 +407,78 @@ lambdas/ # Phase 2: shared Code.from_asset("../lambdas") bundling telemetry.py # EMF ParseMethod + DerivedFieldAgreement emit wrappers (stdout EMF; no boto3) persistence.py # _write_fields/_merge_update + save_cancellation; save_new_po/save_revision collapsed into one behavior-identical _save_merge (sticky-cancel guard intact); lazy dynamodb accessor prompts.py # EXTRACTION_PROMPT (~181 lines); cross-ref header to derived_fields' rule tables - template_parser.py # pure deterministic Coupa parser + fail-closed validation gate - derived_fields.py # deterministic site_code/trade/fiscal_year classifier (untouched by Phase 3/5) + template_parser.py # pure deterministic Coupa parser + fail-closed validation gate; Phase 8: + # extract_new_po/_validate_new_po_values split into C901/PLR-clean per-rule + # helpers (behavior byte-identical; see the ruff C901/PLR floor below) + derived_fields.py # deterministic site_code/trade/fiscal_year classifier (untouched by Phase 3/5/8 -- shadow-bake freeze; ruff per-file-ignore instead of an in-file noqa) tests/ # golden-file + validation-gate + fallback-dispatch + healthcheck + save-merge-parity + behavior-pin tests + fixtures + conftest.py # fake_dynamo fixture, patches persistence.dynamodb + _po_parser_support.py # Phase 8: thin shim -- re-exports tests.support fakes/loader; PO module handles (template_parser, prompts, telemetry, extraction, enrichment, persistence) + test_po_derived_fields.py # derived_fields.derive_all() unit tests + test_po_derived_wiring.py # ai_fallback-only shadow-telemetry wiring; Phase 8 gains the PO-DC-02 64-char po_number clamp regression pin + test_po_bedrock_transport.py # Phase 8: Bedrock transport errors (throttle/missing-content/empty-content/non-JSON) + pre-call metric survival + multi-record failure-isolation pin + test_po_merge.py # Phase 8: moved from tests/ -- PO merge-write semantics tests (#97), moto-backed + test_pad_zip.py # Phase 8: moved from tests/ -- PO zip-code padding tests site_extractor/ web_ui/ # handler.py imports `from web_ui_auth import is_authenticated` (shared) wo/ # WO pipeline Lambdas email_processor/ # Phase 5: decomposed into ~5 flat siblings (no enrichment stage); # validate_ai_fallback + the [0-9]+ work_order_id key guard stay in # the handler loop AHEAD of both saves - handler.py # event loop + fail-closed SES auth + [0-9]+ work_order_id key guard + {"healthcheck": true} early-return; lazy s3 accessor; re-exports EXTRACTION_PROMPT + handler.py # event loop + fail-closed SES auth + [0-9]+ work_order_id key guard + {"healthcheck": true} early-return; lazy s3 accessor; re-exports EXTRACTION_PROMPT; Phase 8: the Bedrock call is wrapped in try/except so a transport error emits one ai_fallback/bedrock_error datapoint then re-raises (a gate rejection still emits only ai_fallback_rejected -- no double-count) extraction.py # extract_with_bedrock + _EMAIL_TAG_RE + EXTRACTION_PROMPT import; lazy bedrock accessor telemetry.py # emit_parse_metric EMF wrapper (stdout EMF; no boto3) persistence.py # save_work_order + _header_date_iso + save_event (#23 comment_id determinism kept WITH persistence); lazy dynamodb accessor prompts.py # EXTRACTION_PROMPT; cross-ref header to template_parser.CONTRACT_KEYS - template_parser.py # pure deterministic parser + fail-closed validation gate + template_parser.py # pure deterministic parser + fail-closed validation gate; Phase 8: the + # template-path bad-status branch now returns "invalid_status" (was a + # copy-paste "malformed_site_code") -- matches validate_ai_fallback's code + # for the identical condition tests/ # golden-file + validation-gate + comment_id + fallback + healthcheck + behavior-pin tests + conftest.py # fake_dynamo fixture, patches persistence.dynamodb (module-top `from _wo_parser_support import wo_persistence`, Phase 8) + _wo_parser_support.py # Phase 8: rewritten OFF the bare `import handler` strategy -- re-exports tests.support fakes/loader; WO module handles (template_parser, persistence, extraction, telemetry) + test_wo_merge.py # Phase 8: moto-backed WorkOrders save_work_order merge-semantics mirror of test_po_merge (null-status never clobbers wo_status, created_at immutable, status->wo_status mapping, None fields absent, record_type only-when-present) + test_wo_bedrock_transport.py # Phase 8: Bedrock transport errors + the hand-computed emitted-series no-double-count pin for the handler.py except-and-reraise wrap + multi-record failure-isolation pin + fixtures/ + ses-stamped/ + auth-pass-01.eml # Phase 8: the one new fixture allowed this phase -- a synthesized Authentication-Results header block (from WO_SES_HEADER in test_ses_auth.py), not scraped mail web_ui/ # handler.py imports `from web_ui_auth import is_authenticated` (shared) scripts/ reprocess.py backfill_sites.py post-deploy-smoke.sh # CD gate: synchronous healthcheck invoke of both processors, checks FunctionError -test_local.py # Parse sample emails through Bedrock locally (no AWS mutation) +conftest.py # Phase 8: THE repo-root session-invariant conftest. rootdir is pinned by + # pytest.ini at repo root, so this loads for every pytest invocation shape -- + # including a standalone `pytest lambdas/po/email_processor/tests` -- before + # any collection import. Sets the dummy AWS env, imports moto BEFORE any + # handler import (registers moto's botocore stubber hook so boto3 sessions + # created afterwards are stubbable), and re-exports + # `tests.support.loader.load_lambda_module`. Owns the session fixtures + # (po_handler, wo_handler, email_handler, ses_auth, po_persistence, + # po_enrichment, wo_persistence). Supersedes the old tests/conftest.py + # (deleted -- it was never an ancestor of the pipeline test roots, so it + # could not carry these invariants for a standalone pipeline-root run). tests/ - requirements.txt # Test-only deps (moto) - conftest.py # AWS env stubs + module loader (per-pipeline + Phase 5 flat siblings, dependency-ordered, + shared/ fallback); po_persistence/po_enrichment handles for the root PO tests - test_pad_zip.py # PO zip-code padding tests - test_parse_raw_email.py # MIME parsing tests (PO + WO handlers) - test_po_merge.py # PO merge-write semantics tests (#97) - test_ses_auth.py # Sender-authentication parser tests (INFRA-107) - test_bundle_consistency.py # AST check: bundling command ships every handler.py sibling import + requirements.txt # Test-only deps (moto, pytest-cov) + support/ # Phase 8: shared test package (tests/ itself has no __init__.py -- + # PEP-420 namespace resolution via the root conftest's sys.path insert) + __init__.py # load_lambda_module + REPO_ROOT re-export, FIXTURE_DIRS, stems/ + # load_raw/load_email/load_golden (parse_float=Decimal, pinned safe + # for both pipelines), and the superset FakeTable/FakeDynamoResource/FakeS3 + loader.py # the ONE load_lambda_module + sys.modules save/restore dance + # (dependency-topological sibling tuple + web_ui_auth); every + # pipeline handler exec in the suite goes through this + test_parse_raw_email.py # MIME parsing tests (PO + WO handlers) -- stays at root, cross-pipeline + test_ses_auth.py # Sender-authentication parser tests (INFRA-107) -- stays at root, cross-pipeline + test_handler_auth_seam.py # Phase 8: handler-level SES-auth reject seam, per pipeline -- no auth + # monkeypatch + empty ALLOWED_DKIM_DOMAINS -> zero Bedrock calls, zero + # writes, no raise (closes the "delete the gate line, tests still pass" hole) + test_web_ui_auth.py # Phase 8: shared/web_ui_auth.py -- fail-closed on unset ARN / Secrets + # Manager exception, TTL cache refresh, header-matrix case-insensitivity, + # non-ASCII-token documenting pin + test_web_ui_handlers.py # Phase 8: both web_ui handlers (0% coverage before this phase) -- 401 + # without a table scan, authenticated render path, hostile-field escaping + # regression lock + test_bundle_consistency.py # AST check: bundling command ships every handler.py sibling import + test_reprocess_contract.py # reprocess.py synthetic S3-event-shape contract (Phase 7) ``` diff --git a/conftest.py b/conftest.py new file mode 100644 index 0000000..a8ddd23 --- /dev/null +++ b/conftest.py @@ -0,0 +1,112 @@ +"""Repo-root pytest configuration -- the ONE loader of session invariants. + +pytest's rootdir is pinned by pytest.ini at the repo root, so THIS conftest +loads for EVERY invocation, including a standalone +``pytest lambdas/po/email_processor/tests`` run: rootdir conftests load at +session start, before any collection import. ``tests/conftest.py`` is NOT an +ancestor of the pipeline test roots, which is exactly why it cannot carry the +session invariants below. + +The Lambda handlers create boto3 clients lazily on first use, but the moto +stubber hook must still be registered before any client is ever constructed, so +this conftest sets the dummy AWS env and imports moto BEFORE anything loads a +handler. +""" + +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent + +# The handlers build boto3 clients on first call; give boto3 a region and dummy +# credentials so client construction works offline / under CI regardless of the +# ambient environment. +import os # noqa: E402 + +os.environ.setdefault("AWS_DEFAULT_REGION", "us-east-1") +os.environ.setdefault("AWS_ACCESS_KEY_ID", "testing") +os.environ.setdefault("AWS_SECRET_ACCESS_KEY", "testing") +os.environ.setdefault("AWS_SESSION_TOKEN", "testing") + +# Imported for its side effect and DELIBERATELY BEFORE the handler modules are +# loaded below: moto registers its botocore stubber hook into botocore's +# BUILTIN_HANDLERS at import time, and boto3 sessions only pick the hook up if +# they are created AFTER that registration. This conftest chain is imported at +# pytest session start -- before tests/test_po_merge.py gets a chance to import +# moto -- so without this import the handler's module-level boto3 clients would +# be created un-stubbable and the moto-backed suites would hit real AWS. +import moto # noqa: F401,E402 + +# Put the repo root on sys.path so ``tests.support`` resolves as a PEP-420 +# namespace package from every invocation directory. +if str(REPO_ROOT) not in sys.path: + sys.path.insert(0, str(REPO_ROOT)) + +# The single loader lives in tests/support/loader.py; the root conftest exposes +# it (constraint 1). Re-exported rather than reimplemented -- `from conftest +# import ...` is unsafe with three conftest.py files across the roots. +from tests.support.loader import load_lambda_module # noqa: E402,F401 + + +@pytest.fixture(scope="session") +def po_handler(): + """The PO email processor handler module.""" + return load_lambda_module("po", "email_processor/handler") + + +@pytest.fixture(scope="session") +def wo_handler(): + """The WO email processor handler module.""" + return load_lambda_module("wo", "email_processor/handler") + + +@pytest.fixture(scope="session") +def po_persistence(po_handler): + """The PO persistence sibling (owns PO_TABLE + save_* after Phase 5). + + load_lambda_module registers each sibling under ``__`` in + sys.modules (only the bare-name binding is restored afterwards), so the + persistence module stays reachable here by its unique name. test_po_merge.py + keys its moto table on ``po_persistence.PO_TABLE`` rather than a literal. + """ + return sys.modules["po_email_processor_handler__persistence"] + + +@pytest.fixture(scope="session") +def po_enrichment(po_handler): + """The PO enrichment sibling (owns pad_zip after Phase 5). + + pad_zip is PURE and NOT re-exported by handler (handler's body never calls + it -- it runs inside enrich_parsed), so test_pad_zip.py dereferences it on + the owning module instead of po_handler. + """ + return sys.modules["po_email_processor_handler__enrichment"] + + +@pytest.fixture(scope="session") +def wo_persistence(wo_handler): + """The WO persistence sibling (owns save_work_order / save_event). + + Reachable by its unique sys.modules name for test_wo_merge symmetry with + po_persistence. + """ + return sys.modules["wo_email_processor_handler__persistence"] + + +@pytest.fixture(params=["po_handler", "wo_handler"]) +def email_handler(request): + """Parametrized fixture yielding each email processor handler module.""" + return request.getfixturevalue(request.param) + + +@pytest.fixture +def ses_auth(): + """The single-sourced ses_auth module (lambdas/shared/, Phase 3). + + Previously parameterized over the two per-pipeline copies to prove they + stayed byte-identical; now there is exactly one copy, so this loads it + once -- halving the test_ses_auth run. + """ + return load_lambda_module("shared", "ses_auth") diff --git a/docs/po-template-parser.md b/docs/po-template-parser.md index 6f9680b..7c087e2 100644 --- a/docs/po-template-parser.md +++ b/docs/po-template-parser.md @@ -256,7 +256,7 @@ Plus `missing_required_field` for the required labeled fields (per-item metadata - **`ship_to.street`/`ship_to.address` join convention:** multi-line segments joined with `'\n'`; `address` = the lines from `ship_to.name` through `United States` inclusive (this is what `enrich_parsed` promotes to `ship_to_raw` on the site-extractor stream). - **`quantity`/`unit`/`price` source:** the Items-summary ` x ` line (e.g. `1.0 EACH x 55,206.00`); the Lines-block ` EA` line is gate evidence only (V13 numeric cross-check). Emails without a summary line (e.g. new-po-14) leave all three `null` — nullable by contract. - **V10 URL-id sweep outcome:** all 20 harvested new_po emails satisfy `orders/ == po_number` digit suffix — the strict V10 check stays live (no demotion needed); a corpus-sweep test locks it. -- **Canonical `quantity`/`price` type = Decimal (DynamoDB Number), converged in the shared `enrich_parsed`:** `EXTRACTION_PROMPT` declares both fields as JSON strings, so a prompt-obedient Bedrock response arrives as `str` where the template path emits `Decimal`. The shared post-stage coerces numeric strings (thousands-separator-safe) to `Decimal` so both paths write the same attribute type to the purchase-orders table stream; non-numeric strings are left verbatim. Prompt rewording itself stays PR #2 scope. The two-path parity test feeds a prompt-shaped payload (string quantity/price, LLM-filled `site_code`) — never the parser-derived golden verbatim — so it cannot be circular. +- **Canonical `quantity`/`price` type = Decimal (DynamoDB Number), converged in the shared `enrich_parsed`:** `EXTRACTION_PROMPT` declares both fields as JSON `"number or null"` (see the PR #2 prompt tweak above — `prompts.py:67-69`), not JSON strings; `parse_float=Decimal` already handles a conforming numeric response the same as the template path's `Decimal`. The shared post-stage's numeric-string-to-`Decimal` coercion (thousands-separator-safe) remains as a defensive net for a non-conforming model response that arrives as `str` anyway; non-numeric strings are left verbatim. `prompts.py` itself is frozen this phase — this is a doc-only correction. The two-path parity test feeds a prompt-shaped payload (string quantity/price, LLM-filled `site_code`) — never the parser-derived golden verbatim — so it cannot be circular. - **Second-pass header scrub (post-review):** the first-pass harvest scrub left the real SES `Feedback-ID` sender-identity hash on every Coupa fixture and, on the two non-Coupa fixtures, an embedded second SES block's `X-Ses-Receipt`, the Exchange cross-tenant UPN ciphertext, Gmail ARC `fh=` / `X-Gm-*` tokens, and related opaque routing blobs. All replaced with same-shape `ScrubbedFixture` placeholders (byte-safe, CRLF preserved); the fixture-hygiene test now asserts these token classes are scrubbed in every header block so regressions are caught. --- diff --git a/lambdas/po/email_processor/template_parser.py b/lambdas/po/email_processor/template_parser.py index fb95cd3..c985112 100644 --- a/lambdas/po/email_processor/template_parser.py +++ b/lambdas/po/email_processor/template_parser.py @@ -45,6 +45,7 @@ template_id, reason_code). """ import re +from dataclasses import dataclass from decimal import Decimal, InvalidOperation # --------------------------------------------------------------------------- @@ -274,7 +275,7 @@ def _clean(value): if value is None: return None v = value.replace("\r", "").replace(" ", " ").strip() - if v == "" or v == "None": + if v in ("", "None"): return None return v @@ -419,30 +420,10 @@ def _assign_bullet_metadata(item, raw_line): item[field] = _clean(value) -def extract_new_po(email_data): - """Extract the new_po contract from the text/plain body. - - Permissive capture, section-windowed: each anchor line is located by exact - _clean-ed full-line equality, STRICTLY AFTER the previous anchor. Missing - anchors leave fields None -- the gate then fails closed. Representation- - agnostic: matches only on _clean-ed lines, never on '\\r'-suffixed literals - (body line endings are decode-path dependent). - - Duplicate labels: 'Supplier', 'Shipping', 'Total' each appear TWICE (summary - placeholder + detail block; the first 'Shipping' value is literally 'None'). - supplier.name anchors on the FIRST 'Supplier'; ship_to on the SECOND - 'Shipping'; total on the SECOND 'Total'. - - DERIVED_KEYS (site_code, trade, fiscal_year) stay None -- filled later by - the shared post-stage identically on both paths. - """ - candidate = _empty_candidate() - candidate["email_type"] = "new_po" - candidate["po_number"] = _subject_po_id(email_data) - - lines = _plain_lines(email_data["body"]) - - # --- Summary section (start .. 'More Detail') --- +def _extract_summary_section(candidate, lines): + """Summary block (start .. 'More Detail'): submitted_by, on_behalf_of, the + FIRST 'Supplier' name, the unique view_order_url, and the Items-summary + quantity lines. Returns (more_detail_index_or_None, summary_item_matches).""" more_detail = _find_after(lines, "More Detail", 0) summary_end = more_detail if more_detail is not None else len(lines) for label, field in ( @@ -473,8 +454,15 @@ def extract_new_po(email_data): for i in range(summary_end) if (m := _SUMMARY_ITEM_RE.fullmatch(_clean(lines[i]) or "")) ] + return more_detail, summary_items - # --- More Detail block ('More Detail' .. second 'Supplier') --- + +def _extract_more_detail_block(candidate, lines, more_detail): + """More Detail block ('More Detail' .. second 'Supplier'): the labeled + header fields. Returns the second 'Supplier' index (or None). + + The FIRST 'Shipping' lives in this block; its value must be the literal + 'None' placeholder (gate rule V4 tripwire) and is never used for ship_to.""" sup2 = None if more_detail is not None: sup2 = _find_after(lines, "Supplier", more_detail + 1) @@ -485,53 +473,102 @@ def extract_new_po(email_data): j = _next_visible(lines, idx, md_end) if j is not None: candidate[field] = _clean(lines[j]) - # The FIRST 'Shipping' lives in this block; its value must be the literal - # 'None' placeholder (gate rule V4 tripwire) and is never used for ship_to. + return sup2 - # --- ship_to (second 'Shipping' .. 'Lines'), sentinel-anchored --- + +def _fill_location_attn(ship_to, lines, start, end): + """Location Code + Attn lines after the 'United States' sentinel; the first + of each wins (mirrors the extractor's None-guarded assignment).""" + for j in range(start, end): + cl = _clean(lines[j]) or "" + lc = _LOCATION_CODE_RE.fullmatch(cl) + if lc and ship_to["location_code"] is None: + ship_to["location_code"] = lc.group("lc") + attn = _ATTN_RE.fullmatch(cl) + if attn and ship_to["attn"] is None: + ship_to["attn"] = _clean(attn.group("attn")) + + +def _fill_ship_to_address(ship_to, lines, ship2, st_end): + """Populate ship_to from the second 'Shipping' block, sentinel-anchored on + the 'United States' line that terminates the address.""" + name_idx = _next_visible(lines, ship2, st_end) + if name_idx is None: + return + ship_to["name"] = _clean(lines[name_idx]) + us_idx = _find_after(lines, _US_SENTINEL, name_idx + 1) + if us_idx is None or us_idx >= st_end: + return + city_idx = us_idx - 1 + m = None + if city_idx > name_idx: + m = _CITY_STATE_ZIP_RE.fullmatch(_clean(lines[city_idx]) or "") + if m: + ship_to["city"] = m.group("city") + ship_to["state"] = m.group("state") + ship_to["zip"] = m.group("zip") + street = [ + _clean(lines[j]) + for j in range(name_idx + 1, city_idx) + if _visible(lines[j]) + ] + if street: + ship_to["street"] = "\n".join(street) + ship_to["address"] = "\n".join( + _clean(lines[j]) for j in range(name_idx, us_idx + 1) if _visible(lines[j]) + ) + _fill_location_attn(ship_to, lines, us_idx + 1, st_end) + + +def _extract_ship_to(candidate, lines, sup2): + """ship_to block (second 'Shipping' .. 'Lines'). Returns the 'Lines' anchor + index (or None).""" ship2 = _find_after(lines, "Shipping", sup2 + 1) if sup2 is not None else None lines_anchor = _find_after(lines, "Lines", ship2 + 1) if ship2 is not None else None st_end = lines_anchor if lines_anchor is not None else len(lines) - ship_to = candidate["ship_to"] if ship2 is not None: - name_idx = _next_visible(lines, ship2, st_end) - if name_idx is not None: - ship_to["name"] = _clean(lines[name_idx]) - us_idx = _find_after(lines, _US_SENTINEL, name_idx + 1) - if us_idx is not None and us_idx < st_end: - city_idx = us_idx - 1 - m = None - if city_idx > name_idx: - m = _CITY_STATE_ZIP_RE.fullmatch(_clean(lines[city_idx]) or "") - if m: - ship_to["city"] = m.group("city") - ship_to["state"] = m.group("state") - ship_to["zip"] = m.group("zip") - street = [ - _clean(lines[j]) - for j in range(name_idx + 1, city_idx) - if _visible(lines[j]) - ] - if street: - ship_to["street"] = "\n".join(street) - ship_to["address"] = "\n".join( - _clean(lines[j]) - for j in range(name_idx, us_idx + 1) - if _visible(lines[j]) - ) - for j in range(us_idx + 1, st_end): - cl = _clean(lines[j]) or "" - lc = _LOCATION_CODE_RE.fullmatch(cl) - if lc and ship_to["location_code"] is None: - ship_to["location_code"] = lc.group("lc") - attn = _ATTN_RE.fullmatch(cl) - if attn and ship_to["attn"] is None: - ship_to["attn"] = _clean(attn.group("attn")) + _fill_ship_to_address(candidate["ship_to"], lines, ship2, st_end) + return lines_anchor - # --- Lines section ('Lines' .. second 'Total') --- - # Item blocks are delimited by the lone U+00A0 line(s). EVERY block is - # extracted, even when >1, so gate rule 7 fires with honest - # multiline_unsupported data (never silently keep item 0). + +def _parse_line_blocks(lines, start, end): + """Split the Lines section into U+00A0-delimited blocks and build one line + item per non-empty block. EVERY block is extracted, even when >1, so gate + rule 7 fires with honest multiline_unsupported data (never silently keep + item 0).""" + blocks, block = [], [] + for j in range(start, end): + if lines[j].replace("\r", "") == "\xa0": + blocks.append(block) + block = [] + else: + block.append(lines[j]) + blocks.append(block) + items = [] + for block in blocks: + visible = [ln for ln in block if _visible(ln)] + if not visible: + continue + item = _empty_line_item() + for raw in visible: + dm = _LINE_DESC_AMT_RE.fullmatch(_clean(raw) or "") + if dm and item["description"] is None: + item["description"] = _clean(dm.group("desc")) + item["amount"] = _to_decimal(dm.group("amt")) + item["currency"] = dm.group("cur") + elif _BULLET in raw: + _assign_bullet_metadata(item, raw) + # The optional ' EA' evidence line is not stored: quantity/ + # unit/price come from the Items summary; gate rule V13 cross-checks + # the EA line against it from the body. + items.append(item) + return items + + +def _extract_line_items(candidate, lines, lines_anchor, summary_items): + """Lines section ('Lines' .. second 'Total'). Populates line_items, + coupa_category, and (single-line only) quantity/unit/price from the Items + summary. Returns the second 'Total' index (or None).""" total2 = ( _find_after(lines, "Total", lines_anchor + 1) if lines_anchor is not None @@ -540,31 +577,7 @@ def extract_new_po(email_data): items = [] if lines_anchor is not None: end = total2 if total2 is not None else len(lines) - blocks, block = [], [] - for j in range(lines_anchor + 1, end): - if lines[j].replace("\r", "") == "\xa0": - blocks.append(block) - block = [] - else: - block.append(lines[j]) - blocks.append(block) - for block in blocks: - visible = [ln for ln in block if _visible(ln)] - if not visible: - continue - item = _empty_line_item() - for raw in visible: - dm = _LINE_DESC_AMT_RE.fullmatch(_clean(raw) or "") - if dm and item["description"] is None: - item["description"] = _clean(dm.group("desc")) - item["amount"] = _to_decimal(dm.group("amt")) - item["currency"] = dm.group("cur") - elif _BULLET in raw: - _assign_bullet_metadata(item, raw) - # The optional ' EA' evidence line is not stored: quantity/ - # unit/price come from the Items summary; gate rule V13 - # cross-checks the EA line against it from the body. - items.append(item) + items = _parse_line_blocks(lines, lines_anchor + 1, end) if items: candidate["line_items"] = items # coupa_category is a VERBATIM copy of item 0's Category bullet value. @@ -574,19 +587,56 @@ def extract_new_po(email_data): items[0]["quantity"] = _to_decimal(m.group("qty")) items[0]["unit"] = m.group("unit") items[0]["price"] = _to_decimal(m.group("price")) + return total2 - # --- Total block (second 'Total' .. end) --- - if total2 is not None: - amt_idx = _next_visible(lines, total2) - if amt_idx is not None: - token = _clean(lines[amt_idx]) or "" - if _MONEY_RE.fullmatch(token): - candidate["total_amount"] = _to_decimal(token) - cur_idx = _next_visible(lines, amt_idx) - if cur_idx is not None: - cur_token = _clean(lines[cur_idx]) or "" - if _CURRENCY_RE.fullmatch(cur_token): - candidate["currency"] = cur_token + +def _extract_total_block(candidate, lines, total2): + """Total block (second 'Total' .. end): total_amount + currency.""" + if total2 is None: + return + amt_idx = _next_visible(lines, total2) + if amt_idx is None: + return + token = _clean(lines[amt_idx]) or "" + if _MONEY_RE.fullmatch(token): + candidate["total_amount"] = _to_decimal(token) + cur_idx = _next_visible(lines, amt_idx) + if cur_idx is not None: + cur_token = _clean(lines[cur_idx]) or "" + if _CURRENCY_RE.fullmatch(cur_token): + candidate["currency"] = cur_token + + +def extract_new_po(email_data): + """Extract the new_po contract from the text/plain body. + + Permissive capture, section-windowed: each anchor line is located by exact + _clean-ed full-line equality, STRICTLY AFTER the previous anchor. Missing + anchors leave fields None -- the gate then fails closed. Representation- + agnostic: matches only on _clean-ed lines, never on '\\r'-suffixed literals + (body line endings are decode-path dependent). + + Duplicate labels: 'Supplier', 'Shipping', 'Total' each appear TWICE (summary + placeholder + detail block; the first 'Shipping' value is literally 'None'). + supplier.name anchors on the FIRST 'Supplier'; ship_to on the SECOND + 'Shipping'; total on the SECOND 'Total'. + + Delegated section by section to the _extract_* helpers, threading the + anchor indices each stage discovers into the next; DERIVED_KEYS (site_code, + trade, fiscal_year) stay None -- filled later by the shared post-stage + identically on both paths. + """ + candidate = _empty_candidate() + candidate["email_type"] = "new_po" + candidate["po_number"] = _subject_po_id(email_data) + + lines = _plain_lines(email_data["body"]) + + more_detail, summary_items = _extract_summary_section(candidate, lines) + sup2 = _extract_more_detail_block(candidate, lines, more_detail) + lines_anchor = _extract_ship_to(candidate, lines, sup2) + total2 = _extract_line_items(candidate, lines, lines_anchor, summary_items) + _extract_total_block(candidate, lines, total2) # DERIVED_KEYS intentionally left None (shared post-stage fills them). return _normalize(candidate) @@ -608,9 +658,14 @@ def _structural_keys_ok(candidate): return all(set((it or {}).keys()) == set(LINE_ITEM_KEYS) for it in items) -def validate(candidate, template_id, email_data): +def validate(candidate, template_id, email_data): # noqa: C901, PLR0911, PLR0912 """Return (True, 'ok') only if provably conformant; else (False, reason). - Every rule must hold. See failClosedGateRules in the investigation report.""" + Every rule must hold. See failClosedGateRules in the investigation report. + + A linear fail-closed rule ladder with one return per reason code -- the + branch count is the rule count. Splitting it further adds indirection, not + clarity, so the complexity/return/branch ceilings are suppressed here (the + value-level rules V1-V13 ARE decomposed, in _validate_new_po_values).""" # (1) known template if template_id not in _TEMPLATE_EMAIL_TYPE: return False, "subject_no_match" @@ -689,218 +744,320 @@ def _money_border_ok(line_text, serialized): return False -def _validate_new_po_values(candidate, email_data): # noqa: PLR0911, PLR0912, PLR0915 - """Value-level gate rules V1-V13 for coupa_new_po. FAIL CLOSED. +@dataclass +class _NewPoAnchorFrame: + """Anchor frame for the coupa_new_po value gate. - Anchor integrity (V4) runs first because every later byte proof needs the - anchor frame; the remaining rules run in spec order. Each rule re-derives - its evidence from email_data["body"] -- duplicated regex work, negligible - at ~57 emails/day, in exchange for extractor-bug independence.""" - lines = _plain_lines(email_data["body"]) - items = candidate.get("line_items") or [] - ship_to = candidate.get("ship_to") or {} - supplier_name = (candidate.get("supplier") or {}).get("name") + V4 (_build_anchor_frame) proves the duplicate-label anchor layout ONCE and + carries the shared byte evidence every later rule re-derives from the body: + the section indices, plus the Items-summary matches and the captured line + price -- so V13 (_check_qty_unit_price) can re-consume exactly what V1 + (_check_money_fidelity) proved, without either rule trusting extractor- + carried state it cannot re-derive from email_data["body"].""" - # --- V4 anchor integrity -> anchor_violation --- - more_detail_idxs = _indices(lines, "More Detail") - supplier_idxs = _indices(lines, "Supplier") - shipping_idxs = _indices(lines, "Shipping") - total_idxs = _indices(lines, "Total") - if len(more_detail_idxs) != 1: - return False, "anchor_violation" - if len(supplier_idxs) != 2 or len(shipping_idxs) != 2 or len(total_idxs) != 2: - return False, "anchor_violation" - more_detail = more_detail_idxs[0] - # The FIRST Shipping value must be the literal 'None' placeholder -- the - # dup-label-swap tripwire (a real address under the first label means the - # layout drifted; ship_to would have been read from the wrong block). - first_ship_val = "" - if shipping_idxs[0] + 1 < len(lines): - first_ship_val = ( - lines[shipping_idxs[0] + 1].replace("\r", "").replace("\xa0", "").strip() - ) - if first_ship_val != "None": - return False, "anchor_violation" - lines_anchor = _find_after(lines, "Lines", shipping_idxs[1] + 1) - if lines_anchor is None: - return False, "anchor_violation" - # Section ordering: summary Supplier < More Detail < detail Supplier < - # second Shipping < Lines < second Total; summary Total before More Detail. - if not ( + lines: list + more_detail: int + supplier_idxs: list + shipping_idxs: list + total_idxs: list + lines_anchor: int + lines_end: int + summary_matches: list + price: object + + +def _anchor_order_ok( + more_detail, supplier_idxs, shipping_idxs, total_idxs, lines_anchor +): + """Section ordering: summary Supplier < More Detail < detail Supplier < + second Shipping < Lines < second Total; summary Total before More Detail.""" + return ( supplier_idxs[0] < more_detail < supplier_idxs[1] < shipping_idxs[1] < lines_anchor < total_idxs[1] - ): - return False, "anchor_violation" - if not (total_idxs[0] < more_detail < shipping_idxs[0] < supplier_idxs[1]): - return False, "anchor_violation" - lines_end = total_idxs[1] + ) and (total_idxs[0] < more_detail < shipping_idxs[0] < supplier_idxs[1]) - # --- V1 money fidelity -> amount_mismatch --- - # Every captured money value must re-locate its raw source token in the - # body: token fullmatches the grouped money shape, format(value, ',.2f') - # byte-equals it, and the character before it is not a digit/comma. - for_matches = [] - for j in range(lines_anchor + 1, lines_end): - m = _LINE_DESC_AMT_RE.fullmatch(_clean(lines[j]) or "") - if m: - for_matches.append(m) - if len(for_matches) != len(items): - return False, "amount_mismatch" - for item, m in zip(items, for_matches): - amount = item.get("amount") - if not isinstance(amount, Decimal): - return False, "amount_mismatch" - serialized = format(amount, ",.2f") - if serialized != m.group("amt"): - return False, "amount_mismatch" - if not _money_border_ok(m.string, serialized): - return False, "amount_mismatch" - # Line-level currency pin (rule 8 companion): rule 8 already proved the - # top-level currency is exactly 'USD', but that only covers the Total - # block. Every line item's captured currency AND its re-derived body - # token must byte-equal it too -- a single non-USD line item fails - # closed even when the Total block reads USD (the non-USD path is - # entirely unexercised at every level, not just the total). - if item.get("currency") != candidate.get("currency"): - return False, "non_usd" - if m.group("cur") != candidate.get("currency"): - return False, "non_usd" + +def _first_shipping_value(lines, first_shipping_idx): + """Value line under the FIRST 'Shipping' (the dup-label-swap tripwire: a + real address here means the layout drifted and ship_to was read from the + wrong block; the genuine layout carries the literal 'None').""" + if first_shipping_idx + 1 >= len(lines): + return "" + return lines[first_shipping_idx + 1].replace("\r", "").replace("\xa0", "").strip() + + +def _build_anchor_frame(lines, items): + """V4 anchor integrity. Returns (frame, None) when the duplicate-label + layout is proven, else (None, 'anchor_violation'). Also precomputes the + Items-summary matches and the captured line price onto the frame.""" + more_detail_idxs = _indices(lines, "More Detail") + supplier_idxs = _indices(lines, "Supplier") + shipping_idxs = _indices(lines, "Shipping") + total_idxs = _indices(lines, "Total") + # The Coupa layout repeats each of Supplier/Shipping/Total exactly twice + # (summary placeholder + detail block) -- 2 is a structural constant of the + # template, not a tunable magic number. + if ( + len(more_detail_idxs) != 1 + or len(supplier_idxs) != 2 # noqa: PLR2004 + or len(shipping_idxs) != 2 # noqa: PLR2004 + or len(total_idxs) != 2 # noqa: PLR2004 + ): + return None, "anchor_violation" + more_detail = more_detail_idxs[0] + lines_anchor = _find_after(lines, "Lines", shipping_idxs[1] + 1) + # lines_anchor is proven non-None before _anchor_order_ok consumes it (the + # `or` short-circuits); every index below is safe once the counts hold. + if ( + lines_anchor is None + or _first_shipping_value(lines, shipping_idxs[0]) != "None" + or not _anchor_order_ok( + more_detail, supplier_idxs, shipping_idxs, total_idxs, lines_anchor + ) + ): + return None, "anchor_violation" summary_matches = [ m for i in range(more_detail) if (m := _SUMMARY_ITEM_RE.fullmatch(_clean(lines[i]) or "")) ] - price = items[0].get("price") if items else None - if price is not None: - if not isinstance(price, Decimal) or len(summary_matches) != 1: - return False, "amount_mismatch" - serialized = format(price, ",.2f") - if serialized != summary_matches[0].group("price"): - return False, "amount_mismatch" - if not _money_border_ok(summary_matches[0].string, serialized): - return False, "amount_mismatch" - total = candidate.get("total_amount") - if not isinstance(total, Decimal): - return False, "amount_mismatch" + frame = _NewPoAnchorFrame( + lines=lines, + more_detail=more_detail, + supplier_idxs=supplier_idxs, + shipping_idxs=shipping_idxs, + total_idxs=total_idxs, + lines_anchor=lines_anchor, + lines_end=total_idxs[1], + summary_matches=summary_matches, + price=items[0].get("price") if items else None, + ) + return frame, None - # --- V3 dual-Total proof -> amount_mismatch (V2 needs its token, so it - # derives here; sum proof follows immediately) --- + +def _check_line_item_money(candidate, frame): + """V1 line-level money fidelity -> amount_mismatch / non_usd. + + Every captured line amount must re-locate its raw source token in the body: + the token fullmatches the grouped money shape, format(value, ',.2f') byte- + equals it, and the character before it is not a digit/comma. Line-level + currency is pinned too -- a single non-USD line item fails closed even when + the Total block reads USD (the non-USD path is entirely unexercised).""" + lines = frame.lines + items = candidate.get("line_items") or [] + for_matches = [] + for j in range(frame.lines_anchor + 1, frame.lines_end): + m = _LINE_DESC_AMT_RE.fullmatch(_clean(lines[j]) or "") + if m: + for_matches.append(m) + if len(for_matches) != len(items): + return "amount_mismatch" + for item, m in zip(items, for_matches): + amount = item.get("amount") + if not isinstance(amount, Decimal): + return "amount_mismatch" + serialized = format(amount, ",.2f") + if serialized != m.group("amt") or not _money_border_ok(m.string, serialized): + return "amount_mismatch" + currency = candidate.get("currency") + if item.get("currency") != currency or m.group("cur") != currency: + return "non_usd" + return None + + +def _check_summary_price(frame): + """V1 summary-price fidelity -> amount_mismatch. The captured Items-summary + price must re-locate its raw token exactly as the line amounts do.""" + price = frame.price + if price is None: + return None + if not isinstance(price, Decimal) or len(frame.summary_matches) != 1: + return "amount_mismatch" + serialized = format(price, ",.2f") + if serialized != frame.summary_matches[0].group("price"): + return "amount_mismatch" + if not _money_border_ok(frame.summary_matches[0].string, serialized): + return "amount_mismatch" + return None + + +def _check_money_fidelity(candidate, frame): + """V1 money fidelity: line-item amounts then the Items-summary price.""" + return _check_line_item_money(candidate, frame) or _check_summary_price(frame) + + +def _read_total_tokens(lines, total_idxs): + """Read the (amount, currency) token pair under each 'Total' anchor. + Returns (tokens, True) or (None, False) on any missing/malformed token.""" total_tokens = [] for t_idx in total_idxs: a_idx = _next_visible(lines, t_idx) if a_idx is None: - return False, "amount_mismatch" + return None, False token = _clean(lines[a_idx]) or "" if not _MONEY_RE.fullmatch(token): - return False, "amount_mismatch" + return None, False c_idx = _next_visible(lines, a_idx) cur_token = (_clean(lines[c_idx]) or "") if c_idx is not None else "" if not _CURRENCY_RE.fullmatch(cur_token): - return False, "amount_mismatch" + return None, False total_tokens.append((token, cur_token)) - if total_tokens[0] != total_tokens[1]: - return False, "amount_mismatch" - serialized = format(total, ",.2f") - if serialized != total_tokens[1][0]: - return False, "amount_mismatch" - if candidate.get("currency") != total_tokens[1][1]: - return False, "amount_mismatch" + return total_tokens, True - # --- V2 sum proof -> amount_mismatch (exact Decimal equality; deliberately - # NO qty*price==amount rule -- corpus shows partial quantities) --- - if sum(item["amount"] for item in items) != total: - return False, "amount_mismatch" - # --- V5 supplier proof -> anchor_violation --- +def _check_total_proof(candidate, frame): + """V3 dual-Total proof + V2 sum proof -> amount_mismatch. Both Total blocks + must carry byte-identical tokens that byte-equal the captured total, and the + line amounts must sum to it exactly (no qty*price rule -- partial qtys).""" + items = candidate.get("line_items") or [] + total = candidate.get("total_amount") + if not isinstance(total, Decimal): + return "amount_mismatch" + total_tokens, ok = _read_total_tokens(frame.lines, frame.total_idxs) + if ( + not ok + or total_tokens[0] != total_tokens[1] + or format(total, ",.2f") != total_tokens[1][0] + or candidate.get("currency") != total_tokens[1][1] + or sum(item["amount"] for item in items) != total + ): + return "amount_mismatch" + return None + + +def _check_supplier_proof(candidate, frame): + """V5 supplier proof -> anchor_violation. The supplier name must carry the + SEA HAVEN marker and byte-equal its restatements in both the summary and + the detail block, and must not collide with the ship_to name.""" + lines = frame.lines + ship_to = candidate.get("ship_to") or {} + supplier_name = (candidate.get("supplier") or {}).get("name") if not supplier_name or _SUPPLIER_MARKER not in supplier_name: - return False, "anchor_violation" - det_idx = _next_visible(lines, supplier_idxs[1], shipping_idxs[1]) + return "anchor_violation" + det_idx = _next_visible(lines, frame.supplier_idxs[1], frame.shipping_idxs[1]) if det_idx is None or _clean(lines[det_idx]) != supplier_name: - return False, "anchor_violation" - sum_idx = _next_visible(lines, supplier_idxs[0], more_detail) + return "anchor_violation" + sum_idx = _next_visible(lines, frame.supplier_idxs[0], frame.more_detail) if sum_idx is None or _clean(lines[sum_idx]) != supplier_name: - return False, "anchor_violation" + return "anchor_violation" if ship_to.get("name") == supplier_name: - return False, "anchor_violation" + return "anchor_violation" + return None - # --- V8 bullet discipline -> bullet_label_unrecognized (V5's per-item - # Supplier-segment proof rides the same walk) --- + +def _check_bullet_line(raw, supplier_name): + """One Lines-section bullet metadata line: closed label set, no dupes, the + required labels present, and the per-item Supplier segment (V5) byte-equal + to supplier_name.""" + seen = {} + for segment in _split_bullet_segments(raw): + label, value = _match_bullet_label(segment) + if label is None or label in seen: + return "bullet_label_unrecognized" + seen[label] = value + if set(seen) - { + "Supplier", + "Need By", + "Category", + "Account", + "Period", + "Part Number", + }: + return "bullet_label_unrecognized" + if not {"Supplier", "Need By", "Category", "Account", "Period"} <= set(seen): + return "bullet_label_unrecognized" + if _clean(seen["Supplier"]) != supplier_name: + return "anchor_violation" + return None + + +def _check_bullet_discipline(candidate, frame): + """V8 bullet discipline -> bullet_label_unrecognized (V5's per-item + Supplier-segment proof rides the same walk).""" + lines = frame.lines + items = candidate.get("line_items") or [] + supplier_name = (candidate.get("supplier") or {}).get("name") bullet_lines = [ - lines[j] for j in range(lines_anchor + 1, lines_end) if _BULLET in lines[j] + lines[j] + for j in range(frame.lines_anchor + 1, frame.lines_end) + if _BULLET in lines[j] ] if len(bullet_lines) != len(items): - return False, "bullet_label_unrecognized" + return "bullet_label_unrecognized" for raw in bullet_lines: - seen = {} - for segment in _split_bullet_segments(raw): - label, value = _match_bullet_label(segment) - if label is None or label in seen: - return False, "bullet_label_unrecognized" - seen[label] = value - if set(seen) - { - "Supplier", - "Need By", - "Category", - "Account", - "Period", - "Part Number", - }: - return False, "bullet_label_unrecognized" - if not {"Supplier", "Need By", "Category", "Account", "Period"} <= set(seen): - return False, "bullet_label_unrecognized" - if _clean(seen["Supplier"]) != supplier_name: - return False, "anchor_violation" + reason = _check_bullet_line(raw, supplier_name) + if reason: + return reason + return None - # --- V6 ship_to required -> missing_required_field --- + +def _check_ship_to_required(candidate, frame): + """V6 ship_to required fields -> missing_required_field. Required fields + present, numeric location_code that re-derives from the body, and attn (if + present) matching a body Attn line.""" + lines = frame.lines + ship_to = candidate.get("ship_to") or {} for field in ("name", "street", "city", "state", "zip", "location_code"): if not ship_to.get(field): - return False, "missing_required_field" + return "missing_required_field" if not re.fullmatch(r"\d+", ship_to["location_code"]): - return False, "missing_required_field" + return "missing_required_field" lc_values = [ m.group("lc") - for j in range(shipping_idxs[1] + 1, lines_anchor) + for j in range(frame.shipping_idxs[1] + 1, frame.lines_anchor) if (m := _LOCATION_CODE_RE.fullmatch(_clean(lines[j]) or "")) ] if ship_to["location_code"] not in lc_values: - return False, "missing_required_field" + return "missing_required_field" attn_values = [ _clean(m.group("attn")) - for j in range(shipping_idxs[1] + 1, lines_anchor) + for j in range(frame.shipping_idxs[1] + 1, frame.lines_anchor) if (m := _ATTN_RE.fullmatch(_clean(lines[j]) or "")) ] if attn_values: if ship_to.get("attn") not in attn_values: - return False, "missing_required_field" + return "missing_required_field" elif ship_to.get("attn") is not None: - return False, "missing_required_field" + return "missing_required_field" + return None - # --- V7 address shape -> address_shape_invalid (gated on the RAW - # pre-enrichment zip: validate() runs BEFORE enrich_parsed, so a short zip - # like '2149' fails closed to the LLM path where pad_zip repairs it -- - # both paths then get identical pad_zip treatment downstream) --- - us_idx = _find_after(lines, _US_SENTINEL, shipping_idxs[1] + 1) - if us_idx is None or us_idx >= lines_anchor: - return False, "address_shape_invalid" + +def _check_address_shape(candidate, frame): + """V7 address shape -> address_shape_invalid. Gated on the RAW pre- + enrichment zip: validate() runs BEFORE enrich_parsed, so a short zip fails + closed to the LLM path where pad_zip repairs it (both paths then get + identical pad_zip treatment downstream).""" + lines = frame.lines + ship_to = candidate.get("ship_to") or {} + us_idx = _find_after(lines, _US_SENTINEL, frame.shipping_idxs[1] + 1) + if us_idx is None or us_idx >= frame.lines_anchor: + return "address_shape_invalid" m = _CITY_STATE_ZIP_RE.fullmatch(_clean(lines[us_idx - 1]) or "") if not m: - return False, "address_shape_invalid" + return "address_shape_invalid" if ( m.group("city") != ship_to["city"] or m.group("state") != ship_to["state"] or m.group("zip") != ship_to["zip"] ): - return False, "address_shape_invalid" + return "address_shape_invalid" if ship_to["state"] not in _USPS_STATES: - return False, "address_shape_invalid" + return "address_shape_invalid" if not re.fullmatch(r"\d{5}(-\d{4})?", ship_to["zip"]): - return False, "address_shape_invalid" + return "address_shape_invalid" + return None - # --- V9 required labeled fields -> missing_required_field --- + +def _check_required_fields(candidate, frame): + """V9 required labeled fields -> missing_required_field. Nullable by design: + on_behalf_of, department, last_opened, acknowledged_at, revision_date, attn, + quantity, unit, price.""" + lines = frame.lines + items = candidate.get("line_items") or [] for field in ( "po_status", "order_date", @@ -910,7 +1067,7 @@ def _validate_new_po_values(candidate, email_data): # noqa: PLR0911, PLR0912, P "view_order_url", ): if candidate.get(field) is None: - return False, "missing_required_field" + return "missing_required_field" for item in items: for field in ( "description", @@ -922,66 +1079,138 @@ def _validate_new_po_values(candidate, email_data): # noqa: PLR0911, PLR0912, P "period", ): if item.get(field) is None: - return False, "missing_required_field" + return "missing_required_field" for label in _MORE_DETAIL_LABELS: if len(_indices(lines, label)) != 1: - return False, "missing_required_field" + return "missing_required_field" if candidate.get("coupa_category") != items[0].get("category"): - return False, "missing_required_field" - # Nullable by design: on_behalf_of, department, last_opened, acknowledged_at, - # revision_date, attn, quantity, unit, price. + return "missing_required_field" + return None - # --- V10 PO identity proofs -> po_id_mismatch --- + +def _check_po_identity(candidate, frame): + """V10 PO identity proofs -> po_id_mismatch. The PO id must restate under + 'PO ID', in an 'Amazon Purchase Order #' line, and as the numeric tail + of the view_order_url.""" + lines = frame.lines po = candidate["po_number"] po_id_idx = _indices(lines, "PO ID")[0] v_idx = _next_visible(lines, po_id_idx) if v_idx is None or _clean(lines[v_idx]) != po: - return False, "po_id_mismatch" + return "po_id_mismatch" if not _indices(lines, f"Amazon Purchase Order #{po}"): - return False, "po_id_mismatch" + return "po_id_mismatch" um = _ORDER_URL_ID_RE.match(candidate.get("view_order_url") or "") if not um or um.group(1) != po.split("-", 1)[1]: - return False, "po_id_mismatch" + return "po_id_mismatch" + return None - # --- V11 sentinel discipline -> unparseable_value --- + +def _check_sentinels(candidate, frame): + """V11 sentinel discipline -> unparseable_value. A present-but-unparseable + value (the _UNPARSEABLE sentinel) must fail closed.""" for leaf in _walk_leaves(candidate): if isinstance(leaf, str) and leaf == _UNPARSEABLE: - return False, "unparseable_value" + return "unparseable_value" + return None - # --- V12 hygiene -> residual_artifact --- + +def _check_hygiene(candidate, frame): + """V12 hygiene -> residual_artifact. No captured value may retain a raw CR + or nbsp artifact.""" for leaf in _walk_leaves(candidate): if isinstance(leaf, str) and ("\r" in leaf or "\xa0" in leaf): - return False, "residual_artifact" + return "residual_artifact" + return None - # --- V13 quantity/unit/price coherence -> amount_mismatch --- + +def _check_qty_unit_shape(quantity, unit, price): + """V13 scalar shape: a positive Decimal quantity, an all-caps unit, and a + Decimal price.""" + if ( + not isinstance(quantity, Decimal) + or quantity <= 0 + or not unit + or not re.fullmatch(r"[A-Z]+", unit) + or not isinstance(price, Decimal) + ): + return "amount_mismatch" + return None + + +def _check_ea_line_evidence(lines, frame, quantity): + """The Lines-block ' EA' evidence line (when present) must numeric- + equal the summary quantity.""" + ea_matches = [ + m + for j in range(frame.lines_anchor + 1, frame.lines_end) + if (m := _LINE_QTY_RE.fullmatch(_clean(lines[j]) or "")) + ] + if not ea_matches: + return None + if len(ea_matches) != 1: + return "amount_mismatch" + if _to_decimal(ea_matches[0].group("qty")) != quantity: + return "amount_mismatch" + return None + + +def _check_qty_unit_price(candidate, frame): + """V13 quantity/unit/price coherence -> amount_mismatch. Skipped entirely + when all three are absent; otherwise every piece must cohere with the + Items-summary line and the Lines-block EA evidence.""" + items = candidate.get("line_items") or [] quantity = items[0].get("quantity") unit = items[0].get("unit") - if quantity is not None or unit is not None or price is not None: - if not isinstance(quantity, Decimal) or quantity <= 0: - return False, "amount_mismatch" - if not unit or not re.fullmatch(r"[A-Z]+", unit): - return False, "amount_mismatch" - if not isinstance(price, Decimal): - return False, "amount_mismatch" - if len(summary_matches) != 1: - return False, "amount_mismatch" - if _to_decimal(summary_matches[0].group("qty")) != quantity: - return False, "amount_mismatch" - if summary_matches[0].group("unit") != unit: - return False, "amount_mismatch" - # The Lines-block ' EA' evidence line must numeric-equal the - # summary quantity when present. - ea_matches = [ - m - for j in range(lines_anchor + 1, lines_end) - if (m := _LINE_QTY_RE.fullmatch(_clean(lines[j]) or "")) - ] - if ea_matches: - if len(ea_matches) != 1: - return False, "amount_mismatch" - if _to_decimal(ea_matches[0].group("qty")) != quantity: - return False, "amount_mismatch" + price = frame.price + if quantity is None and unit is None and price is None: + return None + reason = _check_qty_unit_shape(quantity, unit, price) + if reason: + return reason + if ( + len(frame.summary_matches) != 1 + or _to_decimal(frame.summary_matches[0].group("qty")) != quantity + or frame.summary_matches[0].group("unit") != unit + ): + return "amount_mismatch" + return _check_ea_line_evidence(frame.lines, frame, quantity) + +# Value-level rules in spec order, each returning a reason code or None. V4 +# builds the shared anchor frame first (below); these consume it. +_NEW_PO_VALUE_CHECKS = ( + _check_money_fidelity, # V1 + _check_total_proof, # V3 + V2 + _check_supplier_proof, # V5 + _check_bullet_discipline, # V8 + _check_ship_to_required, # V6 + _check_address_shape, # V7 + _check_required_fields, # V9 + _check_po_identity, # V10 + _check_sentinels, # V11 + _check_hygiene, # V12 + _check_qty_unit_price, # V13 +) + + +def _validate_new_po_values(candidate, email_data): + """Value-level gate rules V1-V13 for coupa_new_po. FAIL CLOSED. + + V4 anchor integrity builds the shared anchor frame first (every later byte + proof needs it); the remaining rules then run in spec order via the per-rule + _check_* helpers, each re-deriving its evidence from email_data["body"] so + an extractor bug cannot vouch for itself. The first helper to return a + reason code short-circuits to (False, reason).""" + lines = _plain_lines(email_data["body"]) + items = candidate.get("line_items") or [] + frame, reason = _build_anchor_frame(lines, items) + if reason: + return False, reason + for check in _NEW_PO_VALUE_CHECKS: + reason = check(candidate, frame) + if reason: + return False, reason return True, "ok" @@ -1105,7 +1334,7 @@ def _normalize_nested_dict(value, keys): return {k: value.get(k) for k in keys}, True -def validate_ai_fallback(candidate): # noqa: PLR0911, PLR0912 +def validate_ai_fallback(candidate): # noqa: C901, PLR0911, PLR0912 """Fail-closed schema/type validation for the AI-fallback parse path. Returns (ok, reason, normalized_candidate_or_None). On success the handler diff --git a/lambdas/po/email_processor/tests/_po_parser_support.py b/lambdas/po/email_processor/tests/_po_parser_support.py index c0107cb..57e3082 100644 --- a/lambdas/po/email_processor/tests/_po_parser_support.py +++ b/lambdas/po/email_processor/tests/_po_parser_support.py @@ -1,194 +1,83 @@ """Offline test helpers for the purchase-order email processor. Uniquely named (not ``conftest``) so ``from _po_parser_support import ...`` -never collides with the repo's top-level ``tests/conftest.py`` when multiple -test roots are collected in one pytest run. +never collides with the repo's top-level ``tests/`` suite when multiple test +roots are collected in one pytest run. -MODULE-COLLISION GUARD: the WO suite (lambdas/wo/email_processor/tests) puts -its own module dir on ``sys.path`` and bare-imports ``handler`` / -``template_parser``, which ``sys.modules`` then caches session-wide. This -module therefore NEVER bare-imports the PO modules: it loads them by file path -via importlib under unique module names, and binds the PO handler's own bare -sibling imports (``from template_parser import ...`` / ``from ses_auth import -...``) to the PO modules only for the duration of the handler exec, restoring -any previous binding afterwards. The unique names match the ones -``tests/conftest.py`` uses, so whichever loader runs first, the whole session -shares a single PO module instance per file. +Phase 8: this is now a THIN SHIM over ``tests.support``. The single Lambda-module +loader (and its one copy of the sys.modules save/restore dance) lives in +``tests/support/loader.py``; the dummy-AWS-env + moto-before-handler ordering +lives in the repo-root ``conftest.py``. This module just eager-loads the PO +handler + its sibling handles and re-exports the shared fakes / fixture helpers +bound to ``pipeline="po"`` so no PO test file needs an import change. """ -import glob -import importlib.util -import json -import os import sys -from decimal import Decimal -# Imported for its side effect and DELIBERATELY BEFORE the handler modules are -# loaded below: moto registers its botocore stubber hook into botocore's -# BUILTIN_HANDLERS at import time, and boto3 sessions only pick the hook up if -# they are created AFTER that registration. This conftest chain is imported at -# pytest session start -- before tests/test_po_merge.py gets a chance to import -# moto -- so without this import the handler's module-level boto3 clients would -# be created un-stubbable and the moto-backed suites would hit real AWS. -import moto # noqa: F401 +from tests.support import ( + FIXTURE_DIRS, + FakeDynamoResource, + FakeTable, + load_lambda_module, +) +from tests.support import load_email as _load_email +from tests.support import load_golden as _load_golden +from tests.support import load_raw as _load_raw +from tests.support import stems as _stems -_HERE = os.path.dirname(__file__) -_MODULE_DIR = os.path.abspath(os.path.join(_HERE, "..")) -# Phase 3 single-sourced ses_auth/email_parsing/emf under lambdas/shared/. -# _HERE = lambdas/po/email_processor/tests -> ../../.. = lambdas. -_SHARED_DIR = os.path.abspath(os.path.join(_HERE, "..", "..", "..", "shared")) - -# The handler creates boto3 clients at import time; give it a region and dummy -# creds so import works offline/under CI. -os.environ.setdefault("AWS_DEFAULT_REGION", "us-east-1") -os.environ.setdefault("AWS_ACCESS_KEY_ID", "testing") -os.environ.setdefault("AWS_SECRET_ACCESS_KEY", "testing") -os.environ.setdefault("AWS_SESSION_TOKEN", "testing") - -FIXTURES = os.path.join(_HERE, "fixtures") +__all__ = [ + "FakeDynamoResource", + "FakeTable", + "load_lambda_module", + "po_handler", + "template_parser", + "po_prompts", + "po_telemetry", + "po_extraction", + "po_enrichment", + "po_persistence", + "FIXTURES", + "NEW_PO_STEMS", + "CANCELLATION_STEMS", + "AI_FALLBACK_STEMS", + "ADVERSARIAL_STEMS", + "load_raw", + "load_email", + "load_golden", +] _HANDLER_NAME = "po_email_processor_handler" - -def _load_module(filename, module_name): - if module_name in sys.modules: - return sys.modules[module_name] - spec = importlib.util.spec_from_file_location( - module_name, os.path.join(_MODULE_DIR, filename) - ) - module = importlib.util.module_from_spec(spec) - sys.modules[module_name] = module - spec.loader.exec_module(module) - return module - - -def _load_shared_module(filename, module_name): - """Load a Phase 3 shared sibling (ses_auth/email_parsing/emf) from - lambdas/shared/. Mirrors ``_load_module`` but resolves against - ``_SHARED_DIR`` -- the handler ``from email_parsing import ...`` / - ``from emf import ...`` / ``from ses_auth import ...`` bare imports must be - pre-bound in sys.modules because this support module does NOT put shared/ - on sys.path.""" - if module_name in sys.modules: - return sys.modules[module_name] - spec = importlib.util.spec_from_file_location( - module_name, os.path.join(_SHARED_DIR, filename) - ) - module = importlib.util.module_from_spec(spec) - sys.modules[module_name] = module - spec.loader.exec_module(module) - return module - - -def load_template_parser(): - return _load_module("template_parser.py", f"{_HANDLER_NAME}__template_parser") - - -# Phase 5 split handler.py into flat siblings. This ordered table binds each -# sibling's BARE name in sys.modules right after loading it and BEFORE the next -# sibling execs, so a sibling whose body does `from import ...` finds -# already bound. The order is dependency-topological: emf < telemetry, prompts < -# extraction, derived_fields + telemetry < enrichment. The per-pipeline siblings -# load from _MODULE_DIR (_load_module); the Phase 3 single-sourced ones -# (ses_auth/email_parsing/emf) load from lambdas/shared/ (_load_shared_module). -# The WHOLE load (siblings + handler) is wrapped in ONE save/restore so the bare -# bindings never leak past the handler exec. -_PO_SIBLINGS = ( - ("prompts", _load_module, "prompts.py"), - ("emf", _load_shared_module, "emf.py"), - ("telemetry", _load_module, "telemetry.py"), - ("derived_fields", _load_module, "derived_fields.py"), - ("email_parsing", _load_shared_module, "email_parsing.py"), - ("template_parser", _load_module, "template_parser.py"), - ("ses_auth", _load_shared_module, "ses_auth.py"), - ("extraction", _load_module, "extraction.py"), - ("enrichment", _load_module, "enrichment.py"), - ("persistence", _load_module, "persistence.py"), -) - - -def load_po_handler(): - if _HANDLER_NAME in sys.modules: - return sys.modules[_HANDLER_NAME] - saved = {} - try: - for name, loader, filename in _PO_SIBLINGS: - module = loader(filename, f"{_HANDLER_NAME}__{name}") - if name not in saved: - saved[name] = sys.modules.get(name) - sys.modules[name] = module - module = _load_module("handler.py", _HANDLER_NAME) - finally: - for name, previous in saved.items(): - if previous is not None: - sys.modules[name] = previous - else: - sys.modules.pop(name, None) - return module - - -template_parser = load_template_parser() -po_handler = load_po_handler() - -# Phase 5: expose each flat sibling by its unique module name so tests can patch -# accessors / read constants on the OWNING module (a re-exported name patched on -# po_handler only affects calls handler itself makes; a function running in the -# sibling's namespace reads its own binding -- see spec re-export rule). +# Eager-load the PO handler (and, via the loader's save/restore dance, all its +# flat siblings under unique ``__`` names). The loader keys +# are unchanged by design, so the sibling handles below are byte-identical to +# the pre-Phase-8 module names. +po_handler = load_lambda_module("po", "email_processor/handler") +template_parser = sys.modules[f"{_HANDLER_NAME}__template_parser"] po_prompts = sys.modules[f"{_HANDLER_NAME}__prompts"] po_telemetry = sys.modules[f"{_HANDLER_NAME}__telemetry"] po_extraction = sys.modules[f"{_HANDLER_NAME}__extraction"] po_enrichment = sys.modules[f"{_HANDLER_NAME}__enrichment"] po_persistence = sys.modules[f"{_HANDLER_NAME}__persistence"] +FIXTURES = str(FIXTURE_DIRS["po"]) -def _stems(subdir): - return sorted( - os.path.splitext(os.path.basename(p))[0] - for p in glob.glob(os.path.join(FIXTURES, subdir, "*.eml")) - ) - - -NEW_PO_STEMS = _stems("new-po") -CANCELLATION_STEMS = _stems("cancellation") -AI_FALLBACK_STEMS = _stems("ai-fallback") -ADVERSARIAL_STEMS = _stems("adversarial") +NEW_PO_STEMS = _stems("po", "new-po") +CANCELLATION_STEMS = _stems("po", "cancellation") +AI_FALLBACK_STEMS = _stems("po", "ai-fallback") +ADVERSARIAL_STEMS = _stems("po", "adversarial") def load_raw(subdir, stem): - with open(os.path.join(FIXTURES, subdir, f"{stem}.eml"), "rb") as fh: - return fh.read() + return _load_raw("po", subdir, stem) def load_email(subdir, stem): """Parse a fixture .eml into the email_data dict the parser consumes.""" - return po_handler.parse_raw_email(load_raw(subdir, stem)) + return _load_email("po", subdir, stem) def load_golden(stem): - """Load the expected parser output for a positive fixture. - - Goldens store money as JSON numbers; ``parse_float=Decimal`` mirrors the - handler's Bedrock decode and makes every golden money value compare - exactly equal to the parser's Decimal output (2-dp values at PO magnitudes - round-trip float repr losslessly).""" - with open(os.path.join(FIXTURES, "expected", f"{stem}.json")) as fh: - return json.load(fh, parse_float=Decimal) - - -class FakeTable: - """Records update_item calls for assertions.""" - - def __init__(self, name): - self.name = name - self.updates = [] - - def update_item(self, **kwargs): - self.updates.append(kwargs) - - -class FakeDynamoResource: - def __init__(self): - self.tables = {} - - def Table(self, name): # noqa: N802 (boto3 method name) - return self.tables.setdefault(name, FakeTable(name)) + """Load the expected parser output for a positive fixture (money as + Decimal via ``parse_float=Decimal``; see tests.support.load_golden).""" + return _load_golden("po", stem) diff --git a/tests/test_pad_zip.py b/lambdas/po/email_processor/tests/test_pad_zip.py similarity index 100% rename from tests/test_pad_zip.py rename to lambdas/po/email_processor/tests/test_pad_zip.py diff --git a/lambdas/po/email_processor/tests/test_po_bedrock_fallback.py b/lambdas/po/email_processor/tests/test_po_bedrock_fallback.py index ea818a5..06b7f0b 100644 --- a/lambdas/po/email_processor/tests/test_po_bedrock_fallback.py +++ b/lambdas/po/email_processor/tests/test_po_bedrock_fallback.py @@ -635,12 +635,10 @@ def test_fixture_hygiene_ses_auth_and_scrub_markers(): from _po_parser_support import ( ADVERSARIAL_STEMS, AI_FALLBACK_STEMS, - _load_shared_module, + load_lambda_module, ) - ses_auth = _load_shared_module( - "ses_auth.py", "po_email_processor_handler__ses_auth" - ) + ses_auth = load_lambda_module("shared", "ses_auth") coupa = ( [("new-po", s) for s in NEW_PO_STEMS] + [("cancellation", s) for s in CANCELLATION_STEMS] diff --git a/lambdas/po/email_processor/tests/test_po_bedrock_transport.py b/lambdas/po/email_processor/tests/test_po_bedrock_transport.py new file mode 100644 index 0000000..7c0934f --- /dev/null +++ b/lambdas/po/email_processor/tests/test_po_bedrock_transport.py @@ -0,0 +1,149 @@ +"""PO Bedrock transport-error scenarios (constraint 5a) + multi-record +failure-isolation pin (constraint 5e). + +Every Bedrock transport failure -- a throttle, a malformed response envelope, or +non-JSON model text -- must (1) leave the PRE-CALL ParseMethod=ai_fallback +datapoint already emitted (PO emits the metric BEFORE the Bedrock call so a +Bedrock-side error still records the attempted fallback), (2) write NOTHING (no +partial DynamoDB write), and (3) propagate the exception out of handler() into +the async-retry / DLQ path. + +All wiring goes through the shared loader + support package (no bare imports). +""" + +import json + +import pytest + +from tests.support import FakeS3 +from _po_parser_support import ( + load_raw, + po_extraction, + po_handler, + po_persistence, +) + + +class _FakeBody: + def __init__(self, data): + self._data = data + + def read(self): + return self._data + + +class _ThrottlingBedrock: + """invoke_model raises a botocore ThrottlingException ClientError.""" + + def invoke_model(self, modelId, body): # noqa: N803 (boto3 kwarg name) + from botocore.exceptions import ClientError + + raise ClientError( + {"Error": {"Code": "ThrottlingException", "Message": "rate exceeded"}}, + "InvokeModel", + ) + + +class _MalformedResponseBedrock: + """invoke_model returns a structurally-broken response body.""" + + def __init__(self, payload_bytes): + self._payload = payload_bytes + + def invoke_model(self, modelId, body): # noqa: N803 + return {"body": _FakeBody(self._payload)} + + +def _event(*keys): + return { + "Records": [ + { + "s3": { + "bucket": {"name": "po-ingest-emails-x"}, + "object": {"key": key}, + } + } + for key in keys + ] + } + + +@pytest.fixture +def metric_spy(monkeypatch): + calls = [] + monkeypatch.setattr( + po_handler, "_emit_parse_method_metric", lambda *a: calls.append(a) + ) + return calls + + +@pytest.fixture +def _bypass_auth(monkeypatch): + # This suite exercises the transport-error seam, not the SES sender-auth gate + # (covered by tests/test_ses_auth.py + tests/test_handler_auth_seam.py). + monkeypatch.setattr(po_handler, "authenticate_inbound_email", lambda *a: True) + + +# The four transport-failure modes, each a bedrock double whose invoke_model +# fails a different way. All must surface as an exception out of extract_with_claude. +_TRANSPORT_CASES = { + "throttling_client_error": _ThrottlingBedrock(), + "missing_content_key": _MalformedResponseBedrock(json.dumps({}).encode()), + "empty_content_list": _MalformedResponseBedrock( + json.dumps({"content": []}).encode() + ), + "non_json_model_text": _MalformedResponseBedrock( + json.dumps({"content": [{"text": "not json {{{ definitely not"}]}).encode() + ), +} + + +@pytest.mark.parametrize("case", sorted(_TRANSPORT_CASES)) +def test_bedrock_transport_error_pins_pre_call_metric_no_write_and_reraises( + case, fake_dynamo, metric_spy, monkeypatch, _bypass_auth +): + monkeypatch.setattr( + po_handler, "s3", FakeS3({"inbound/o1": load_raw("ai-fallback", "comment-01")}) + ) + monkeypatch.setattr(po_extraction, "bedrock", _TRANSPORT_CASES[case]) + + with pytest.raises(Exception): # noqa: B017,PT011 (contract: it must propagate) + po_handler.handler(_event("inbound/o1"), None) + + # (1) the PRE-CALL ai_fallback datapoint was emitted BEFORE the raise -- the + # only real proof of PO's metric-before-call ordering (handler.py:91-96). + assert metric_spy, "expected a pre-call ParseMethod metric before the raise" + assert metric_spy[0][0] == "ai_fallback" + # No ai_fallback_rejected: the failure was transport, not the gate. + assert [c[0] for c in metric_spy] == ["ai_fallback"] + + # (2) no partial write landed. + po_table = fake_dynamo.tables.get(po_persistence.PO_TABLE) + assert po_table is None or po_table.updates == [] + + +def test_multi_record_failure_isolation_is_all_or_retry( + fake_dynamo, metric_spy, monkeypatch, _bypass_auth +): + """Constraint 5e: there is NO per-record try/except -- a second record whose + Bedrock call dies raises out of the WHOLE invocation (async retry replays the + entire batch). This documents the all-or-retry contract: the first record's + write DID land before the second record blew up.""" + monkeypatch.setattr( + po_handler, + "s3", + FakeS3( + { + "inbound/ok": load_raw("new-po", "new-po-01"), # template -> writes + "inbound/boom": load_raw("ai-fallback", "comment-01"), # -> bedrock + } + ), + ) + monkeypatch.setattr(po_extraction, "bedrock", _ThrottlingBedrock()) + + with pytest.raises(Exception): # noqa: B017,PT011 + po_handler.handler(_event("inbound/ok", "inbound/boom"), None) + + # First record's purchase-order upsert landed before the second record raised. + po_table = fake_dynamo.tables[po_persistence.PO_TABLE] + assert len(po_table.updates) == 1 diff --git a/lambdas/po/email_processor/tests/test_po_derived_wiring.py b/lambdas/po/email_processor/tests/test_po_derived_wiring.py index 171a7fb..5e44151 100644 --- a/lambdas/po/email_processor/tests/test_po_derived_wiring.py +++ b/lambdas/po/email_processor/tests/test_po_derived_wiring.py @@ -12,11 +12,15 @@ smoke. import json +from tests.support import FakeS3 from _po_parser_support import ( NEW_PO_STEMS, load_email, + load_raw, po_enrichment, + po_extraction, po_handler, + po_persistence, po_telemetry, template_parser, ) @@ -250,3 +254,61 @@ def test_pin_shadow_agreement_emf_is_ai_fallback_only(monkeypatch, capsys): recs = _emf_lines(capsys) assert set(recs) == {"site_code"} assert recs["site_code"]["Agreement"] == "disagree" + + +# --- PO-DC-02: 64-char clamp on the ai_fallback_rejected po_number ride-along - + + +class _FakeBody: + def __init__(self, data): + self._data = data + + def read(self): + return self._data + + +class _JsonBedrock: + def __init__(self, payload): + self._payload = payload + + def invoke_model(self, modelId, body): # noqa: N803 (boto3 kwarg name) + text = json.dumps(self._payload) + return {"body": _FakeBody(json.dumps({"content": [{"text": text}]}).encode())} + + +def test_ai_fallback_rejected_po_number_is_clamped_to_64_chars( + fake_dynamo, monkeypatch +): + """PO-DC-02 regression pin (handler.py:118): when the AI-fallback gate rejects + a model output whose po_number is longer than 64 chars, the po_number that + rides along on the ai_fallback_rejected EMF datapoint is clamped to exactly + 64 chars (an unbounded hallucinated value must not land in a 2-month log + line).""" + calls = [] + monkeypatch.setattr( + po_handler, "_emit_parse_method_metric", lambda *a: calls.append(a) + ) + monkeypatch.setattr(po_handler, "authenticate_inbound_email", lambda *a: True) + monkeypatch.setattr( + po_handler, "s3", FakeS3({"inbound/o1": load_raw("ai-fallback", "comment-01")}) + ) + # A dict with an over-long po_number that fails the nested-contract gate. + monkeypatch.setattr( + po_extraction, "bedrock", _JsonBedrock({"po_number": "P" * 100}) + ) + + event = { + "Records": [ + {"s3": {"bucket": {"name": "po-x"}, "object": {"key": "inbound/o1"}}} + ] + } + po_handler.handler(event, None) + + rejected = [c for c in calls if c[0] == "ai_fallback_rejected"] + assert len(rejected) == 1 + po_number_ride_along = rejected[0][3] + assert po_number_ride_along == "P" * 64 + assert len(po_number_ride_along) == 64 + # Nothing was written (the gate rejected before any save). + po_table = fake_dynamo.tables.get(po_persistence.PO_TABLE) + assert po_table is None or po_table.updates == [] diff --git a/tests/test_po_merge.py b/lambdas/po/email_processor/tests/test_po_merge.py similarity index 100% rename from tests/test_po_merge.py rename to lambdas/po/email_processor/tests/test_po_merge.py diff --git a/lambdas/wo/email_processor/handler.py b/lambdas/wo/email_processor/handler.py index 33b1680..404a60d 100644 --- a/lambdas/wo/email_processor/handler.py +++ b/lambdas/wo/email_processor/handler.py @@ -79,8 +79,22 @@ def handler(event, context): # on a miss or an invalid (fail-closed) result. parsed, method, template_id, reason = try_deterministic_parse(email_data) if parsed is None: - parsed = extract_with_bedrock(email_data) method = "ai_fallback" + # A Bedrock transport error (throttle, malformed response, etc.) + # previously emitted ZERO ParseOutcome datapoints -- the only emit + # sites are the post-gate success (below) and the ai_fallback_rejected + # branch. Wrap the call so a fallback attempt that dies in Bedrock + # records exactly one datapoint (ai_fallback / ReasonCode=bedrock_error) + # and then re-raises into the async-retry / DLQ path. The emit is in + # the except -- never pre-call -- so a gate-rejected email (Bedrock + # returned, validate_ai_fallback fails below) still emits ONLY + # ai_fallback_rejected, preserving the wo_stack "a rejected email + # emits nothing else" alarm contract (no double-count). + try: + parsed = extract_with_bedrock(email_data) + except Exception: + emit_parse_metric("ai_fallback", template_id, "bedrock_error", None) + raise # Fail-closed validation gate on AI output: a prompt-injected # email body could steer the model into returning arbitrary # field values, so enforce the same structural contract on both diff --git a/lambdas/wo/email_processor/template_parser.py b/lambdas/wo/email_processor/template_parser.py index 8a70917..4df3518 100644 --- a/lambdas/wo/email_processor/template_parser.py +++ b/lambdas/wo/email_processor/template_parser.py @@ -374,7 +374,7 @@ def validate(candidate, template_id, email_data): # (7) status if non-null in the enum status = candidate.get("status") if status is not None and status not in VALID_STATUSES: - return False, "malformed_site_code" + return False, "invalid_status" if template_id == "update_plaintext": # (4) body must contain "Work Order: " with LITERAL double space. diff --git a/lambdas/wo/email_processor/tests/_wo_parser_support.py b/lambdas/wo/email_processor/tests/_wo_parser_support.py index 9c00bbf..720fd69 100644 --- a/lambdas/wo/email_processor/tests/_wo_parser_support.py +++ b/lambdas/wo/email_processor/tests/_wo_parser_support.py @@ -1,83 +1,80 @@ """Offline test helpers for the work-order email processor. Uniquely named (not ``conftest``) so ``from _wo_parser_support import ...`` never -collides with the repo's top-level ``tests/conftest.py`` when both test roots are +collides with the repo's top-level ``tests/`` suite when both test roots are collected in one pytest run. + +Phase 8: rewritten OFF the old bare-``import handler`` / ``from handler import +parse_raw_email`` strategy (which was the source of the bare-name sys.modules +collision the PO loader had to defend against). This is now a THIN SHIM over +``tests.support``: the single Lambda-module loader lives in +``tests/support/loader.py`` and the dummy-AWS-env + moto-before-handler ordering +lives in the repo-root ``conftest.py``. This module eager-loads the WO handler + +its sibling handles via ``load_lambda_module`` (strictly AFTER the root conftest +imported moto) and re-exports the shared fakes / fixture helpers bound to +``pipeline="wo"``. """ -import glob -import json -import os import sys -# Make handler.py / template_parser.py importable and give boto3 a region so the -# module-level clients construct without a NoRegionError under CI. -_HERE = os.path.dirname(__file__) -_MODULE_DIR = os.path.abspath(os.path.join(_HERE, "..")) -if _MODULE_DIR not in sys.path: - sys.path.insert(0, _MODULE_DIR) -# Phase 3: handler.py now imports ses_auth/email_parsing/emf, single-sourced -# under lambdas/shared/ (no longer in _MODULE_DIR). Put shared/ on sys.path so -# the handler's bare `from email_parsing import ...` etc. resolve. These modules -# are single-copy, so bare-name caching is correct for both pipelines -- no -# collision guard needed (that only matters for the duplicated template_parser). -_SHARED_DIR = os.path.abspath(os.path.join(_HERE, "..", "..", "..", "shared")) -if _SHARED_DIR not in sys.path: - sys.path.insert(0, _SHARED_DIR) -os.environ.setdefault("AWS_DEFAULT_REGION", "us-east-1") +from tests.support import ( + FIXTURE_DIRS, + FakeDynamoResource, + FakeTable, + load_lambda_module, +) +from tests.support import load_email as _load_email +from tests.support import load_golden as _load_golden +from tests.support import stems as _stems -FIXTURES = os.path.join(_HERE, "fixtures") +__all__ = [ + "FakeDynamoResource", + "FakeTable", + "load_lambda_module", + "wo_handler", + "wo_template_parser", + "wo_extraction", + "wo_telemetry", + "wo_persistence", + "FIXTURES", + "UPDATE_STEMS", + "ASSIGN_STEMS", + "AI_FALLBACK_STEMS", + "ADVERSARIAL_STEMS", + "load_email", + "load_golden", +] +_HANDLER_NAME = "wo_email_processor_handler" -def _stems(subdir): - return sorted( - os.path.splitext(os.path.basename(p))[0] - for p in glob.glob(os.path.join(FIXTURES, subdir, "*.eml")) - ) +# Eager-load the WO handler (and, via the loader's save/restore dance, its flat +# siblings under unique ``__`` names). No moto import and no +# env mutation here -- both live in the repo-root conftest, imported by pytest at +# session start, strictly BEFORE this shim's own import runs the loader. +wo_handler = load_lambda_module("wo", "email_processor/handler") +wo_template_parser = sys.modules[f"{_HANDLER_NAME}__template_parser"] +wo_extraction = sys.modules[f"{_HANDLER_NAME}__extraction"] +wo_telemetry = sys.modules[f"{_HANDLER_NAME}__telemetry"] +wo_persistence = sys.modules[f"{_HANDLER_NAME}__persistence"] +FIXTURES = str(FIXTURE_DIRS["wo"]) -UPDATE_STEMS = _stems("update-plaintext") -ASSIGN_STEMS = _stems("assign-html") -AI_FALLBACK_STEMS = _stems("ai-fallback") -ADVERSARIAL_STEMS = _stems("adversarial") +UPDATE_STEMS = _stems("wo", "update-plaintext") +ASSIGN_STEMS = _stems("wo", "assign-html") +AI_FALLBACK_STEMS = _stems("wo", "ai-fallback") +ADVERSARIAL_STEMS = _stems("wo", "adversarial") def load_email(subdir, stem): """Parse a fixture .eml into the email_data dict the parser consumes.""" - from handler import parse_raw_email - - path = os.path.join(FIXTURES, subdir, f"{stem}.eml") - with open(path, "rb") as fh: - return parse_raw_email(fh.read()) + return _load_email("wo", subdir, stem) def load_golden(stem): - with open(os.path.join(FIXTURES, "expected", f"{stem}.json")) as fh: - return json.load(fh) + """Load the expected parser output for a positive fixture. - -class FakeTable: - """Records put_item / update_item calls for assertions.""" - - def __init__(self, name): - self.name = name - self.puts = [] - self.updates = [] - # Simulate a single-row-per-key store to detect overwrite vs distinct. - self.store = {} - - def put_item(self, Item=None, **kwargs): # noqa: N803 (boto3 kwarg name) - self.puts.append(Item) - key = (Item.get("work_order_id"), Item.get("comment_id")) - self.store[key] = Item - - def update_item(self, **kwargs): - self.updates.append(kwargs) - - -class FakeDynamoResource: - def __init__(self): - self.tables = {} - - def Table(self, name): # noqa: N802 (boto3 method name) - return self.tables.setdefault(name, FakeTable(name)) + Phase 8: now decodes with ``parse_float=Decimal`` (via + tests.support.load_golden) -- proven safe, as every WO golden is + integer-only at the JSON-number level, so no value changes. + """ + return _load_golden("wo", stem) diff --git a/lambdas/wo/email_processor/tests/conftest.py b/lambdas/wo/email_processor/tests/conftest.py index 9c2ea90..bd44eab 100644 --- a/lambdas/wo/email_processor/tests/conftest.py +++ b/lambdas/wo/email_processor/tests/conftest.py @@ -7,16 +7,16 @@ which would be ambiguous against the repo's top-level ``tests/conftest.py``. import pytest -from _wo_parser_support import FakeDynamoResource +from _wo_parser_support import FakeDynamoResource, wo_persistence @pytest.fixture def fake_dynamo(monkeypatch): # Phase 5: the dynamodb accessor cache lives on persistence.py now (the # module that owns save_work_order/save_event); patch it there. The attribute - # name is unchanged ("dynamodb") -- only the module moved. - import persistence - + # name is unchanged ("dynamodb") -- only the module moved. Phase 8: the WO + # persistence module is loaded by the shared loader and reached via the + # _wo_parser_support handle (no bare `import persistence`). fake = FakeDynamoResource() - monkeypatch.setattr(persistence, "dynamodb", fake) + monkeypatch.setattr(wo_persistence, "dynamodb", fake) return fake diff --git a/lambdas/wo/email_processor/tests/fixtures/ses-stamped/auth-pass-01.eml b/lambdas/wo/email_processor/tests/fixtures/ses-stamped/auth-pass-01.eml new file mode 100644 index 0000000..4adf4e4 --- /dev/null +++ b/lambdas/wo/email_processor/tests/fixtures/ses-stamped/auth-pass-01.eml @@ -0,0 +1,18 @@ +Authentication-Results: amazonses.com; + spf=pass (spfCheck: domain of seahaven.com designates 209.85.219.70 as permitted sender) client-ip=209.85.219.70; envelope-from=apm+bnc@seahaven.com; helo=mail-qv1-f70.google.com; + dkim=pass header.i=@seahaven.com; + dmarc=none header.from=hxgnsmartcloud.com; +From: APM +To: apm@int.seahaven.com +Subject: Synthesized SES-stamped auth-pass fixture for the WO handler auth seam +Content-Type: text/plain; charset="utf-8" + +This is a synthesized SES-stamped fixture (the one new fixture Phase 8 permits). +Its Authentication-Results header block is copied verbatim from WO_SES_HEADER in +tests/test_ses_auth.py: authserv-id amazonses.com with dkim=pass +header.i=@seahaven.com, so it authenticates for ALLOWED_DKIM_DOMAINS=seahaven.com +exactly like production mail. + +Its body does not match any deterministic WO template, so processing falls +through to the AI-fallback path (Bedrock is faked in the test). It carries no +real signature or receipt tokens -- it is not scraped mail. diff --git a/lambdas/wo/email_processor/tests/test_bedrock_fallback.py b/lambdas/wo/email_processor/tests/test_bedrock_fallback.py index b483819..69f7c7f 100644 --- a/lambdas/wo/email_processor/tests/test_bedrock_fallback.py +++ b/lambdas/wo/email_processor/tests/test_bedrock_fallback.py @@ -7,10 +7,12 @@ import os import pytest -import handler -import extraction -import persistence -from _wo_parser_support import FIXTURES +from _wo_parser_support import ( + FIXTURES, + wo_extraction as extraction, + wo_handler as handler, + wo_persistence as persistence, +) def _raw(subdir, stem): diff --git a/lambdas/wo/email_processor/tests/test_comment_id.py b/lambdas/wo/email_processor/tests/test_comment_id.py index 67a313d..257164d 100644 --- a/lambdas/wo/email_processor/tests/test_comment_id.py +++ b/lambdas/wo/email_processor/tests/test_comment_id.py @@ -7,8 +7,10 @@ not retry-stable, so it must never enter the key -- the time segment derives from the (deterministic) email Date header instead. """ -import handler # noqa: F401 (loads WO siblings via _wo_parser_support path) -import persistence +from _wo_parser_support import ( # noqa: F401 (handler loads WO siblings) + wo_handler as handler, + wo_persistence as persistence, +) DATE_HEADER = "Mon, 27 Apr 2026 23:57:49 +0000 (UTC)" diff --git a/lambdas/wo/email_processor/tests/test_healthcheck.py b/lambdas/wo/email_processor/tests/test_healthcheck.py index af016af..a81801f 100644 --- a/lambdas/wo/email_processor/tests/test_healthcheck.py +++ b/lambdas/wo/email_processor/tests/test_healthcheck.py @@ -5,14 +5,17 @@ probe immediately -- BEFORE any S3 fetch, SES sender-auth gate, or Records iteration -- and must do so without touching AWS or emitting any telemetry that could trip the ``sender_auth_rejected`` metric-filter alarm. -These tests import the WO ``handler`` via the ``_wo_parser_support`` sys.path -loader idiom (region + module dir set on import); no fake_dynamo/S3 wiring is -needed because a correct healthcheck returns before any client is used. +These tests import the WO ``handler`` via the ``_wo_parser_support`` shim (which +loads it through the shared loader after the root conftest set the AWS env and +imported moto); no fake_dynamo/S3 wiring is needed because a correct healthcheck +returns before any client is used. """ -import handler -import persistence -from _wo_parser_support import FakeDynamoResource +from _wo_parser_support import ( + FakeDynamoResource, + wo_handler as handler, + wo_persistence as persistence, +) class _ExplodingS3: diff --git a/lambdas/wo/email_processor/tests/test_parser.py b/lambdas/wo/email_processor/tests/test_parser.py index 6b2eb46..8776c7a 100644 --- a/lambdas/wo/email_processor/tests/test_parser.py +++ b/lambdas/wo/email_processor/tests/test_parser.py @@ -14,8 +14,10 @@ from _wo_parser_support import ( UPDATE_STEMS, load_email, load_golden, + wo_template_parser as template_parser, ) -from template_parser import try_deterministic_parse + +try_deterministic_parse = template_parser.try_deterministic_parse @pytest.mark.parametrize("stem", UPDATE_STEMS) @@ -56,7 +58,7 @@ def test_ai_fallback_samples_return_none(stem): def test_positive_result_matches_ai_contract_keys(): # The deterministic result must carry EXACTLY the keys the AI fallback # produces (the EXTRACTION_PROMPT JSON contract) -- no more, no less. - from template_parser import CONTRACT_KEYS + CONTRACT_KEYS = template_parser.CONTRACT_KEYS parsed, *_ = try_deterministic_parse( load_email("update-plaintext", UPDATE_STEMS[0]) @@ -66,8 +68,6 @@ def test_positive_result_matches_ai_contract_keys(): def test_extractor_exception_fails_closed(monkeypatch): # Any exception inside the extractor must yield None, never a partial parse. - import template_parser - monkeypatch.setattr( template_parser, "extract_update_plaintext", @@ -134,7 +134,8 @@ def test_huge_blank_run_in_comment_parses_in_linear_time(): quadratic in the leading/trailing-blank trim (CWE-407 guard).""" import time - from template_parser import _T1_LABELS, _capture_block + _T1_LABELS = template_parser._T1_LABELS + _capture_block = template_parser._capture_block lines = ["New Comment:"] + [""] * 500_000 + ["x"] + [""] * 500_000 start = time.monotonic() diff --git a/lambdas/wo/email_processor/tests/test_validation_gate.py b/lambdas/wo/email_processor/tests/test_validation_gate.py index 98034b3..6b873f5 100644 --- a/lambdas/wo/email_processor/tests/test_validation_gate.py +++ b/lambdas/wo/email_processor/tests/test_validation_gate.py @@ -6,14 +6,13 @@ code, and direct unit tests exercise each individual gate rule. import pytest -from _wo_parser_support import load_email -from template_parser import ( - CONTRACT_KEYS, - extract_update_plaintext, - try_deterministic_parse, - validate, - validate_ai_fallback, -) +from _wo_parser_support import load_email, wo_template_parser as template_parser + +CONTRACT_KEYS = template_parser.CONTRACT_KEYS +extract_update_plaintext = template_parser.extract_update_plaintext +try_deterministic_parse = template_parser.try_deterministic_parse +validate = template_parser.validate +validate_ai_fallback = template_parser.validate_ai_fallback # Fixture stem -> expected fail-closed reason code. EXPECTED_REASONS = { @@ -101,10 +100,32 @@ def test_rule6_bad_site_code(): def test_rule7_bad_status(): + # Phase 8 (constraint 7a): the template-path status-enum branch now returns + # its OWN reason code "invalid_status" -- previously a copy-paste bug made it + # return "malformed_site_code" (the site_code branch's code), so one status + # failure yielded two different codes depending on which parse path hit it. cand = _good_candidate() cand["status"] = "frobnicated" ok, reason = validate(cand, "update_plaintext", _good_t1_email()) - assert not ok and reason == "malformed_site_code" + assert not ok and reason == "invalid_status" + + +def test_template_and_ai_paths_agree_on_bad_status_reason_code(): + """Regression lock for the invalid_status reason-code fix (constraint 7a): + an identical bad-status failure must yield the SAME reason code on BOTH the + template ``validate`` path and the ``validate_ai_fallback`` path -- ending + the one-failure-two-codes-by-path split (doc Q4).""" + template_cand = _good_candidate() + template_cand["status"] = "frobnicated" + _, template_reason = validate(template_cand, "update_plaintext", _good_t1_email()) + + ai_cand = _ai_candidate() + ai_cand["work_order_id"] = "12345" + ai_cand["email_type"] = "update" + ai_cand["status"] = "frobnicated" + _, ai_reason = validate_ai_fallback(ai_cand) + + assert template_reason == ai_reason == "invalid_status" def test_rule8_empty_comment_text(): @@ -133,7 +154,7 @@ def test_contract_keys_match_extraction_prompt(): the deterministic and AI-fallback paths write identical shapes downstream.""" import re - import handler + from _wo_parser_support import wo_handler as handler # The prompt's JSON skeleton uses union-type pseudo-values (not strict JSON) # and repeats some enum terms in prose bullets, so pull quoted "key": tokens diff --git a/lambdas/wo/email_processor/tests/test_wo_bedrock_transport.py b/lambdas/wo/email_processor/tests/test_wo_bedrock_transport.py new file mode 100644 index 0000000..d646014 --- /dev/null +++ b/lambdas/wo/email_processor/tests/test_wo_bedrock_transport.py @@ -0,0 +1,218 @@ +"""WO Bedrock transport-error scenarios (constraint 5a) AND the no-double-count +pin for the metric-wrap fix (constraint 7b), which land in the same PR. + +The WO handler wraps the Bedrock call in an except-and-RERAISE that emits the +attempted fallback exactly once on a transport error, WITHOUT emitting anything +pre-call -- so a gate-rejected email (Bedrock returned, validate_ai_fallback +fails) still emits ONLY ai_fallback_rejected. A naive pre-call reorder would +double-count gate-rejected email as ai_fallback + ai_fallback_rejected, breaking +wo_stack's "a rejected email emits nothing else" alarm contract. + +HAND-COMPUTED EMITTED ParseOutcome SERIES (per email, post-fix -- this test pins +exactly this table): + * Bedrock transport error -> exactly ONE ("ai_fallback", , + "bedrock_error", None) emission, then the exception propagates (was 0). + * gate-rejected (validate_ai_fallback fails) -> exactly ONE + ai_fallback_rejected emission and NOTHING else (Bedrock RETURNED, so the + except never fires; no accompanying ai_fallback datapoint -> NO double-count). + * ai_fallback success -> exactly ONE ai_fallback emission. + +All wiring goes through the shared loader + support package (no bare imports). +""" + +import json + +import pytest + +from tests.support import FakeS3, load_raw +from _wo_parser_support import ( + wo_extraction as extraction, + wo_handler as handler, + wo_persistence as persistence, +) + +# A full-contract AI response (all 16 keys) for the accept path. +AI_17_KEY = { + "email_type": "comment", + "work_order_id": "77777777777", + "description": None, + "status": None, + "site_code": None, + "building": None, + "address": None, + "severity": None, + "priority": None, + "date_reported": None, + "scheduled_start": None, + "due_date": None, + "assigned_to": None, + "commenter": None, + "comment_text": "ai extracted", + "comment_time": None, +} + + +class _FakeBody: + def __init__(self, data): + self._data = data + + def read(self): + return self._data + + +class _ThrottlingBedrock: + def invoke_model(self, modelId, body): # noqa: N803 (boto3 kwarg name) + from botocore.exceptions import ClientError + + raise ClientError( + {"Error": {"Code": "ThrottlingException", "Message": "rate exceeded"}}, + "InvokeModel", + ) + + +class _MalformedResponseBedrock: + def __init__(self, payload_bytes): + self._payload = payload_bytes + + def invoke_model(self, modelId, body): # noqa: N803 + return {"body": _FakeBody(self._payload)} + + +class _JsonBedrock: + """Returns a valid envelope wrapping a JSON payload (the success/gate path).""" + + def __init__(self, payload): + self._payload = payload + + def invoke_model(self, modelId, body): # noqa: N803 + text = json.dumps(self._payload) + return {"body": _FakeBody(json.dumps({"content": [{"text": text}]}).encode())} + + +def _event(*keys): + return { + "Records": [ + { + "s3": { + "bucket": {"name": "workorder-ingest-emails-x"}, + "object": {"key": key}, + } + } + for key in keys + ] + } + + +@pytest.fixture +def metric_spy(monkeypatch): + calls = [] + monkeypatch.setattr(handler, "emit_parse_metric", lambda *a: calls.append(a)) + return calls + + +@pytest.fixture +def _bypass_auth(monkeypatch): + monkeypatch.setattr(handler, "authenticate_inbound_email", lambda *a: True) + + +_TRANSPORT_CASES = { + "throttling_client_error": _ThrottlingBedrock(), + "missing_content_key": _MalformedResponseBedrock(json.dumps({}).encode()), + "empty_content_list": _MalformedResponseBedrock( + json.dumps({"content": []}).encode() + ), + "non_json_model_text": _MalformedResponseBedrock( + json.dumps({"content": [{"text": "not json {{{ definitely not"}]}).encode() + ), +} + + +@pytest.mark.parametrize("case", sorted(_TRANSPORT_CASES)) +def test_transport_error_emits_one_bedrock_error_then_reraises( + case, fake_dynamo, metric_spy, monkeypatch, _bypass_auth +): + monkeypatch.setattr( + handler, + "s3", + FakeS3({"inbound/o1": load_raw("wo", "ai-fallback", "unknown-subject")}), + ) + monkeypatch.setattr(extraction, "bedrock", _TRANSPORT_CASES[case]) + + with pytest.raises(Exception): # noqa: B017,PT011 (contract: it must propagate) + handler.handler(_event("inbound/o1"), None) + + # Exactly one datapoint: the except-branch bedrock_error emission (the wrap + # emits in the except, never pre-call). + assert len(metric_spy) == 1 + method, _tid, reason, wo = metric_spy[0] + assert method == "ai_fallback" + assert reason == "bedrock_error" + assert wo is None + + # No write landed. + wo_table = fake_dynamo.tables.get(persistence.WORK_ORDERS_TABLE) + comments = fake_dynamo.tables.get(persistence.COMMENTS_TABLE) + assert wo_table is None or wo_table.updates == [] + assert comments is None or comments.puts == [] + + +def test_gate_rejected_emits_only_rejected_no_double_count( + fake_dynamo, metric_spy, monkeypatch, _bypass_auth +): + """Bedrock RETURNS a valid envelope but the payload fails validate_ai_fallback + -> exactly ONE ai_fallback_rejected datapoint, nothing else. Proves the wrap + did not sneak a pre-call ai_fallback emission in (no double-count).""" + monkeypatch.setattr( + handler, + "s3", + FakeS3({"inbound/o1": load_raw("wo", "ai-fallback", "unknown-subject")}), + ) + monkeypatch.setattr( + extraction, "bedrock", _JsonBedrock(dict(AI_17_KEY, email_type="exploit")) + ) + + handler.handler(_event("inbound/o1"), None) + + assert [c[0] for c in metric_spy] == ["ai_fallback_rejected"] + wo_table = fake_dynamo.tables.get(persistence.WORK_ORDERS_TABLE) + assert wo_table is None or wo_table.updates == [] + + +def test_ai_fallback_success_emits_one_ai_fallback( + fake_dynamo, metric_spy, monkeypatch, _bypass_auth +): + monkeypatch.setattr( + handler, + "s3", + FakeS3({"inbound/o1": load_raw("wo", "ai-fallback", "unknown-subject")}), + ) + monkeypatch.setattr(extraction, "bedrock", _JsonBedrock(AI_17_KEY)) + + handler.handler(_event("inbound/o1"), None) + + assert [c[0] for c in metric_spy] == ["ai_fallback"] + + +def test_multi_record_failure_isolation_is_all_or_retry( + fake_dynamo, metric_spy, monkeypatch, _bypass_auth +): + """Constraint 5e: no per-record try/except -- a second record whose Bedrock + call dies raises out of the WHOLE invocation. The first (template) record's + work-order upsert landed before the second record blew up.""" + monkeypatch.setattr( + handler, + "s3", + FakeS3( + { + "inbound/ok": load_raw("wo", "update-plaintext", "update-plaintext-01"), + "inbound/boom": load_raw("wo", "ai-fallback", "unknown-subject"), + } + ), + ) + monkeypatch.setattr(extraction, "bedrock", _ThrottlingBedrock()) + + with pytest.raises(Exception): # noqa: B017,PT011 + handler.handler(_event("inbound/ok", "inbound/boom"), None) + + wo_table = fake_dynamo.tables[persistence.WORK_ORDERS_TABLE] + assert len(wo_table.updates) == 1 diff --git a/lambdas/wo/email_processor/tests/test_wo_merge.py b/lambdas/wo/email_processor/tests/test_wo_merge.py new file mode 100644 index 0000000..6deb5c5 --- /dev/null +++ b/lambdas/wo/email_processor/tests/test_wo_merge.py @@ -0,0 +1,123 @@ +"""Moto-backed merge-semantics tests for save_work_order (constraint 5d). + +A real in-memory work-orders table is stood up with moto so these exercise the +actual DynamoDB UpdateExpression build -- the reserved-word status->wo_status +mapping, the if_not_exists created_at guard, and the None-field-dropping logic -- +against a real store rather than a stub. Mirrors tests/test_po_merge.py's +approach, placed beside the WO code (same constraint-3 principle that moved +test_po_merge out of the repo-root tests/). + +The table name comes from ``wo_persistence.WORK_ORDERS_TABLE`` (default the +literal ``"WorkOrders"``, NOT a kebab-case name) so the fixture and the code +under test resolve the same name from one source. persistence.py is untouched -- +this is a pure new-test addition pinning four existing-but-untested behaviors. +""" + +import boto3 +import pytest +from moto import mock_aws + + +@pytest.fixture +def wo_table(wo_persistence): + with mock_aws(): + resource = boto3.resource("dynamodb", region_name="us-east-1") + table = resource.create_table( + TableName=wo_persistence.WORK_ORDERS_TABLE, + KeySchema=[{"AttributeName": "work_order_id", "KeyType": "HASH"}], + AttributeDefinitions=[ + {"AttributeName": "work_order_id", "AttributeType": "S"}, + ], + BillingMode="PAY_PER_REQUEST", + ) + table.wait_until_exists() + # Point the lazily-cached module resource at the moto-mocked one. + wo_persistence.dynamodb = resource + try: + yield table + finally: + wo_persistence.dynamodb = None + + +def _item(table, work_order_id): + return table.get_item(Key={"work_order_id": work_order_id}).get("Item") + + +def _parsed(work_order_id, **overrides): + base = { + "work_order_id": work_order_id, + "email_type": "update", + "description": None, + "status": None, + "site_code": None, + "building": None, + "address": None, + "severity": None, + "priority": None, + "date_reported": None, + "scheduled_start": None, + "due_date": None, + "assigned_to": None, + } + base.update(overrides) + return base + + +def test_null_status_never_clobbers_wo_status(wo_persistence, wo_table): + """A later email with status=None must NOT overwrite an established + wo_status (None fields are dropped from the SET clause).""" + wo_table.put_item(Item={"work_order_id": "WO-A", "wo_status": "assigned"}) + + wo_persistence.save_work_order(_parsed("WO-A", status=None), "s3://b/k") + + item = _item(wo_table, "WO-A") + assert item["wo_status"] == "assigned" # not clobbered + + +def test_created_at_immutable_via_if_not_exists(wo_persistence, wo_table): + """created_at is set with if_not_exists, so the first write's value survives + every subsequent upsert.""" + wo_persistence.save_work_order(_parsed("WO-B"), "s3://b/k1") + first = _item(wo_table, "WO-B")["created_at"] + + wo_persistence.save_work_order(_parsed("WO-B", description="later"), "s3://b/k2") + second = _item(wo_table, "WO-B") + + assert second["created_at"] == first # immutable + assert second["description"] == "later" # other fields still enriched + assert second["updated_at"] >= first # updated_at is always refreshed + + +def test_status_maps_to_wo_status_reserved_word(wo_persistence, wo_table): + """'status' is a DynamoDB reserved word -- the source field maps to the + 'wo_status' attribute, and no attribute literally named 'status' is written.""" + wo_persistence.save_work_order(_parsed("WO-C", status="completed"), "s3://b/k") + + item = _item(wo_table, "WO-C") + assert item["wo_status"] == "completed" + assert "status" not in item + + +def test_none_fields_absent_from_set_clause(wo_persistence, wo_table): + """None source fields are dropped from the SET clause: a pre-existing + attribute is retained rather than being overwritten with None.""" + wo_table.put_item( + Item={"work_order_id": "WO-D", "building": "B12", "priority": "high"} + ) + + wo_persistence.save_work_order(_parsed("WO-D", description="fix door"), "s3://b/k") + + item = _item(wo_table, "WO-D") + assert item["description"] == "fix door" # the one non-None field written + assert item["building"] == "B12" # pre-existing attrs retained + assert item["priority"] == "high" + + +def test_record_type_only_when_email_type_present(wo_persistence, wo_table): + """record_type is written only when email_type is present (truthy); a falsy + email_type leaves the attribute absent.""" + wo_persistence.save_work_order(_parsed("WO-E", email_type="new_work_order"), "s") + assert _item(wo_table, "WO-E")["record_type"] == "new_work_order" + + wo_persistence.save_work_order(_parsed("WO-F", email_type=""), "s") + assert "record_type" not in _item(wo_table, "WO-F") diff --git a/pytest.ini b/pytest.ini index 62d1b76..9511b4a 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,12 +1,24 @@ [pytest] -; Discovery is restricted to the repo's test roots. test_local.py at the repo -; root matches the default test_*.py glob but is a manual script that imports a -; handler (and therefore creates boto3 clients) at collection time, so it is -; deliberately excluded. The WO email-processor keeps a self-contained, -; offline suite (deterministic template parser + Bedrock fallback + issue #23 -; comment_id) alongside its module. The PO email-processor keeps the parallel -; offline suite (PO template parser + gate + Bedrock fallback dispatch). +; Three test roots: cross-pipeline tests/, plus each pipeline's own +; email_processor/tests/ offline suite (deterministic template parser + +; Bedrock fallback dispatch + healthcheck, per pipeline). testpaths = tests lambdas/wo/email_processor/tests lambdas/po/email_processor/tests +addopts = + --cov=lambdas/po/email_processor + --cov=lambdas/wo/email_processor + --cov=lambdas/po/web_ui + --cov=lambdas/wo/web_ui + --cov=lambdas/po/site_extractor + --cov=lambdas/shared + --cov-report=term-missing + --cov-fail-under=80 +; The 80% floor is an aggregate whole-suite gate, enforced in CI by the +; reusable ci-python-sam workflow's bare `pytest` run (which inherits these +; addopts). Because the floor is inherited by every invocation, running a +; single root or file for local iteration (e.g. `pytest tests/test_web_ui_auth.py`) +; under-measures the fixed --cov set and red-exits even when the selected tests +; pass. That is expected: append `--cov-fail-under=0` (or `-p no:cov`) to a +; subset run to skip the floor while iterating. The full-suite run still gates. diff --git a/ruff.toml b/ruff.toml new file mode 100644 index 0000000..4ca9eb3 --- /dev/null +++ b/ruff.toml @@ -0,0 +1,33 @@ +# Phase 8: make the C901/PLR complexity ceilings ENFORCED, not decorative — +# the pre-existing `noqa: PLR09xx` suppressions were inert with no config. +extend-exclude = ["cdk.out"] + +[lint] +extend-select = ["C901", "PLR"] + +[lint.mccabe] +# 12, not the default 10: high enough that frozen-but-sane modules pass +# untouched (scripts/reprocess.py main=11, po/web_ui render_po_detail=11, +# derived_fields derive_site_code=12 and derive_trade=11), low enough to keep +# the Phase 8 split targets honest. +max-complexity = 12 + +[lint.per-file-ignores] +# Test suites assert fixture literals; naming every expected value is noise. +"tests/*" = ["PLR2004"] +"lambdas/*/email_processor/tests/*" = ["PLR2004"] +# SHADOW-BAKE FREEZE (docs/refactor-evaluation.md, exclusions table): +# derived_fields.py is under agreement telemetry — even a noqa comment is a +# barred in-file edit, so its findings are surfaced here instead. +"lambdas/po/email_processor/derived_fields.py" = ["C901", "PLR0911", "PLR0912", "PLR2004"] +# Frozen this phase (Phase 8's only product deltas are wo handler.py, the +# wo :377 reason-code string, and the po template_parser split): +"lambdas/po/email_processor/enrichment.py" = ["C901", "PLR0912", "PLR2004"] +"lambdas/wo/email_processor/template_parser.py" = ["C901", "PLR0911", "PLR0912"] +"lambdas/shared/ses_auth.py" = ["C901", "PLR0911"] +"lambdas/shared/emf.py" = ["PLR0913"] # 6-arg emitter IS the Phase 3 envelope contract +# Phase 4 plain helpers take (scope, id, ...) positionally by design to keep +# construct logical IDs byte-stable; the arg count IS the contract (like emf.py). +"cdk/common.py" = ["PLR0913"] +"lambdas/po/site_extractor/handler.py" = ["PLR0911"] # Phase 6 owns this file +"scripts/reprocess.py" = ["PLR0912"] # argparse validation ladder; Phase 7 file diff --git a/test_local.py b/test_local.py deleted file mode 100644 index cf6332f..0000000 --- a/test_local.py +++ /dev/null @@ -1,54 +0,0 @@ -#!/usr/bin/env python3 -""" -Local test script - parses sample WO emails through the deterministic template -parser, falling back to Claude on Bedrock for anything the templates don't cover. - -Usage: - # AWS credentials with bedrock:InvokeModel are only needed if a sample falls - # back to the AI extractor; pure-template samples parse fully offline. - python test_local.py -""" - -import json -import sys -from pathlib import Path - -sys.path.insert(0, str(Path(__file__).parent / "lambdas" / "wo" / "email_processor")) - -from handler import parse_raw_email, extract_with_bedrock -from template_parser import try_deterministic_parse - - -def main(): - samples_dir = Path(__file__).parent / "samples" - eml_files = list(samples_dir.glob("*.eml")) - - if not eml_files: - print("No .eml files found in samples/") - return - - for eml_path in eml_files: - print(f"\n{'=' * 80}") - print(f"FILE: {eml_path.name}") - print(f"{'=' * 80}") - - raw = eml_path.read_bytes() - email_data = parse_raw_email(raw) - - print(f"Subject: {email_data['subject']}") - print(f"From: {email_data['sender']}") - print(f"Date: {email_data['date']}") - print(f"Body preview: {email_data['body'][:200]}...") - print() - - parsed, method, template_id, reason = try_deterministic_parse(email_data) - if parsed is None: - print(f"Template miss ({template_id}/{reason}); sending to Bedrock...") - parsed = extract_with_bedrock(email_data) - method = "ai_fallback" - print(f"Parse method: {method} (template={template_id}, reason={reason})") - print(json.dumps(parsed, indent=2)) - - -if __name__ == "__main__": - main() diff --git a/tests/conftest.py b/tests/conftest.py deleted file mode 100644 index fdb0b76..0000000 --- a/tests/conftest.py +++ /dev/null @@ -1,176 +0,0 @@ -"""Shared pytest configuration for the procurement-ingest test suite. - -The Lambda handlers create boto3 clients at module import time, so a -region and dummy credentials must be present in the environment before -any handler module is imported. Setting them here at conftest import -time guarantees they exist before test collection touches a handler. -""" - -import importlib.util -import os -import sys -from pathlib import Path - -import pytest - -os.environ.setdefault("AWS_DEFAULT_REGION", "us-east-1") -os.environ.setdefault("AWS_ACCESS_KEY_ID", "testing") -os.environ.setdefault("AWS_SECRET_ACCESS_KEY", "testing") -os.environ.setdefault("AWS_SESSION_TOKEN", "testing") - -REPO_ROOT = Path(__file__).resolve().parents[1] -_SHARED_DIR = REPO_ROOT / "lambdas" / "shared" - - -# Sibling modules imported by bare name from the handlers (the Lambda runtime -# puts each function's own directory on sys.path; the CDK bundling then cp's the -# shared modules in flat beside handler.py so those bare imports resolve too). -# template_parser/derived_fields are duplicated PER PIPELINE, so their bare -# names MUST be bound to the right pipeline's file around each handler exec -- -# relying on sys.path ordering (or on whatever a previously collected suite left -# in sys.modules) silently binds a handler to the OTHER pipeline's sibling. -# ses_auth/email_parsing/emf are now single-sourced under lambdas/shared/ (Phase -# 3); the loop below resolves them from there via a shared-dir fallback. -# -# Phase 5 decomposed each God-handler into flat siblings (prompts/telemetry/ -# extraction/enrichment/persistence). The order below is DEPENDENCY-TOPOLOGICAL, -# not alphabetical: the loader binds each bare name in sys.modules right after -# exec'ing it, so a sibling whose module body does `from import ...` must -# appear AFTER here or its exec ImportErrors. The load-bearing edges are -# emf < telemetry, prompts < extraction, and derived_fields + telemetry < -# enrichment. WO has no enrichment/derived_fields sibling -- the loader's -# `if not sibling_path.exists(): continue` silently skips them there, so one -# unified tuple serves both pipelines. -_SIBLING_MODULES = ( - "ses_auth", - "email_parsing", - "emf", - "prompts", - "template_parser", - "derived_fields", - "telemetry", - "extraction", - "enrichment", - "persistence", -) - - -def _load_module(path, module_name): - if module_name in sys.modules: - return sys.modules[module_name] - spec = importlib.util.spec_from_file_location(module_name, path) - module = importlib.util.module_from_spec(spec) - sys.modules[module_name] = module - spec.loader.exec_module(module) - return module - - -def load_handler(relative_path, module_name): - """Load a Lambda handler module by file path under a unique name. - - The handler files all share the basename ``handler.py`` and are not - importable as packages, so a plain ``import handler`` would collide - across Lambdas. The same loader serves the ``ses_auth.py`` modules, - which are likewise duplicated per pipeline and not importable as - packages. - - Handler modules import their siblings by bare name (e.g. ``from - template_parser import try_deterministic_parse``). Each sibling is loaded - from the handler's own directory under a unique module name and registered - under its bare name only for the duration of the handler exec, then the - previous binding is restored -- so this loader is deterministic regardless - of collection order and of what the per-Lambda test suites (which put - their own module dir on sys.path) have already cached in sys.modules. - """ - path = REPO_ROOT / relative_path - if module_name in sys.modules: - return sys.modules[module_name] - # Keep the handler dir on sys.path for parity with the Lambda runtime. - handler_dir = str(path.parent) - if handler_dir not in sys.path: - sys.path.insert(0, handler_dir) - if path.name != "handler.py": - # Leaf modules (e.g. ses_auth.py itself) have no sibling imports. - return _load_module(path, module_name) - saved = {} - for sibling in _SIBLING_MODULES: - # Per-pipeline siblings (template_parser/derived_fields) resolve next to - # the handler; the shared, single-sourced siblings (ses_auth/ - # email_parsing/emf) fall back to lambdas/shared/. No ambiguity: post - # Phase 3 the shared names exist ONLY under shared/, the per-pipeline - # names ONLY next to the handler. - sibling_path = path.parent / f"{sibling}.py" - if not sibling_path.exists(): - sibling_path = _SHARED_DIR / f"{sibling}.py" - if not sibling_path.exists(): - continue - saved[sibling] = sys.modules.get(sibling) - sys.modules[sibling] = _load_module(sibling_path, f"{module_name}__{sibling}") - try: - module = _load_module(path, module_name) - finally: - for sibling, previous in saved.items(): - if previous is not None: - sys.modules[sibling] = previous - else: - sys.modules.pop(sibling, None) - return module - - -@pytest.fixture(scope="session") -def po_handler(): - """The PO email processor handler module.""" - return load_handler( - "lambdas/po/email_processor/handler.py", - "po_email_processor_handler", - ) - - -@pytest.fixture(scope="session") -def po_persistence(po_handler): - """The PO persistence sibling (owns PO_TABLE + save_* after Phase 5). - - load_handler registers each sibling under ``__`` in - sys.modules (only the bare-name binding is restored afterwards), so the - persistence module stays reachable here by its unique name. The root - tests/test_po_merge.py keys its moto table on ``po_persistence.PO_TABLE`` - rather than a hardcoded literal. - """ - return sys.modules["po_email_processor_handler__persistence"] - - -@pytest.fixture(scope="session") -def po_enrichment(po_handler): - """The PO enrichment sibling (owns pad_zip after Phase 5). - - pad_zip is PURE and NOT re-exported by handler (handler's body never calls - it -- it runs inside enrich_parsed), so tests/test_pad_zip.py dereferences it - on the owning module instead of po_handler. - """ - return sys.modules["po_email_processor_handler__enrichment"] - - -@pytest.fixture(scope="session") -def wo_handler(): - """The WO email processor handler module.""" - return load_handler( - "lambdas/wo/email_processor/handler.py", - "wo_email_processor_handler", - ) - - -@pytest.fixture(params=["po_handler", "wo_handler"]) -def email_handler(request): - """Parametrized fixture yielding each email processor handler module.""" - return request.getfixturevalue(request.param) - - -@pytest.fixture -def ses_auth(): - """The single-sourced ses_auth module (lambdas/shared/, Phase 3). - - Previously parameterized over the two per-pipeline copies to prove they - stayed byte-identical; now there is exactly one copy, so this loads it - once -- halving the test_ses_auth run. - """ - return load_handler("lambdas/shared/ses_auth.py", "shared_ses_auth") diff --git a/tests/requirements.txt b/tests/requirements.txt index 0c521bd..3a5fddf 100644 --- a/tests/requirements.txt +++ b/tests/requirements.txt @@ -1 +1,2 @@ moto==5.2.2 +pytest-cov==7.1.0 diff --git a/tests/support/__init__.py b/tests/support/__init__.py new file mode 100644 index 0000000..225364c --- /dev/null +++ b/tests/support/__init__.py @@ -0,0 +1,122 @@ +"""Shared offline test helpers for the procurement-ingest suite. + +A real package (``tests.support``) resolved as a PEP-420 namespace member via +the repo-root conftest's sys.path insert. ``tests/`` itself gets NO __init__.py +(adding it would change pytest's module naming for the root suite). + +This module imports NO moto, mutates NO environment, and imports NO handler at +module level -- the moto-before-handler ordering authority stays solely with the +repo-root conftest. The loader (which does exec handlers) is only invoked lazily +by ``load_email`` / by the pipeline shims, never at ``__init__`` import time. +""" + +import io +import json +from decimal import Decimal + +from tests.support.loader import REPO_ROOT, load_lambda_module + +__all__ = [ + "REPO_ROOT", + "load_lambda_module", + "FIXTURE_DIRS", + "stems", + "load_raw", + "load_email", + "load_golden", + "FakeTable", + "FakeDynamoResource", + "FakeS3", +] + +FIXTURE_DIRS = { + "po": REPO_ROOT / "lambdas/po/email_processor/tests/fixtures", + "wo": REPO_ROOT / "lambdas/wo/email_processor/tests/fixtures", +} + + +def stems(pipeline, subdir): + """Sorted list of fixture stems in ``/fixtures//*.eml``.""" + return sorted(p.stem for p in (FIXTURE_DIRS[pipeline] / subdir).glob("*.eml")) + + +def load_raw(pipeline, subdir, stem): + """Raw bytes of a fixture .eml.""" + return (FIXTURE_DIRS[pipeline] / subdir / f"{stem}.eml").read_bytes() + + +def load_email(pipeline, subdir, stem): + """Parse a fixture .eml into the email_data dict the parser consumes. + + Uses the Phase-3 single-sourced ``email_parsing.parse_raw_email`` that BOTH + handlers re-export -- semantics identical to the old + ``po_handler.parse_raw_email`` / bare ``from handler import parse_raw_email`` + paths, now with one implementation. + """ + email_parsing = load_lambda_module("shared", "email_parsing") + return email_parsing.parse_raw_email(load_raw(pipeline, subdir, stem)) + + +def load_golden(pipeline, stem): + """Load the expected parser output for a positive fixture. + + Goldens store money as JSON numbers; ``parse_float=Decimal`` mirrors the + handler's Bedrock decode and makes every golden money value compare exactly + equal to the parser's Decimal output (2-dp values at PO magnitudes round-trip + float repr losslessly). Applied UNCONDITIONALLY: WO goldens are integer-only + at the JSON-number level (verified: zero float-typed values across all 55 WO + goldens; decimal-looking strings occur only inside comment_text STRING + values, which parse_float never touches), so WO callers gain Decimal decoding + with no observable change. + """ + path = FIXTURE_DIRS[pipeline] / "expected" / f"{stem}.json" + with open(path) as fh: + return json.load(fh, parse_float=Decimal) + + +class FakeTable: + """Superset offline DynamoDB table -- union of the PO and WO fakes. + + PO callers only ever ``update_item`` (their ``puts``/``store`` stay + permanently empty); WO callers use ``put_item`` (recorded in ``puts`` and in + the keyed single-row ``store``) AND ``update_item``. Neither suite regresses: + each sees exactly the recorder it used before. + """ + + def __init__(self, name): + self.name = name + self.puts = [] + self.updates = [] + # Single-row-per-key store (WO): detects overwrite vs distinct rows. + self.store = {} + + def put_item(self, Item=None, **kwargs): # noqa: N803 (boto3 kwarg name) + self.puts.append(Item) + key = (Item.get("work_order_id"), Item.get("comment_id")) + self.store[key] = Item + + def update_item(self, **kwargs): + self.updates.append(kwargs) + + +class FakeDynamoResource: + def __init__(self): + self.tables = {} + + def Table(self, name): # noqa: N802 (boto3 method name) + return self.tables.setdefault(name, FakeTable(name)) + + +class FakeS3: + """Offline S3 client for the handler-level seam tests. + + The sender-auth gate sits AFTER ``get_object`` in both handlers, so a fake + S3 is mandatory to reach (and exercise) the gate. ``objects`` maps the S3 + object Key to its raw bytes. + """ + + def __init__(self, objects): + self.objects = dict(objects) + + def get_object(self, Bucket, Key): # noqa: N803 (boto3 kwarg names) + return {"Body": io.BytesIO(self.objects[Key])} diff --git a/tests/support/loader.py b/tests/support/loader.py new file mode 100644 index 0000000..a826b78 --- /dev/null +++ b/tests/support/loader.py @@ -0,0 +1,140 @@ +"""The ONE Lambda-module loader for the whole procurement-ingest test suite. + +Every test root (the repo-root ``tests/`` suite AND the two per-pipeline +``lambdas/*/email_processor/tests`` suites) loads Lambda modules through this +single ``load_lambda_module`` -- there is exactly one copy of the sys.modules +save/restore dance in the repo, and the repo-root ``conftest.py`` re-exports it. + +``from conftest import ...`` is deliberately NOT used: three ``conftest.py`` +files exist across the roots and pytest's per-directory sys.path prepending +makes the bare name ``conftest`` resolve nondeterministically -- exactly the +bare-name-collision class this phase eliminates. The loader lives here instead, +imported the same way from every invocation directory as ``tests.support``. +""" + +import importlib.util +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +_SHARED_DIR = REPO_ROOT / "lambdas" / "shared" + + +# Sibling modules imported by bare name from the handlers (the Lambda runtime +# puts each function's own directory on sys.path; the CDK bundling then cp's the +# shared modules in flat beside handler.py so those bare imports resolve too). +# template_parser/derived_fields are duplicated PER PIPELINE, so their bare +# names MUST be bound to the right pipeline's file around each handler exec -- +# relying on sys.path ordering (or on whatever a previously collected suite left +# in sys.modules) silently binds a handler to the OTHER pipeline's sibling. +# ses_auth/email_parsing/emf/web_ui_auth are now single-sourced under +# lambdas/shared/ (Phase 3); the loop below resolves them from there via a +# shared-dir fallback. +# +# Phase 5 decomposed each God-handler into flat siblings (prompts/telemetry/ +# extraction/enrichment/persistence). The order below is DEPENDENCY-TOPOLOGICAL, +# not alphabetical: the loader binds each bare name in sys.modules right after +# exec'ing it, so a sibling whose module body does `from import ...` must +# appear AFTER here or its exec ImportErrors. The load-bearing edges are +# emf < telemetry, prompts < extraction, and derived_fields + telemetry < +# enrichment. WO has no enrichment/derived_fields sibling -- the loader's +# `if not sibling_path.exists(): continue` silently skips them there, so one +# unified tuple serves both pipelines. +# +# web_ui_auth was added (no dependencies; after emf) so +# load_lambda_module("po"|"wo", "web_ui/handler") can bind the web_ui handlers' +# bare `from web_ui_auth import is_authenticated` (lambdas/po/web_ui/handler.py:15 +# and the wo equivalent) via the same shared-dir fallback. +_SIBLING_MODULES = ( + "ses_auth", + "email_parsing", + "emf", + "web_ui_auth", + "prompts", + "template_parser", + "derived_fields", + "telemetry", + "extraction", + "enrichment", + "persistence", +) + + +def _load_module(path, module_name): + if module_name in sys.modules: + return sys.modules[module_name] + spec = importlib.util.spec_from_file_location(module_name, path) + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + spec.loader.exec_module(module) + return module + + +def load_lambda_module(pipeline, name): + """Load a Lambda module by file path under a unique, deterministic name. + + ``pipeline`` is one of ``{"po", "wo", "shared"}`` and ``name`` is the path + under ``lambdas//`` without the ``.py`` suffix (e.g. + ``"email_processor/handler"``, ``"web_ui/handler"``, or ``"ses_auth"`` for + shared). The module name is ``f"{pipeline}_{name.replace('/', '_')}"`` -- + this reproduces the existing unique names byte-for-byte + (``po_email_processor_handler``, ``wo_email_processor_handler``, + ``shared_ses_auth``), so every existing ``sys.modules`` sibling key + (``po_email_processor_handler__persistence`` etc.) is unchanged. + + The handler files all share the basename ``handler.py`` and are not + importable as packages, so a plain ``import handler`` would collide across + Lambdas. The same loader serves leaf modules (``ses_auth.py``, + ``web_ui_auth.py``), which are likewise loaded by file path. + + Handler modules import their siblings by bare name (e.g. ``from + template_parser import try_deterministic_parse``). Each sibling is loaded + from the handler's own directory (falling back to ``lambdas/shared/``) under + a unique module name and registered under its bare name only for the + duration of the handler exec, then the previous binding is restored -- so + this loader is deterministic regardless of collection order and of what the + per-Lambda test suites (which put their own module dir on sys.path) have + already cached in sys.modules. + + The save/restore dance does NOT shrink to nothing: template_parser (and + derived_fields/prompts/telemetry/extraction/enrichment/persistence) remain + duplicated bare names ACROSS pipelines. One pytest session execs BOTH + handlers; without per-exec bare-name binding + restore, whichever pipeline + loads second silently binds to the first pipeline's sibling. Only + ses_auth/email_parsing/emf/web_ui_auth are single-sourced. + """ + path = REPO_ROOT / "lambdas" / pipeline / f"{name}.py" + module_name = f"{pipeline}_{name.replace('/', '_')}" + if module_name in sys.modules: + return sys.modules[module_name] + # Keep the handler dir on sys.path for parity with the Lambda runtime. + handler_dir = str(path.parent) + if handler_dir not in sys.path: + sys.path.insert(0, handler_dir) + if path.name != "handler.py": + # Leaf modules (e.g. ses_auth.py / web_ui_auth.py) have no sibling + # imports of the per-pipeline kind the dance guards. + return _load_module(path, module_name) + saved = {} + for sibling in _SIBLING_MODULES: + # Per-pipeline siblings (template_parser/derived_fields/...) resolve next + # to the handler; the shared, single-sourced siblings (ses_auth/ + # email_parsing/emf/web_ui_auth) fall back to lambdas/shared/. No + # ambiguity: post Phase 3 the shared names exist ONLY under shared/, the + # per-pipeline names ONLY next to the handler. + sibling_path = path.parent / f"{sibling}.py" + if not sibling_path.exists(): + sibling_path = _SHARED_DIR / f"{sibling}.py" + if not sibling_path.exists(): + continue + saved[sibling] = sys.modules.get(sibling) + sys.modules[sibling] = _load_module(sibling_path, f"{module_name}__{sibling}") + try: + module = _load_module(path, module_name) + finally: + for sibling, previous in saved.items(): + if previous is not None: + sys.modules[sibling] = previous + else: + sys.modules.pop(sibling, None) + return module diff --git a/tests/test_handler_auth_seam.py b/tests/test_handler_auth_seam.py new file mode 100644 index 0000000..adef2d6 --- /dev/null +++ b/tests/test_handler_auth_seam.py @@ -0,0 +1,205 @@ +"""Handler-level SES-auth reject seam (constraint 5b), per pipeline. + +The sender-auth gate sits AFTER ``get_object`` in both handlers, and the +allowlist is read from the environment at CALL time. With an empty +``ALLOWED_DKIM_DOMAINS`` and NO auth monkeypatch, every email must be rejected: +ZERO Bedrock calls, ZERO DynamoDB writes, and NO raise (rejected mail is skipped, +never DLQ'd). This closes the hole where deleting the gate line still passed +every other test (those bypass auth via monkeypatch). + +An accept-path companion per pipeline proves the gate lets good mail THROUGH when +the allowlist is set to the fixture's SES-stamped domain. + +Parameterized over both handlers via the root-conftest ``email_handler`` fixture; +all wiring goes through the shared loader + support package (no bare imports; PO +web_ui lacks __init__.py -- the loader, never package imports). +""" + +import sys + +from tests.support import FakeDynamoResource, FakeS3, load_raw + +# Per-pipeline reject fixture: any committed .eml works (the gate fails at the +# empty-allowlist check, before the DKIM verdict even matters). +_REJECT_FIXTURE = { + "po": ("po", "ai-fallback", "comment-01"), + "wo": ("wo", "ai-fallback", "unknown-subject"), +} + + +class _RecordingBedrock: + """Records every invoke_model call so the test can assert ZERO were made.""" + + def __init__(self): + self.calls = [] + + def invoke_model(self, modelId, body): # noqa: N803 (boto3 kwarg name) + self.calls.append((modelId, body)) + raise AssertionError("bedrock must not be called on a rejected email") + + +def _siblings(handler_module): + name = handler_module.__name__ # "po_email_processor_handler" / "wo_..." + pipeline = name.split("_", 1)[0] + return ( + pipeline, + sys.modules[f"{name}__extraction"], + sys.modules[f"{name}__persistence"], + ) + + +def _event(key="inbound/o1"): + return { + "Records": [ + {"s3": {"bucket": {"name": "ingest-emails-x"}, "object": {"key": key}}} + ] + } + + +def test_empty_allowlist_rejects_before_bedrock_and_writes(email_handler, monkeypatch): + pipeline, extraction, persistence = _siblings(email_handler) + raw = load_raw(*_REJECT_FIXTURE[pipeline]) + + # Env read at call time; the gate sits after get_object, so a fake S3 is + # still needed to reach it. + monkeypatch.setenv("ALLOWED_DKIM_DOMAINS", "") + monkeypatch.setattr(email_handler, "s3", FakeS3({"inbound/o1": raw})) + bedrock = _RecordingBedrock() + monkeypatch.setattr(extraction, "bedrock", bedrock) + fake = FakeDynamoResource() + monkeypatch.setattr(persistence, "dynamodb", fake) + + # NO raise: rejected mail is skipped, and the invocation returns normally. + result = email_handler.handler(_event(), None) + assert result == {"statusCode": 200, "body": "OK"} + + # ZERO bedrock calls, ZERO writes. + assert bedrock.calls == [] + assert not any(table.updates or table.puts for table in fake.tables.values()) + + +# --- Accept-path companions: the gate lets good mail THROUGH ----------------- + +# A full-contract PO AI payload (valid -> a write lands once past the gate). +_PO_AI_PAYLOAD = { + "email_type": "new_po", + "po_number": "2D-70000001", + "po_status": "Issued - Created", + "source_system": "coupa", + "submitted_by": None, + "on_behalf_of": None, + "order_date": None, + "revision_date": None, + "last_opened": None, + "acknowledged_at": None, + "payment_terms": None, + "requisition_number": None, + "department": None, + "view_order_url": None, + "supplier": {"name": None}, + "site_code": None, + "ship_to": { + "name": None, + "address": None, + "street": None, + "city": None, + "state": None, + "zip": None, + "location_code": None, + "attn": None, + }, + "total_amount": 123.45, + "currency": "USD", + "fiscal_year": None, + "trade": None, + "coupa_category": None, + "line_items": [], +} + +_WO_AI_PAYLOAD = { + "email_type": "comment", + "work_order_id": "77777777777", + "description": None, + "status": None, + "site_code": None, + "building": None, + "address": None, + "severity": None, + "priority": None, + "date_reported": None, + "scheduled_start": None, + "due_date": None, + "assigned_to": None, + "commenter": None, + "comment_text": "ai extracted", + "comment_time": None, +} + + +class _FakeBody: + def __init__(self, data): + self._data = data + + def read(self): + return self._data + + +class _JsonBedrock: + def __init__(self, payload): + self.calls = [] + self._payload = payload + + def invoke_model(self, modelId, body): # noqa: N803 + import json + + self.calls.append((modelId, body)) + text = json.dumps(self._payload) + return {"body": _FakeBody(json.dumps({"content": [{"text": text}]}).encode())} + + +def test_po_accept_path_passes_gate_and_writes(po_handler, monkeypatch): + """PO: a scrubbed Coupa fixture (dkim=pass amazon.coupahost.com) authenticates + when the allowlist names its stamped domain -- processing reaches Bedrock and + a purchase-order write lands. NO auth monkeypatch (the real gate runs).""" + extraction = sys.modules["po_email_processor_handler__extraction"] + persistence = sys.modules["po_email_processor_handler__persistence"] + + monkeypatch.setenv("ALLOWED_DKIM_DOMAINS", "amazon.coupahost.com") + monkeypatch.setattr( + po_handler, + "s3", + FakeS3({"inbound/o1": load_raw("po", "ai-fallback", "new-po-05")}), + ) + bedrock = _JsonBedrock(_PO_AI_PAYLOAD) + monkeypatch.setattr(extraction, "bedrock", bedrock) + fake = FakeDynamoResource() + monkeypatch.setattr(persistence, "dynamodb", fake) + + result = po_handler.handler(_event(), None) + assert result == {"statusCode": 200, "body": "OK"} + assert bedrock.calls, "authenticated mail must reach the Bedrock fallback" + assert fake.tables[persistence.PO_TABLE].updates + + +def test_wo_accept_path_passes_gate_and_writes(wo_handler, monkeypatch): + """WO: the one permitted new fixture (dkim=pass seahaven.com) authenticates + when the allowlist names seahaven.com -- processing reaches Bedrock and a + work-order write lands. NO auth monkeypatch (the real gate runs).""" + extraction = sys.modules["wo_email_processor_handler__extraction"] + persistence = sys.modules["wo_email_processor_handler__persistence"] + + monkeypatch.setenv("ALLOWED_DKIM_DOMAINS", "seahaven.com") + monkeypatch.setattr( + wo_handler, + "s3", + FakeS3({"inbound/o1": load_raw("wo", "ses-stamped", "auth-pass-01")}), + ) + bedrock = _JsonBedrock(_WO_AI_PAYLOAD) + monkeypatch.setattr(extraction, "bedrock", bedrock) + fake = FakeDynamoResource() + monkeypatch.setattr(persistence, "dynamodb", fake) + + result = wo_handler.handler(_event(), None) + assert result == {"statusCode": 200, "body": "OK"} + assert bedrock.calls, "authenticated mail must reach the Bedrock fallback" + assert fake.tables[persistence.WORK_ORDERS_TABLE].updates diff --git a/tests/test_web_ui_auth.py b/tests/test_web_ui_auth.py new file mode 100644 index 0000000..8602e96 --- /dev/null +++ b/tests/test_web_ui_auth.py @@ -0,0 +1,120 @@ +"""web_ui_auth.is_authenticated coverage (constraint 5c) -- 0% before Phase 8. + +Auth is the mandatory-review surface, so this pins: fail-closed on an unset ARN +(read at import -> reload), fail-closed on a Secrets-Manager exception, the TTL +cache refresh, the Bearer / X-Auth-Token / case-insensitivity header matrix, +wrong-token, and the non-ASCII-token TypeError (a documenting pin -- the module +is frozen this phase, so the fix is deferred). + +Loaded through the shared loader (``load_lambda_module("shared", "web_ui_auth")``) +and reloaded per test because the secret ARN is read at module-import time. +""" + +import pytest + +from tests.support import load_lambda_module + +_ARN = "arn:aws:secretsmanager:us-east-1:000000000000:secret:web-ui-auth-token" + + +class _FakeSecrets: + def __init__(self, value): + self.value = value + + def get_secret_value(self, SecretId): # noqa: N803 (boto3 kwarg name) + return {"SecretString": self.value} + + +class _RaisingSecrets: + def get_secret_value(self, SecretId): # noqa: N803 + raise RuntimeError("secretsmanager access denied") + + +@pytest.fixture +def load_auth(monkeypatch): + """Return a loader that (re)imports web_ui_auth with a chosen ARN env, so the + module-level ``_WEB_UI_AUTH_TOKEN_SECRET_ARN`` (read at import) reflects it.""" + mod = load_lambda_module("shared", "web_ui_auth") + + def _load(arn=_ARN): + if arn is None: + monkeypatch.delenv("WEB_UI_AUTH_TOKEN_SECRET_ARN", raising=False) + else: + monkeypatch.setenv("WEB_UI_AUTH_TOKEN_SECRET_ARN", arn) + # Re-exec the module body in place (importlib.reload can't re-find a + # file-path-loaded module by name). This re-reads the ARN env and resets + # _auth_token_cache / _auth_token_cached_at to their module defaults. + mod.__spec__.loader.exec_module(mod) + return mod + + return _load + + +def _evt(**headers): + return {"headers": headers} + + +def test_fail_closed_on_unset_arn(load_auth): + mod = load_auth(arn=None) + assert mod.is_authenticated(_evt(**{"x-auth-token": "anything"})) is False + + +def test_fail_closed_on_secrets_manager_exception(load_auth, monkeypatch): + mod = load_auth() + monkeypatch.setattr(mod.boto3, "client", lambda *a, **k: _RaisingSecrets()) + # Fails closed (False) and does NOT raise -- the except in _get_auth_token. + assert mod.is_authenticated(_evt(**{"x-auth-token": "anything"})) is False + + +def test_ttl_cache_refresh_picks_up_rotated_secret(load_auth, monkeypatch): + mod = load_auth() + fake = _FakeSecrets("tok1") + monkeypatch.setattr(mod.boto3, "client", lambda *a, **k: fake) + clock = [1000.0] + monkeypatch.setattr(mod.time, "monotonic", lambda: clock[0]) + + assert mod._get_auth_token() == "tok1" # first fetch, cached at t=1000 + + fake.value = "tok2" # secret rotated + clock[0] = 1000.0 + 299 # still within the 300s TTL + assert mod._get_auth_token() == "tok1" # served from cache + + clock[0] = 1000.0 + 301 # TTL elapsed + assert mod._get_auth_token() == "tok2" # refetched, rotation picked up + + +@pytest.mark.parametrize( + ("headers", "expected"), + [ + ({"x-auth-token": "SECRET"}, True), + ({"X-Auth-Token": "SECRET"}, True), # header-name case-insensitive + ({"Authorization": "Bearer SECRET"}, True), + ({"authorization": "bearer SECRET"}, True), # scheme case-insensitive + # X-Auth-Token takes precedence over an Authorization header. + ({"X-Auth-Token": "SECRET", "Authorization": "Bearer WRONG"}, True), + ({"x-auth-token": "WRONG"}, False), + ({"Authorization": "Bearer WRONG"}, False), + ({"Authorization": "SECRET"}, False), # no Bearer prefix + ({}, False), # no credentials at all + ({"x-auth-token": ""}, False), # empty presented token + ], +) +def test_header_matrix(load_auth, monkeypatch, headers, expected): + mod = load_auth() + monkeypatch.setattr(mod, "_get_auth_token", lambda: "SECRET") + assert mod.is_authenticated(_evt(**headers)) is expected + + +@pytest.mark.xfail( + strict=True, + reason="web_ui_auth is frozen this phase: is_authenticated raises TypeError on " + "a non-ASCII presented token (hmac.compare_digest is ASCII-only) and surfaces " + "as a 500 instead of failing closed to False. Asserting the DESIRED behavior " + "under xfail(strict) documents the intended fix and auto-fails (xpass) the moment " + "the module is corrected, forcing the marker's removal. Tracked as a follow-up.", + raises=TypeError, +) +def test_non_ascii_token_should_fail_closed(load_auth, monkeypatch): + mod = load_auth() + monkeypatch.setattr(mod, "_get_auth_token", lambda: "SECRET") + assert mod.is_authenticated(_evt(**{"x-auth-token": "SÉCRET"})) is False diff --git a/tests/test_web_ui_handlers.py b/tests/test_web_ui_handlers.py new file mode 100644 index 0000000..3d8b37e --- /dev/null +++ b/tests/test_web_ui_handlers.py @@ -0,0 +1,142 @@ +"""web_ui handler coverage for BOTH pipelines (constraint 5c) -- 0% before +Phase 8. Root placement is deliberate: web_ui_auth is cross-pipeline and PO +web_ui lacks __init__.py, so the modules are reached ONLY through the shared +loader (``load_lambda_module("po"|"wo", "web_ui/handler")``), never package +imports. The loader's web_ui_auth sibling entry binds each handler's bare +``from web_ui_auth import is_authenticated``. + +Pins: wrong/absent token -> 401; 401 WITHOUT any table access (auth is the first +line of handler()); the authenticated render path; and a hostile-field escaping +regression lock (every rendered field carrying &\"'" + + +class _WebFakeTable: + def __init__(self, items): + self._items = items + + def scan(self, **kwargs): + return {"Items": list(self._items)} + + def get_item(self, Key): # noqa: N803 (boto3 kwarg name) + pk, val = next(iter(Key.items())) + for item in self._items: + if item.get(pk) == val: + return {"Item": item} + return {} + + def query(self, **kwargs): + return {"Items": []} + + +class _WebFakeDynamo: + def __init__(self, tables): + self._tables = tables + + def Table(self, name): # noqa: N802 (boto3 method name) + return _WebFakeTable(self._tables.get(name, [])) + + +class _ExplodingDynamo: + """Any table access is a contract violation: auth must run first.""" + + def Table(self, name): # noqa: N802 + raise AssertionError(f"auth must precede any table access ({name})") + + +@pytest.fixture(params=["po", "wo"]) +def web_ui(request): + mod = load_lambda_module(request.param, "web_ui/handler") + if request.param == "po": + table = mod.PO_TABLE + hostile_item = { + "po_number": _HOSTILE, + "po_status": _HOSTILE, + "email_type": _HOSTILE, + "supplier": {"name": _HOSTILE}, + "site_code": _HOSTILE, + "trade": _HOSTILE, + "total_amount": _HOSTILE, + "processed_at": _HOSTILE, + } + else: + table = mod.WORK_ORDERS_TABLE + hostile_item = { + "work_order_id": _HOSTILE, + "description": _HOSTILE, + "site_code": _HOSTILE, + "wo_status": _HOSTILE, + "record_type": _HOSTILE, + "due_date": _HOSTILE, + "updated_at": _HOSTILE, + } + return SimpleNamespace(mod=mod, table=table, hostile_item=hostile_item) + + +def test_wrong_token_returns_401(web_ui, monkeypatch): + monkeypatch.setattr(web_ui.mod, "is_authenticated", lambda event: False) + result = web_ui.mod.handler({"headers": {"x-auth-token": "wrong"}}, None) + assert result["statusCode"] == 401 + + +def test_absent_token_returns_401(web_ui, monkeypatch): + monkeypatch.setattr(web_ui.mod, "is_authenticated", lambda event: False) + result = web_ui.mod.handler({}, None) + assert result["statusCode"] == 401 + + +def test_401_does_not_touch_the_table(web_ui, monkeypatch): + """Auth is the first line of handler(): a rejected request must return 401 + WITHOUT scanning (or otherwise touching) the DynamoDB table.""" + monkeypatch.setattr(web_ui.mod, "is_authenticated", lambda event: False) + monkeypatch.setattr(web_ui.mod, "dynamodb", _ExplodingDynamo()) + result = web_ui.mod.handler({}, None) # _ExplodingDynamo raises if touched + assert result["statusCode"] == 401 + + +def test_authenticated_render_path(web_ui, monkeypatch): + monkeypatch.setattr(web_ui.mod, "is_authenticated", lambda event: True) + monkeypatch.setattr( + web_ui.mod, "dynamodb", _WebFakeDynamo({web_ui.table: [web_ui.hostile_item]}) + ) + result = web_ui.mod.handler({}, None) + assert result["statusCode"] == 200 + assert result["headers"]["Content-Type"] == "text/html" + assert " in element context, and the payload's quotes + must be entity-escaped in attribute context (the onclick row-link sink), or + a " breaks out of the attribute value and injects an event handler.""" + monkeypatch.setattr(web_ui.mod, "is_authenticated", lambda event: True) + monkeypatch.setattr( + web_ui.mod, "dynamodb", _WebFakeDynamo({web_ui.table: [web_ui.hostile_item]}) + ) + body = web_ui.mod.handler({}, None)["body"] + + # element context: angle brackets escaped + assert "" not in body # never reflected raw + assert "<script>alert(1)</script>" in body # escaped form present + + # attribute context: the id flows through json.dumps into + # onclick="window.location={esc(..., quote=True)}", so the JSON string's + # opening quote must render as " -- a raw " right after the = means + # quote-escaping regressed and the attribute is breakable + assert "window.location="" in body + assert 'window.location="' not in body + + # the payload's own quote characters appear only entity-escaped + assert """ in body + assert "'" in body + assert _HOSTILE not in body