From cb5539bd68469c70fc710272c4e0a40de2c998c4 Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Fri, 17 Jul 2026 13:18:45 -0400 Subject: [PATCH] =?UTF-8?q?feat:=20deploy-pipeline=20guards=20=E2=80=94=20?= =?UTF-8?q?healthcheck,=20smoke=20gate,=20bundle=20glob=20+=20AST=20test?= =?UTF-8?q?=20(refactor=20phase=200)=20(#107)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: deploy-pipeline guards — healthcheck, smoke gate, bundle glob + AST test (refactor phase 0) Deploys of po-email-processor and workorder-email-processor had no verification step, so an init-time ImportError in the bundled zip could ship silently and only surface on the next real S3 event. This adds a synchronous post-deploy smoke gate wired into the deploy workflow: both Lambdas are invoked with {"healthcheck": true} and the FunctionError field is checked, since an Unhandled init error still returns HTTP 200 on RequestResponse invokes and would false-pass a plain exit-code check. The healthcheck branch is the first statement in each handler, before any boto3/S3 use or ses_auth, and only fires on a top-level direct invoke ("healthcheck" is not a key AWS ever sets on a real S3 ObjectCreated event, so mail content can't reach this path). It emits no EMF metrics and no log text that could match the sender-auth-rejected metric filter, so two deploys in one window won't trip the alarm. Separately, the PO stack's asset bundling copied a hand-maintained four-file allowlist into the zip, so every new sibling module handler.py imports had to be added by hand or the deploy shipped a Lambda that ImportErrors at cold start (bit us for template_parser in PR #105 and nearly for derived_fields in PR #2). Replaced it with a non-recursive ./*.py glob so top-level source files ship automatically while tests/ and the stale package/ dir still cannot, and added an AST-based bundle-consistency test that parses each handler's first-party imports and fails CI if the bundling command would omit any of them (a revert to an incomplete allowlist, or code moved into a subdirectory the glob doesn't cover). Includes the refactor-evaluation report that scoped this phase. * fix: review nits — unambiguous bundling-command extraction, smoke payload-parse message, dead asserts - tests/test_bundle_consistency.py: _extract_bundling_command now collects all command=[...] matches and demands exactly one per stack file, instead of silently returning whichever ast.walk visits first if a second bundled function is ever added. - scripts/post-deploy-smoke.sh: distinguish an unparseable response payload from a payload mismatch so the failure message says what actually happened (the previous "could not parse" branch was unreachable — the inline python always exited 0). - test_po_healthcheck.py: drop the substring assertions on stdout that were dead behind the stricter `captured.out == ""` assertion; keep the stderr filter-pattern check. Review follow-up on PR #107; no behavior change to any shipped code path. --- .claude/workflows/phase-0-deploy-guards.js | 428 ++++++++++++++++++ .github/workflows/deploy.yaml | 1 + README.md | 42 +- cdk/po_stack.py | 20 +- docs/refactor-evaluation.md | 305 +++++++++++++ lambdas/po/email_processor/handler.py | 10 + .../tests/test_po_healthcheck.py | 172 +++++++ lambdas/wo/email_processor/handler.py | 10 + .../email_processor/tests/test_healthcheck.py | 69 +++ scripts/post-deploy-smoke.sh | 95 ++++ tests/test_bundle_consistency.py | 250 ++++++++++ 11 files changed, 1388 insertions(+), 14 deletions(-) create mode 100644 .claude/workflows/phase-0-deploy-guards.js create mode 100644 docs/refactor-evaluation.md create mode 100644 lambdas/po/email_processor/tests/test_po_healthcheck.py create mode 100644 lambdas/wo/email_processor/tests/test_healthcheck.py create mode 100755 scripts/post-deploy-smoke.sh create mode 100644 tests/test_bundle_consistency.py diff --git a/.claude/workflows/phase-0-deploy-guards.js b/.claude/workflows/phase-0-deploy-guards.js new file mode 100644 index 0000000..7bbb130 --- /dev/null +++ b/.claude/workflows/phase-0-deploy-guards.js @@ -0,0 +1,428 @@ +export const meta = { + name: 'phase-0-deploy-guards', + description: 'Phase 0 of the procurement-ingest refactor (docs/refactor-evaluation.md): healthcheck early-return in both email processors, synchronous post-deploy smoke script wired into cd-cdk, PO cp-allowlist replaced with a non-recursive glob, and an AST bundle-consistency test. Built and adversarially verified on a branch off main, committed locally, never pushed (push is gated on /sh-security-review in the main loop).', + phases: [ + { title: 'Setup', detail: 'clean-tree check, branch feature/phase-0-deploy-guards off up-to-date main', model: 'haiku' }, + { title: 'Recon', detail: '4 parallel read-only mappers over handlers, CDK/alarms/CI, and test infra', model: 'haiku' }, + { title: 'Implement', detail: 'handlers on opus; smoke script, CDK glob + AST test on sonnet — disjoint file ownership, one branch', model: 'opus' }, + { title: 'Verify', detail: 'mechanical gates (pytest/ruff/synth/scope) on sonnet + 3 adversarial fable lenses' }, + { title: 'Fix', detail: 'opus fixer applies confirmed findings, full re-verify, max 3 rounds', model: 'opus' }, + { title: 'Package', detail: 'README update, GPT-4.1 cross-family review of the diff, single commit via -F (no push)', model: 'sonnet' }, + ], +} + +// ---------------------------------------------------------------- constants + +const REPO = '/Users/adammoussa/Documents/repositories/seahaven/procurement-ingest' +const BRANCH = 'feature/phase-0-deploy-guards' + +// Pinned contract (memory lesson: define the shared contract BEFORE the +// parallel fan-out — parallel leaves can't see each other's choices). +const CONTRACT = ` +HEALTHCHECK CONTRACT — pinned, do not deviate or "improve": +- Trigger: top-level direct-invoke payload only. In lambda handler ("handler" + in handler.py), the VERY FIRST statements: if the event is a dict and + event.get("healthcheck") is True -> return {"healthcheck": "ok"} immediately. +- Placement: BEFORE any boto3/S3 use, BEFORE ses_auth, BEFORE iterating + event["Records"]. It must not create any accept path for mail: real mail + events are S3 ObjectCreated events whose top-level keys AWS controls + ("Records"); email content can never set a top-level event key. +- Telemetry: the healthcheck branch emits NO EMF metrics and NO log line + whose text could match the sender-auth-rejected metric-filter pattern + (read the filter pattern in cdk/*_stack.py before choosing any log text; + safest is a single log line exactly "healthcheck ok" or no logging). + That alarm pages at >=1 match, so two deploys in ~30 min must not page. +- SMOKE SCRIPT CONTRACT: scripts/post-deploy-smoke.sh invokes BOTH functions + (po-email-processor, workorder-email-processor) with payload + '{"healthcheck": true}' using: aws lambda invoke --invocation-type + RequestResponse. It must check the FunctionError field of the response + (an init ImportError returns HTTP 200 + FunctionError=Unhandled — exit-code + checks false-pass) AND that the payload equals {"healthcheck": "ok"}. + Non-zero exit on any failure; set -euo pipefail; region us-east-1. +` + +const PREAMBLE = ` +You are one of several agents building refactor Phase 0 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; read-only aws CLI calls are also unnecessary). +Authoritative spec: docs/refactor-evaluation.md, section "Phase 0". +Work ONLY in the files you are told you own. Other agents are concurrently +editing other files in this same working tree — do not read-depend on or +modify their files. +${CONTRACT} +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 IMPL = { + type: 'object', + required: ['filesChanged', 'testsAdded', 'summary', 'checksRun'], + properties: { + filesChanged: { type: 'array', items: { type: 'string' } }, + testsAdded: { type: 'array', items: { type: 'string' } }, + summary: { type: 'string' }, + checksRun: { type: 'string', description: 'exact commands run + pass/fail' }, + blockers: { type: 'array', items: { type: 'string' } }, + }, +} + +const CHECKS = { + type: 'object', + required: ['passed', 'details'], + properties: { + passed: { type: 'boolean' }, + details: { type: 'string', description: 'per-gate results; verbatim failure output' }, + 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', description: 'true only with concrete file:line evidence' }, + evidence: { type: 'string' }, + fix: { type: 'string' }, + }, + }, + }, + }, +} + +const TEXT = { + type: 'object', + required: ['summary'], + properties: { summary: { type: 'string' }, verdict: { type: 'string' } }, +} + +// ------------------------------------------------------------------- setup + +phase('Setup') +const setup = await agent(` +In ${REPO}: verify the working tree is clean apart from untracked .coverage +and docs/refactor-evaluation.md (if anything ELSE is dirty, STOP and report a +blocker — do not stash or discard anything). Then: + git fetch origin && git checkout main && git pull --ff-only + git checkout -b ${BRANCH} +Also run: gh pr list --state open --json number,title,headRefName +(memory lesson: a branch cut fresh from main misses fixes sitting in unmerged +PRs — list them so the orchestrator can flag overlaps). +Return facts: current HEAD sha, branch created y/n, open PR list, blockers. +`, { label: 'setup:branch', model: 'haiku', schema: RECON }) + +if (setup && setup.blockers && setup.blockers.length) { + return { aborted: 'setup blockers', blockers: setup.blockers, openPRs: setup.facts } +} +log(`Branch ${BRANCH} ready. ${setup ? setup.summary : ''}`) + +// ------------------------------------------------------------------- recon + +phase('Recon') +const recon = await parallel([ + () => agent(`${PREAMBLE} +Read-only recon of lambdas/po/email_processor/handler.py and its tests dir. +Report: exact def line of the lambda handler; the first statements it executes +(S3 fetch? ses_auth call? Records iteration?) with line numbers; how existing +handler tests load the module and fake AWS (loader idiom, moto import-order +invariant in _po_parser_support.py); where a healthcheck test would naturally +live; any existing early-return branches. 10-20 precise facts.`, + { label: 'recon:po-handler', model: 'haiku', phase: 'Recon', schema: RECON }), + + () => agent(`${PREAMBLE} +Read-only recon of lambdas/wo/email_processor/handler.py and its tests dir. +Same report shape: handler def line, first statements executed with line +numbers, test loader idiom (_wo_parser_support.py), where a healthcheck test +lives, existing early returns. 10-20 precise facts.`, + { label: 'recon:wo-handler', model: 'haiku', phase: 'Recon', schema: RECON }), + + () => agent(`${PREAMBLE} +Read-only recon of cdk/po_stack.py, cdk/wo_stack.py and .github/workflows/. +Report: (1) the exact PO bundling command incl. the cp allowlist at +po_stack.py:~245-256 and the full file list of lambdas/po/email_processor/*.py +so the glob replacement can be proven identical-output; (2) the exact +sender-auth-rejected metric FILTER PATTERNS in both stacks (quote them +verbatim) and their alarm eval windows; (3) deploy.yaml's inputs to the +cd-cdk reusable workflow — fetch the pinned workflow file with +'gh api repos/Sea-Haven-Industries/.github/contents/.github/workflows/cd-cdk.yaml?ref=fd60e4c9041784f666ac0fdefb9bec3c7fbf5143' +(base64 -d the content) and confirm whether a post-deploy-script input exists +and its semantics (cwd, when it runs, failure handling). If it does NOT +exist, report that as a blocker with the closest available mechanism. +(4) WO bundling command for comparison. 15-25 precise facts.`, + { label: 'recon:cdk-ci', model: 'sonnet', phase: 'Recon', schema: RECON }), + + () => agent(`${PREAMBLE} +Read-only recon of test infrastructure: pytest.ini, tests/ (repo root), +tests/conftest.py, how CI (.github/workflows/ci.yaml) invokes pytest/ruff. +Report: how a NEW repo-root test file (tests/test_bundle_consistency.py) +would be collected; what it can import; whether tests/ has helpers for +locating lambda dirs; the ruff invocation used in CI. 8-15 precise facts.`, + { label: 'recon:tests-ci', model: 'haiku', phase: 'Recon', schema: RECON }), +]) + +const reconOk = recon.filter(Boolean) +const reconBlockers = reconOk.flatMap(r => r.blockers || []) +const pack = reconOk.map(r => `## ${r.summary}\n${r.facts.join('\n')}`).join('\n\n') +log(`Recon complete: ${reconOk.length}/4 mappers, ${reconBlockers.length} blockers`) + +// --------------------------------------------------------------- implement + +phase('Implement') +const implTasks = [ + { label: 'impl:po-healthcheck', model: 'opus', prompt: `${PREAMBLE} +YOU OWN: lambdas/po/email_processor/handler.py and NEW test file(s) under +lambdas/po/email_processor/tests/ ONLY. +Task: add the healthcheck early-return branch to the PO handler exactly per +the pinned contract. Add tests: (1) {"healthcheck": true} returns +{"healthcheck": "ok"} with ZERO S3 calls, ZERO ses_auth calls, ZERO DynamoDB +writes, ZERO metric emission (assert via monkeypatch/fakes per the existing +loader idiom — preserve the moto-before-handler import ordering); (2) a normal +S3 mail event is completely unaffected (an existing golden-path test still +passing is necessary but ALSO assert a mail event containing the string +"healthcheck" in its email body does NOT take the branch). +Run before returning: ruff check lambdas/po, ruff format lambdas/po --check, +pytest lambdas/po/email_processor/tests -q --no-cov. +Recon context:\n${pack}` }, + + { label: 'impl:wo-healthcheck', model: 'opus', prompt: `${PREAMBLE} +YOU OWN: lambdas/wo/email_processor/handler.py and NEW test file(s) under +lambdas/wo/email_processor/tests/ ONLY. +Task: identical healthcheck branch + tests for the WO handler, per contract, +mirroring the PO task one-for-one but using the WO test loader idiom. +Run before returning: ruff check lambdas/wo, ruff format lambdas/wo --check, +pytest lambdas/wo/email_processor/tests -q --no-cov. +Recon context:\n${pack}` }, + + { label: 'impl:smoke-script', model: 'sonnet', prompt: `${PREAMBLE} +YOU OWN: scripts/post-deploy-smoke.sh (new) and .github/workflows/deploy.yaml ONLY. +Task: write the synchronous smoke script per the pinned SMOKE SCRIPT CONTRACT +(both functions, RequestResponse, FunctionError field check + payload check, +set -euo pipefail, executable bit, shellcheck-clean). Wire it into deploy.yaml +via the cd-cdk reusable workflow's post-deploy-script input per the recon +facts below — if recon reported that input missing, implement the closest +mechanism recon identified and record a blocker note instead of inventing +workflow inputs. Do NOT restructure deploy.yaml otherwise. +Run before returning: bash -n scripts/post-deploy-smoke.sh, and +python3 -c "import yaml,sys;yaml.safe_load(open('.github/workflows/deploy.yaml'))". +Recon context:\n${pack}` }, + + { label: 'impl:bundle-glob-ast', model: 'sonnet', prompt: `${PREAMBLE} +YOU OWN: cdk/po_stack.py (ONLY the email-processor bundling command block, +~lines 241-258) and tests/test_bundle_consistency.py (new) ONLY. +Task A: replace the four-file cp allowlist with a non-recursive glob: +"cp ./*.py /asset-output/". Keep the pip install line untouched. Update the +warning comment to explain the glob + that the AST test now enforces +consistency. PROVE identical output: list lambdas/po/email_processor/*.py +(non-recursive) and confirm the set equals {handler, ses_auth, +template_parser, derived_fields}.py plus any other top-level .py that SHOULD +ship; if extras exist (e.g. prompts or __init__), state explicitly whether +shipping them is a no-op and why. tests/ and package/ are directories, so a +non-recursive ./*.py glob never matches them. +Task B: write tests/test_bundle_consistency.py: for EACH pipeline (po, wo), +ast-parse email_processor/handler.py, collect top-level "import X" / +"from X import ..." names, filter to first-party siblings (X.py exists in the +same dir), and assert the bundling guarantee ships them — for PO: assert the +po_stack.py bundling command contains the glob "cp ./*.py"; for WO: assert +its bundling command copies them (read wo_stack.py to see its current cp -r +form and assert accordingly). The test must FAIL if someone reverts the glob +to an allowlist missing a sibling — include a unit-level check that simulates +an allowlist command string missing derived_fields.py and asserts the +detection logic catches it. No AWS/boto3/synth in the test — pure +ast + file reads, fast. +Run before returning: ruff check cdk tests, pytest tests/test_bundle_consistency.py -q --no-cov. +Recon context:\n${pack}` }, +] + +const impl = await parallel(implTasks.map(t => () => + agent(t.prompt, { label: t.label, model: t.model, phase: 'Implement', schema: IMPL }))) +const implOk = impl.filter(Boolean) +const implBlockers = implOk.flatMap(r => r.blockers || []) +log(`Implement complete: ${implOk.length}/4 agents, blockers: ${implBlockers.length}`) + +// ---------------------------------------------------- verify + fix loop + +const EXPECTED_SCOPE = [ + 'lambdas/po/email_processor/handler.py', + 'lambdas/po/email_processor/tests/', + 'lambdas/wo/email_processor/handler.py', + 'lambdas/wo/email_processor/tests/', + 'scripts/post-deploy-smoke.sh', + '.github/workflows/deploy.yaml', + 'cdk/po_stack.py', + 'tests/test_bundle_consistency.py', +] + +const mechanicalPrompt = `${PREAMBLE} +Independent re-verification (trust-but-verify — do not rely on implementers' +self-reports). Run ALL of, reporting each verbatim on failure: +1. pytest -q --no-cov (repo root — all 3 roots, expect ~568+new all green) +2. ruff check . +3. ruff format --check . +4. npx cdk synth po-ingest -q && npx cdk synth workorder-ingest -q + (run inside cdk/; selectors are ARTIFACT IDs, not stack_name) +5. bash -n scripts/post-deploy-smoke.sh; test -x scripts/post-deploy-smoke.sh +6. git status --porcelain — every modified/added path must fall under: + ${EXPECTED_SCOPE.join(', ')} (plus untracked .coverage, + docs/refactor-evaluation.md, .claude/workflows/). List violations. +7. Grep the healthcheck branch in both handlers: confirm it precedes any S3 + get_object and any ses_auth call by line number. +YOU MAY NOT edit any file. passed=true only if every gate is green and scope +is clean.` + +const lenses = [ + { key: 'security', prompt: `${PREAMBLE} +ADVERSARIAL REVIEW — security lens. Try to REFUTE the safety of this diff +(git diff main). Attack: (1) can any S3/SES-delivered email reach the +healthcheck branch or any other new early-return (top-level key forgery via +event shape, weird typing like event={"healthcheck":"true"} vs True)? +(2) does placement before ses_auth weaken fail-closed behavior in ANY path? +(3) does the smoke script or deploy.yaml change introduce injection (unquoted +vars, payload echoed into shell)? (4) does the AST test import or execute +handler code (it must not)? confirmed=true ONLY with a concrete exploit +sketch + file:line.` }, + { key: 'telemetry', prompt: `${PREAMBLE} +ADVERSARIAL REVIEW — telemetry/alarm lens. Read the sender-auth-rejected +metric FILTER PATTERNS in cdk/po_stack.py and cdk/wo_stack.py verbatim, then +try to prove a healthcheck invocation (incl. Lambda platform START/REPORT +lines and any new log text) produces a filter match — that alarm pages at +>=1. Also verify: zero EMF emission on the healthcheck path; ParseMethod +metric contract untouched (PO emits ai_fallback BEFORE the Bedrock call — +must not have moved); no alarm/math changes snuck into po_stack.py beyond +the bundling block. confirmed=true only with file:line evidence.` }, + { key: 'bundling', prompt: `${PREAMBLE} +ADVERSARIAL REVIEW — bundling/CI lens. (1) Enumerate +lambdas/po/email_processor/*.py and prove the new glob ships EXACTLY the +right set vs the old allowlist {handler,ses_auth,template_parser, +derived_fields}.py — flag any extra top-level .py whose shipping is NOT a +proven no-op, and confirm tests/ + package/ cannot match a non-recursive +./*.py. (2) Mutation-test the AST consistency test: temporarily copy the +detection logic and feed it an allowlist string missing derived_fields.py — +does it fail? (do this in /tmp scratch, not by editing repo files). +(3) Smoke script: does it actually catch FunctionError on HTTP 200 (trace the +aws CLI output handling — --query vs jq parsing), and does a missing function +name or region default break it? (4) deploy.yaml: is the post-deploy wiring +consistent with the cd-cdk reusable workflow's actual input names (re-fetch +the pinned file via gh api if needed)? confirmed=true only with 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 === 'critical' || f.severity === 'high' || f.severity === 'medium')) + const mechanicalGreen = checks && checks.passed + log(`Verify round ${round}: mechanical ${mechanicalGreen ? 'GREEN' : 'RED'}, confirmed findings: ${confirmed.length}`) + if (mechanicalGreen && confirmed.length === 0) break + + round += 1 + if (round >= 3) break + phase('Fix') + await agent(`${PREAMBLE} +You are the fix agent — you may edit any Phase-0-owned file listed here: +${EXPECTED_SCOPE.join(', ')}. +Fix EVERY item below with the minimal change; do not expand scope; keep the +pinned contract intact. Re-run the specific failing check/test for each fix. +MECHANICAL FAILURES:\n${checks ? checks.details : '(mechanical agent died — rerun everything)'} +CONFIRMED FINDINGS:\n${JSON.stringify(confirmed, null, 2)}`, + { 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, + openPRs: setup ? setup.facts : [], + } +} + +// ----------------------------------------------------------------- package + +phase('Package') +const readme = await agent(`${PREAMBLE} +YOU OWN: README.md only. Document (in the style of the existing README): +the {"healthcheck": true} direct-invoke contract on both processors, the +post-deploy smoke gate (what it checks, that FunctionError is the signal), +and the PO bundling glob + AST consistency test (replacing the allowlist +note if one exists). Same-commit README updates are a handbook requirement. +Run: ruff format --check . still clean (README is md, but confirm no stray +edits). Return filesChanged.`, + { label: 'package:readme', model: 'sonnet', phase: 'Package', schema: IMPL }) + +const crossReview = await agent(`${PREAMBLE} +The healthcheck branch adds a new event-contract field to both Lambda +handlers — the handbook mandates a cross-family review for handler-signature +/ event-shape changes. Run exactly: + cd ${REPO} && git diff main > /tmp/phase0.diff + python3 ~/Documents/repositories/seahaven/security-review/cross_review.py \ + "Review this diff for breaking changes. Context: procurement-ingest refactor Phase 0 per docs/refactor-evaluation.md — healthcheck early-return added to both email-processor handlers (new top-level event field, direct-invoke only), post-deploy smoke script, PO bundling cp-allowlist replaced with ./*.py glob, AST bundle-consistency test. Diff follows: $(cat /tmp/phase0.diff)" +(cross_review.py is stateless/one-shot — the diff must be inline.) Return its +verdict VERBATIM in summary, and set verdict to one of: PASS / FIX / BLOCK +based on its highest finding. Do not fix anything yourself.`, + { label: 'package:cross-review', model: 'sonnet', phase: 'Package', schema: TEXT }) + +const commit = await agent(`${PREAMBLE.replace('do NOT commit, ', '')} +YOU are the commit agent. Steps: +1. Read ~/Documents/repositories/seahaven/engineering-handbook/commit-messages.md + and follow it exactly. +2. git add: the Phase-0 files (${EXPECTED_SCOPE.join(', ')}), README.md, AND + docs/refactor-evaluation.md (untracked evaluation report — must land with + this first refactor PR or it is lost) AND .claude/workflows/phase-0-deploy-guards.js. + Do NOT add .coverage. Verify with git status that nothing unexpected is staged. +3. ONE commit. Write the message to /tmp/phase0-commit-msg.txt and use + git commit -F /tmp/phase0-commit-msg.txt (backticks in -m get eaten by zsh). + Suggested subject: "feat: deploy-pipeline guards — healthcheck, smoke gate, bundle glob + AST test (refactor phase 0)". + NO AI attribution / Co-Authored-By lines. +4. Do NOT push. Return the commit sha + shortstat in summary.`, + { label: 'package:commit', model: 'sonnet', phase: 'Package', schema: IMPL }) + +return { + status: 'BUILT — committed locally, NOT pushed', + branch: BRANCH, + commit: commit ? commit.summary : 'commit agent died — commit manually', + crossFamilyReview: crossReview ? { verdict: crossReview.verdict, detail: crossReview.summary } : 'NOT RUN — outstanding', + implementation: implOk.map(r => r.summary), + filesChanged: implOk.flatMap(r => r.filesChanged).concat(readme ? readme.filesChanged : []), + verifyRounds: round + 1, + blockers: implBlockers.concat(reconBlockers), + openPRsAtBranchTime: setup ? setup.facts : [], + outstandingGates: [ + '/sh-security-review (MANDATORY before push — handler = untrusted-input surface); run in the main loop on the committed diff', + 'if cross-family verdict is FIX/BLOCK: resolve, re-run cross_review.py on the amended diff', + 'push + PR + gh pr checks green', + 'deploy-then-merge: deploy from branch, smoke green, one real PO + WO email each showing ParseMethod=template, all alarms green, THEN merge', + ], +} diff --git a/.github/workflows/deploy.yaml b/.github/workflows/deploy.yaml index 5a06e5a..ef45e6c 100644 --- a/.github/workflows/deploy.yaml +++ b/.github/workflows/deploy.yaml @@ -17,5 +17,6 @@ jobs: with: python-version: "3.12" cdk-dir: cdk + post-deploy-script: scripts/post-deploy-smoke.sh secrets: deploy-role-arn: ${{ secrets.AWS_DEPLOY_ROLE_ARN }} diff --git a/README.md b/README.md index 701f000..5b440b8 100644 --- a/README.md +++ b/README.md @@ -146,6 +146,30 @@ The `-duration` and `-throttles` alarms for `po-email-processor` and `wo **DynamoDB alarms** (`AWS/DynamoDB`): each owned table gets `-throttles` (`ThrottledRequests`) and `
-system-errors` (`SystemErrors`). These metrics emit only at the `TableName` + `Operation` dimension set, so each alarm is a `Sum` math expression across the operations the table uses (Get/BatchGet/Query/Scan/Put/Update/Delete/BatchWrite). Tables covered: `purchase-orders`, `verified-sites`, `pending-site-review` (po-ingest); `WorkOrders`, `WorkOrderComments` (workorder-ingest). +## Deploy-Pipeline Guards (Phase 0) + +**Goal:** a broken Lambda bundle fails the deploy job, not Monday's first email. + +**Healthcheck direct-invoke contract.** Both `po-email-processor` and `workorder-email-processor` recognize a top-level direct-invoke probe payload `{"healthcheck": true}`. In each `handler(event, context)`, the **very first statements** — before any S3 fetch, before `ses_auth`, before iterating `event["Records"]` — are: + +```python +if isinstance(event, dict) and event.get("healthcheck") is True: + return {"healthcheck": "ok"} +``` + +This placement is deliberate, not incidental: real mail always arrives as an S3 `ObjectCreated` event whose top-level keys (`Records`) AWS controls, so email content can never set a top-level `healthcheck` key — the branch creates no accept path for forged mail. It also emits **no EMF metric and no log line**, so it can never match the `sender_auth_rejected` log-metric-filter pattern that feeds the `-sender-auth-rejected` alarm (see [CloudWatch alarms](#cloudwatch-alarms)) — that alarm pages at ≥1 match in its window, so repeated healthcheck invokes across deploys (two deploys in ~30 min is routine) must never contribute to it. Unit coverage: `lambdas/po/email_processor/tests/test_po_healthcheck.py` and `lambdas/wo/email_processor/tests/test_healthcheck.py`. + +**Post-deploy smoke gate.** `scripts/post-deploy-smoke.sh` is wired into the CD workflow as `cd-cdk.yaml`'s `post-deploy-script` input (see [CI/CD](#cicd)) and runs synchronously after every deploy, before the workflow is considered green. It invokes both `po-email-processor` and `workorder-email-processor` with `aws lambda invoke --invocation-type RequestResponse --payload '{"healthcheck": true}'` (region `us-east-1`) and asserts, per function: + +1. The invoke response's **`FunctionError` field is absent** — this is the load-bearing check. A broken bundle (e.g. an `ImportError` at module init from a missing sibling module) still returns HTTP 200 from the Lambda Invoke API with `FunctionError=Unhandled`; a bare exit-code check on `aws lambda invoke` would false-pass on exactly the failure this gate exists to catch. +2. The returned payload is **exactly** `{"healthcheck": "ok"}`. + +The script runs `set -euo pipefail` and exits non-zero on any invoke failure, any `FunctionError`, or a payload mismatch on either function, failing the deploy job. + +**PO bundling: glob replaces the hand-maintained allowlist.** `cdk/po_stack.py`'s asset bundling command now ships PO's Lambda source with a non-recursive glob, `cp ./*.py /asset-output/`, instead of a hand-maintained list of filenames (`cp handler.py ses_auth.py template_parser.py derived_fields.py /asset-output/`). The glob is functionally identical for today's file set — non-recursive, so `tests/` and other subdirectories are still excluded — but structurally eliminates the failure mode that shipped a broken bundle twice (PR #105 omitted `template_parser.py`; PR #2 nearly omitted `derived_fields.py`): a new sibling module the handler imports now ships automatically instead of requiring someone to remember to add it to the list. WO's bundling (`cdk/wo_stack.py`) already used a recursive `cp -r` of the whole source dir and is unaffected. + +`tests/test_bundle_consistency.py` guards both bundling commands with a pure-AST check (no synth, no boto3, no handler import): it parses each handler.py's top-level first-party sibling imports, extracts the bundling `command=[...]` string from the corresponding CDK stack file, and asserts every required sibling module is guaranteed to ship — recognizing the PO glob and the WO recursive copy as unconditionally-safe shapes, and falling back to literal filename matching for any other (allowlist-style) shape. It also pins the PO command to the glob form specifically, so a future revert back to a filename allowlist that omits a sibling fails CI rather than merely relying on the general detection logic. Runs in the existing pytest step, before synth. + ## Security **Web UI auth (defense-in-depth).** The `po-web-ui` / `workorder-web-ui` handlers refuse unauthenticated requests even though their public Function URLs were removed (INFRA-74). Each requires a shared secret in the `X-Auth-Token` header (or `Authorization: Bearer `), compared in constant time against the configured token. The handler **fails closed** if the token is unset or unreadable (denies all). The token lives in the Secrets Manager secret `procurement-ingest/web-ui-auth-token`; only its ARN is passed to the Lambda (`WEB_UI_AUTH_TOKEN_SECRET_ARN`), and the value is fetched at runtime — never embedded in the CloudFormation template or Lambda env vars. The fetched value is cached in the warm container with a short TTL (5 min) so a rotated secret propagates without waiting for the execution environment to recycle. This is a defense-in-depth floor for a detached URL, not primary auth. @@ -216,7 +240,7 @@ The canonical map of Sea Haven's AWS infrastructure lives in Confluence. This pr GitHub Actions with reusable workflows from `Sea-Haven-Industries/.github` (all pinned to a commit SHA of `main`): - **CI** (`ci.yaml`, PR to `main`): linting + `cdk synth` via `ci-python-sam.yaml` -- **CD** (`deploy.yaml`, push to `main`): CDK deploy via `cd-cdk.yaml` (OIDC auth) +- **CD** (`deploy.yaml`, push to `main`): CDK deploy via `cd-cdk.yaml` (OIDC auth), followed by the synchronous `post-deploy-script: scripts/post-deploy-smoke.sh` healthcheck gate (see [Deploy-Pipeline Guards](#deploy-pipeline-guards-phase-0)) — `cd-cdk.yaml`'s `stack-name` input only accepts one stack, so the smoke script itself enumerates both `po-email-processor` and `workorder-email-processor` - Plus dependency review and PR labeler workflows on every PR Branch protection on `main` — all changes through PR. @@ -254,9 +278,9 @@ pytest ``` 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), and the fail-closed sender-authentication parser (`test_ses_auth.py`, INFRA-107). -- `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`), and the Bedrock-fallback dispatch/EMF-metric behavior with a mocked `invoke_model` (`test_bedrock_fallback.py`). Golden JSON lives under `tests/fixtures/expected/`. -- `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), and the Bedrock-fallback dispatch/EMF-metric behavior (`test_po_bedrock_fallback.py`). +- `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`), and the Phase 0 direct-invoke healthcheck contract (`test_healthcheck.py`). Golden JSON lives under `tests/fixtures/expected/`. +- `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 (`test_po_bedrock_fallback.py`), and the Phase 0 direct-invoke healthcheck contract (`test_po_healthcheck.py`). All three roots are discovered by `pytest.ini` (`testpaths`). @@ -283,20 +307,21 @@ cdk/ lambdas/ po/ # PO pipeline Lambdas email_processor/ - handler.py # template-first + Bedrock fallback, EMF metric, merge writes + handler.py # template-first + Bedrock fallback, EMF metric, merge writes, {"healthcheck": true} early-return template_parser.py # pure deterministic Coupa parser + fail-closed validation gate - tests/ # golden-file + validation-gate + fallback-dispatch tests + fixtures + tests/ # golden-file + validation-gate + fallback-dispatch + healthcheck tests + fixtures site_extractor/ web_ui/ wo/ # WO pipeline Lambdas email_processor/ - handler.py # template-first + Bedrock fallback, EMF metric, #23 comment_id + handler.py # template-first + Bedrock fallback, EMF metric, #23 comment_id, {"healthcheck": true} early-return template_parser.py # pure deterministic parser + fail-closed validation gate - tests/ # golden-file + validation-gate + comment_id + fallback tests + tests/ # golden-file + validation-gate + comment_id + fallback + healthcheck tests web_ui/ 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) tests/ requirements.txt # Test-only deps (moto) @@ -305,4 +330,5 @@ 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 ``` diff --git a/cdk/po_stack.py b/cdk/po_stack.py index 76fa71a..e78adfe 100644 --- a/cdk/po_stack.py +++ b/cdk/po_stack.py @@ -247,12 +247,20 @@ class PoIngestStack(Stack): "-c", "pip install --platform manylinux2014_aarch64 --only-binary=:all: " "-r requirements.txt -t /asset-output && " - # NOTE: every module handler.py imports as a sibling - # MUST be listed here or the deploy ships a Lambda that - # ImportErrors at runtime (bit us for template_parser - # in PR #105 and nearly for derived_fields in PR #2). - "cp handler.py ses_auth.py template_parser.py " - "derived_fields.py /asset-output/", + # NOTE: non-recursive glob (not `cp -r`) so tests/ and + # the stale package/ dir are never shipped -- only + # top-level .py siblings of handler.py. This replaces + # a hand-maintained four-file allowlist that twice + # nearly shipped a broken Lambda (missing + # template_parser in PR #105, nearly missing + # derived_fields in PR #2) because a new sibling + # import wasn't added to the list. The glob makes + # that class of bug structurally impossible; + # tests/test_bundle_consistency.py ast-parses + # handler.py's first-party imports and asserts this + # command ships all of them, so a future revert back + # to an allowlist that omits a sibling fails CI. + "cp ./*.py /asset-output/", ], ), ), diff --git a/docs/refactor-evaluation.md b/docs/refactor-evaluation.md new file mode 100644 index 0000000..45daa59 --- /dev/null +++ b/docs/refactor-evaluation.md @@ -0,0 +1,305 @@ +# procurement-ingest — Refactor Evaluation Report + +**Date:** 2026-07-16 · **Branch audited:** `feature/po-derived-classifier` · **Findings:** 71 (55 confirmed by verification pass, 16 unverified) · **Suite:** 568 passed / 0 failed / 1.47s + +--- + +## 1. Executive summary + +**Verdict: a full rewrite is NOT warranted; a staged consolidation refactor IS — and one security-parity gap must land before (or as the first phase of) any restructuring.** + +The core ingest path is in good shape: template-first parsers with fail-closed gates, 93% coverage on the tested tree, 0 ruff violations, clean CI. The problems are structural, not functional: + +1. **Security drift between twin pipelines (fix first, not a refactor).** The AI-fallback hardening from #104 (XML-delimited prompt, tag neutralization, `validate_ai_fallback` fail-closed gate, `ai_fallback_rejected` metric + alarm) exists **only in WO**. PO's `extract_with_claude` sends the raw untrusted body bare after the prompt and routes unvalidated LLM output straight to `save_cancellation`/`save_new_po` — a DKIM-passing injected `{"email_type":"cancellation"}` sticky-cancels a live PO. This is the canonical drifted-copy failure and the strongest argument for consolidation. +2. **Duplication with no shared home.** `ses_auth.py` is a byte-identical 452-line copy (sync policed by a hand-parameterized test fixture); the web_ui auth gate is a byte-identical ~61-line block; the EMF emitter is copied 3×; ~379 identical lines sit between the two CDK stacks. The handbook's `lambdas/shared/` location is unused, blocked today by per-function `Code.from_asset` roots that can't reach it. +3. **The deploy pipeline cannot catch the refactor's main failure mode.** The hand-maintained `cp` allowlist in po_stack bundling has already caused production ImportErrors (PR #105); deploys run zero tests; cd-cdk's pre-flight/health-check steps are dormant (no stack-name passed); detection of a broken bundle is traffic-dependent. **Guards must land before any packaging change.** + +**Shape:** ~11 PR-sized phases over the sequence *guards → security parity → bundling-root move → shared-code extraction → CDK dedup → handler decomposition → site_extractor reconciliation → test/doc consolidation*. Every phase is independently deployable and revertible per deploy-then-merge. Rough scope: ~2–3 weeks of focused work; phases 0–3 are the high-value 20% that eliminates the drifted-copy failure class. + +**Hard constraints respected throughout:** fail-closed gates unchanged or strengthened (never weakened); shadow telemetry observe-only (no `derived_fields` behavior change until the bake concludes); ParseMethod EMF / fallback-alarm contracts preserved (with the two deliberate per-pipeline emission-ordering differences pinned by tests, not accidentally "fixed"); all CDK changes either provably logical-ID-safe or gated on a zero-change `cdk diff`. + +--- + +## 2. Current-state assessment (measured) + +### Tests +| Root | Tests | +|---|---| +| `tests/` (cross-pipeline) | 117 | +| `lambdas/wo/email_processor/tests/` | 143 | +| `lambdas/po/email_processor/tests/` | 308 | +| **Total** | **568 passed, 0 failed, 1.47s** | + +- `pytest.ini` is the only config; `test_local.py` deliberately excluded (imports handler → boto3 clients at collection). CI runs bare `pytest` at repo root so all three roots are collected; no coverage step in CI. +- **Three divergent module-loading mechanisms** (tests/conftest importlib loader with sys.modules save/restore; `_po_parser_support.py` independent importlib reimplementation + load-bearing `import moto` ordering; `_wo_parser_support.py` bare sys.path `import handler`) exist solely because both pipelines duplicate bare module names. + +### Coverage (pytest-cov 7.1.0, Python 3.12.13) +| Module | Coverage | +|---|---| +| po/derived_fields | 95% (296 stmts) | +| po/handler | 96% (186) | +| po/ses_auth · wo/ses_auth | 94% (223 each) | +| po/template_parser | 89% (494) | +| wo/handler | 96% (139) | +| wo/template_parser | 95% (274) | +| **Tested-tree total** | **93% (3,356 stmts)** | +| po/web_ui, wo/web_ui, po/site_extractor, scripts/ | **0% — 540 stmts, zero test references** | +| **True all-first-party coverage** | **~80% (3,896 stmts, 777 miss)** | + +Gotcha: naive `--cov=lambdas` silently omits web_ui/site_extractor (missing `__init__.py`) — the 93% headline overstates reality. + +### Duplication (difflib line-level, autojunk off) +| Pair | Similarity | +|---|---| +| ses_auth.py (po vs wo) | **100% — byte-identical, 452 lines** | +| web_ui/handler.py | 59.6% (212 common lines; auth block byte-identical) | +| cdk/po_stack.py vs wo_stack.py | 59.9% (379 identical lines measured) | +| email_processor/handler.py | 30.9% | +| template_parser.py | 15.4% (structurally parallel, legitimately divergent) | + +### Lint +- ruff 0.15.12: `ruff check .` → 0 violations; `ruff format --check` → 135 files clean. +- **Caveat:** there is no ruff config file, so PLR/C901 are never selected — the three `noqa: PLR09xx` suppressions on `_validate_new_po_values` are inert, and `extract_new_po` (C901=35) passes lint for the same reason. "0 violations" reflects default rules only. + +### Dead weight +- Untracked 44 MB `lambdas/po/email_processor/package/` — pre-Bedrock vendored anthropic SDK + stale handler. Not in any build; copied into cdk.out staging on every synth. + +--- + +## 3. Refactor plan — phased PR sequence + +Ordering principle: **guards first → byte-identical consolidation → behavior-preserving extraction → structural moves last.** Each phase deploys and verifies (deploy-then-merge per `git-workflow.md`) before the next merges. Rollback at every step: `git revert` + push (auto-redeploy ~5–10 min) or local redeploy of previous commit; dropped mail recoverable via DLQ (14 d) and S3 `inbound/` replay (90 d). + +### Phase 0 — Deploy-pipeline guards (PR-0) · effort: S · risk: low +**Goal:** make a broken bundle fail the deploy job, not Monday's first email. +- Both handlers: healthcheck early-return branch (`{"healthcheck": true}` → ok) placed **before** S3 fetch and SES-auth so it neither creates an accept path nor trips the `sender_auth_rejected` substring metric-filter alarm (which pages at ≥1 reject, 2-of-6 × 5 min — two deploys in ~30 min would false-page otherwise). +- `scripts/post-deploy-smoke.sh`: **synchronous** invoke (`RequestResponse`) of both processors, checking the `FunctionError` response field (init ImportError returns HTTP 200 + `FunctionError=Unhandled` — exit-code checks false-pass). Wire via cd-cdk's `post-deploy-script` input (stack-name input takes only one stack; this repo has two). +- Replace PO's four-file `cp` allowlist with `cp ./*.py /asset-output/` (identical output today; non-recursive glob excludes tests/). +- CI-time ast consistency test: extract handler.py's first-party sibling imports, assert each ships in the bundle. Runs in the existing pytest step, before synth. +- **Gates:** deploy, smoke green, one real PO + WO email each showing `ParseMethod=template`, all alarms green. Handler touch = untrusted-input surface → **`/sh-security-review` mandatory**; new event field in handler → run cross-family review (`cross_review.py`) for the handler-contract change. + +### Phase 1 — PO AI-fallback security parity (port of #104) · effort: M · risk: medium +**Goal:** close the highest-impact confirmed finding; make pipelines structurally symmetric. +- `lambdas/po/email_processor/`: add PO-specific `validate_ai_fallback` (NOT a WO copy — PO contract is nested: `CONTRACT_KEYS` exact key-set with missing-key normalization, `po_number` vs `_PO_ID_RE` (protects the DynamoDB partition key built at handler.py:523/621), `email_type` ∈ {new_po, revision, cancellation}, Decimal/int/None money types since PO parses with `parse_float=Decimal`). Call immediately after `extract_with_claude`, before `enrich_parsed`/dispatch; on failure emit `ParseMethod=ai_fallback_rejected` then `continue` (skip, never raise — avoids DLQ churn on attacker-controlled input). +- Port `` data-block wrapping + `_EMAIL_TAG_RE` tag neutralization + `temperature=0` into `extract_with_claude`. +- **Preserve the deliberate PO metric ordering:** PO emits `ai_fallback` BEFORE the Bedrock call (so Bedrock-side errors still record the outcome — handler.py:656-669). Do not move it. `ai_fallback_rejected` is an additive second datapoint on rejection; document the intentional double-count. +- `cdk/po_stack.py` in the same PR: (a) fold `FILL(rej,0)` into the existing fallback-rate MathExpression numerator+denominator **inside** the IF volume floor (in-place property update to the existing alarm logical ID — safe; respect the post-#102 no-element-wise-MAX rule at po_stack.py:416-418); **exclude the rejected series from the rate numerator or account for the pre-call double-count** — do not copy WO's fb+rej math verbatim, it double-counts PO rejections. (b) net-new `EmailProcessorAiFallbackRejectedAlarm` — **retune for ~57 emails/day** (WO's 5-min/30-min sparse idiom is structurally dead at PO volume; use 1h–6h periods like the existing PO fallback-rate retune). +- Tests (mirroring WO one-for-one): non-dict model output, injected po_number (`123#x`, fullwidth digits), injected email_type (assert no save_* AND no misroute into `save_new_po` via the else branch), injected status, XML-wrap, forged-tag neutralization, linear-time regex. +- README: fix the false "Data is never corrupted" PO claim (line 29) and add `ai_fallback_rejected` to the PO ParseMethod list (line 145). +- **Gates:** `/sh-security-review` (untrusted-input, mandatory), full suite, `npx cdk synth po-ingest`, deploy-then-merge with live-email verification of both metric series. + +### Phase 2 — Bundling-root move, no code move (PR-1 of migration) · effort: S · risk: medium (deploy-mechanics only) +**Goal:** make `lambdas/shared/` reachable before anything moves into it. +- Both stacks: `Code.from_asset("../lambdas")` with `command: cp po/email_processor/*.py /asset-output/` (resp. wo) and `pip install -r po/email_processor/requirements.txt` (the `-r` path change is required — bundling cwd is the asset root). Add `exclude=['**/__pycache__/**','**/tests/**','**/package/**']` — `from_asset` does NOT honor .gitignore, and the stale 44 MB `package/` dir would otherwise diverge local vs CI asset hashes. +- WO note: its current `cp -r .` ships tests/ (real scrubbed .eml fixtures), `__pycache__`, and requirements.txt in the prod zip. Accept and document the file-list shrinkage as deliberate cleanup; verification criterion for WO is "runtime-imported module set unchanged + smoke", byte-identical zip diff applies to PO only. +- Also add `exclude` to the three plain `from_asset` calls (po web_ui :502, site_extractor :581, wo web_ui :548) — local `__pycache__` currently makes their hashes nondeterministic. +- **Gates:** `aws lambda get-function` zip file-list diff (PO identical; WO shrinkage reviewed), smoke, one real email per pipeline. Zero handler diff. + +### Phase 3 — `lambdas/shared/` extraction (PR-2) · effort: M · risk: medium +**Goal:** one canonical copy per bare module name where copies are byte-identical or trivially superset-able. +- Create `lambdas/shared/` (handbook `cdk-project-layout.md`). Move in dependency-risk order: + 1. **`ses_auth.py`** (byte-identical; the prime target — security-critical auth that currently requires every hardening fix to land twice). Add `cp shared/*.py /asset-output/` to both bundling commands; module lands flat so `from ses_auth import authenticate_inbound_email` is unchanged — zero handler diff keeps fail-closed auth byte-identical. + 2. **`web_ui_auth.py`**: the byte-identical block (`_get_auth_token`/`_header`/`is_authenticated` + the four cache globals). The web_ui functions have no bundling today — add the same staging mechanism. Keep the per-stack INFRA-74 comments in each handler (they've already drifted in wording; they're stack-specific). + 3. **`email_parsing.py`**: `parse_raw_email` superset returning `cc` unconditionally (PO ignores it — harmless). + 4. **`emf.py`**: generic emitter parameterized by namespace/dimension-sets/properties, serving all **three** hand-built `_aws` envelopes (both ParseMethod emitters + `_emit_derived_agreement_metric`). Dimension-set list `[["ParseMethod"],["ParseMethod","TemplateId"]]` is load-bearing for the alarms; a shared function makes one-sided dimension fixes impossible. +- **Do NOT move:** `template_parser.py` (990 vs 508 lines, genuinely divergent — stays per-pipeline in `_SIBLING_MODULES`), the Bedrock extraction functions (divergent contracts; share only the invocation/prompt-delimiting layer, parameterizing `json.loads` kwargs, `max_tokens` 2048/1024, prompt), per-pipeline `validate_ai_fallback` gates. +- Test plumbing in the same PR: update `_SIBLING_MODULES` resolution, `_po_parser_support.py:71`, the fixture-hygiene test, drop the `ses_auth` fixture params (halves the 448-line test_ses_auth run). The sys.modules save/restore dance **survives for template_parser** — it does not shrink to nothing. +- **Gates:** full suite, smoke, deploy-then-merge watching both sender-auth-rejected alarms through live mail. The post-merge CI redeploy being a no-op (unchanged asset hash) is itself a verification signal. Auth code moved → `/sh-security-review`. + +### Phase 4 — `cdk/common.py` dedup · effort: M · risk: medium (logical-ID discipline) +**Goal:** collapse the 379 identical CDK lines. +- Extract as **plain functions taking `(scope, id, ...)` called with the SAME scope (the Stack) and SAME construct ids** — 100% logical-ID-safe. **Do NOT wrap in Construct subclasses**: that inserts a tree node, changes every child logical ID, and would attempt replacement of the RETAIN-protected `purchase-orders`/`WorkOrders` tables and named buckets. +- Contents: `_DDB_ALARM_OPERATIONS` + `add_ddb_alarms`, `add_sender_auth_rejected_alarm`, `add_standard_lambda_alarms(scope, id_prefix, fn, name_prefix, topic, *, duration_statistic, errors=True, dlq=None, descriptions=...)` (variance to preserve: PO p99 vs wo-email-processor p95, po-web-ui throttles+duration only, site_extractor no-DLQ, **wo web_ui has zero alarms — do not silently add any**; bespoke description strings passed verbatim), `make_bedrock_invoke_statement` (derive the inference-profile ARN from `Stack.account`/`Stack.region` instead of hardcoding 328440206208), `make_email_bucket`, `make_processor_dlq`, `make_fallback_rate_alarm(namespace, rejected_included, period, threshold, floor, evaluation_periods, datapoints_to_alarm)` reproducing expression strings/FILL/labels byte-for-byte. WO's rejected alarm stays a WO-only call (until Phase 1's PO twin). +- Do not import stack-specific services (kms/ssm/event_sources) into common.py. +- **Gates:** `cdk diff` on BOTH stacks showing **zero changes** (the acceptance test); moving IAM PolicyStatement construction → mandatory **cross-family review** (`cross_review.py`) even though semantics are identical. +- Same PR: add `account='328440206208'` to both `cdk.Environment` calls, pin `constructs==` exact, fix the stale "2.259.0" comments, add CfnOutputs for the five function ARNs + consumed table names (net-new — ID-safe). + +### Phase 5 — Handler decomposition + lazy boto3 clients · effort: M · risk: medium +**Goal:** break the God-modules along the seams that already work (ses_auth/template_parser/derive_all prove the flat-sibling pattern). +- PO: `handler.py` (event loop + auth + routing) / `extraction.py` (parse_raw_email shim, extract_with_claude, prompt import) / `enrichment.py` (enrich_parsed, pad_zip — PO-only, WO has no enrichment stage) / `telemetry.py` / `persistence.py` (`_write_fields`/`_merge_update`/save_*; collapse the byte-identical `save_new_po`/`save_revision` into one `_save_merge`). WO: ~5 concerns, not 7 — its split must keep `validate_ai_fallback` + the `[0-9]+` work_order_id key guard in the handler loop ahead of both saves, and keep `_header_date_iso`/comment_id determinism with persistence. +- Move `EXTRACTION_PROMPT` (181/693 lines of po handler) to `prompts.py` with cross-reference headers to derived_fields; keep a re-export since 4 tests dereference `handler.EXTRACTION_PROMPT`. +- Lazy cached boto3 accessors in I/O modules; pure modules import no boto3. Update the `monkeypatch.setattr(handler, "dynamodb", fake)` patch surface in the same change. Preserve the **moto-before-handler import ordering** (`_po_parser_support.py:26-33`) or moto-backed suites hit real AWS. +- **Behavior-preservation obligations stated per pipeline:** PO metric before Bedrock call; WO metric after gate with mutually-exclusive `ai_fallback`/`ai_fallback_rejected`; shadow telemetry ai_fallback-only. Bundling: the glob from Phase 0 already ships new siblings automatically; the ast test verifies. +- **Gates:** full suite (goldens unchanged), smoke, one live email per pipeline. Handler decomposition keeps signatures — no cross-family review needed unless the event/return contract changes. + +### Phase 6 — site_extractor reconciliation · effort: M · risk: medium · **timing: after the shadow bake concludes** +**Goal:** end the three-way site_code contradiction (KLAL-class all-letter codes currently can NEVER self-register — permanent pending-review rows). +- `extract_site_code` already prefers `record["site_code"]` (line 36) — the defect is the digit-requiring `SITE_CODE_PATTERN.match` (prefix-anchored, rejects KLAL, accepts overlong junk like `DLI6X`). Fix: validate the direct field with the canonical `derived_fields` shape + skip-list semantics (fullmatch), import `derive_site_code` for the fallback path (ship `derived_fields.py` into the site_extractor asset via the Phase 2/3 mechanism), delete the bespoke regex ladder (line 62's `[A-Z]{4,5}` alternation is dead code). Keep shape/skip validation on the ai_fallback-sourced field rather than blind trust (LLM value is authoritative during the bake). Keep parse_address + pending-review flow as-is. +- Repoint `scripts/backfill_sites.py` at `derive_site_code` (or delete it — one-time script). +- Characterization tests FIRST (pin current behavior, including the KLAL divergence, before changing it) — see §4. +- **Gates:** new site_extractor test suite, deploy, watch pending-site-review write rate drop. + +### Phase 7 — Ops/recovery + dependency hygiene · effort: S · risk: low (can run parallel to 4–6) +- Generalize `scripts/reprocess.py`: `--pipeline po|wo`, `--key/--prefix/--since`; targeted replay primary, full-prefix demoted behind `--all` with documented caveats (Event-type invocation is concurrent → sorting doesn't serialize; switch to RequestResponse if order matters; metrics double-counted; Bedrock re-billed; out-of-order replay regresses merged fields). +- `docs/runbook-dlq-recovery.md`: async-destination DLQ has **no redrive-to-source**; procedure = receive-message → key from event body → targeted re-invoke → verify → purge. State the windows: 14 d DLQ breadcrumb, 90 d raw-email S3 (lifecycle overrides RETAIN). Document that sender-auth and ai_fallback_rejected drops intentionally never reach the DLQ. Link from README alarms section. +- Dependency hygiene: drop vendored boto3 (both email-processor requirements → handbook empty-with-comment form; only-boto3 functions correctly use the runtime copy per `lambda-template.md`); simplify bundling to cp-only; exact-pin `moto==` and add `/tests`, `/lambdas/po/web_ui`, `/lambdas/po/site_extractor` dependabot entries (pin first — floor pins make dependabot entries no-ops); reduce wo/web_ui's dead manifest to empty-with-comment. +- Delete the 44 MB `package/` dir (one benign asset-hash redeploy; do it before/with Phase 2's excludes). + +### Phase 8 — Test-root consolidation (last PR — validates the new boundaries) · effort: M · risk: low +See §4. Also the small correctness/doc items batched here: WO `invalid_status` reason-code fix (+ grep dashboards for `malformed_site_code` first), WO Bedrock-error metric fix (wrap the call: emit `ai_fallback`/`bedrock_error` in an except-and-reraise — NOT a naive reorder, which double-counts against wo_stack's "rejected emits nothing else" alarm contract), `_validate_new_po_values` per-rule split (V4 anchor-frame dataclass must carry `summary_matches`/`price` for V13; add a ruff config enabling PLR/C901 or the noqa-drop is meaningless; include `extract_new_po` C901=35), docstring/README drift batch (findings 33–38 appendix refs). + +--- + +## 4. Test-hardening plan + +### Target layout — resolving the two-roots question +**Keep both roots; fix the loading.** The split itself is principled (per-lambda golden suites beside the code; cross-pipeline suites at top). Changes: +- **One loader.** New **repo-root `conftest.py`** (required anyway for package mapping; `tests/conftest.py` does not load for standalone `pytest lambdas/po` runs, so it cannot carry session invariants): dummy AWS env, **moto BUILTIN_HANDLERS registration before any handler import** (with the explanatory comment currently buried in `_po_parser_support.py:33`), and a single `load_lambda_module(pipeline, name)` keeping one copy of the sys.modules save/restore (still needed for template_parser's duplicated bare name). +- Rewrite `_wo_parser_support.py` off the bare-import strategy (it is the source of the collision the other two loaders defend against). Shared `tests/support/` package: superset `FakeTable` (update_item + WO's put_item/keyed store), `FakeDynamoResource`, `load_email`, `load_golden` **keeping PO's `parse_float=Decimal`** (load-bearing for exact money comparison). +- File moves: `test_po_merge.py`, `test_pad_zip.py` → `lambdas/po/email_processor/tests/`. `test_parse_raw_email.py` and `test_ses_auth.py` **stay at root** (genuinely cross-pipeline, parameterized over both handlers). +- Delete `test_local.py` (globs a nonexistent `samples/`, WO-only, bypasses the gate; the golden suites cover its role) or rewrite with `--pipeline` — deletion preferred. + +### Missing scenarios by module (priority order) +1. **PO AI-fallback negatives** (with Phase 1): non-dict output (list/str/int/None — today AttributeErrors at the logger f-string into retries/DLQ), injected po_number/email_type/status, XML-wrap + neutralization + linear-time regex, markdown-fence stripping (currently untested in PO). +2. **Bedrock transport errors, both pipelines:** ThrottlingException (assert PO's pre-call metric survived + no partial write + exception propagates into the errors-alarm/DLQ path — this doubles as the only real proof of PO's metric-before-call ordering), missing `content` key, empty content list, non-JSON model text. **Pin WO's no-datapoint-on-throttle behavior with a documenting test** (do not "fix" by reordering — see Phase 8 note). +3. **Handler-level SES auth seam:** today every dispatch test monkeypatches `authenticate_inbound_email=True`; deleting the gate line would pass all 568 tests. Add per pipeline: reject-path with no auth monkeypatch + empty `ALLOWED_DKIM_DOMAINS` (assert zero Bedrock calls, zero writes, no raise; env is read at call time so setenv suffices; FakeS3 still needed — the gate sits after get_object). Accept-path is turnkey for PO (scrubbed Coupa fixtures authenticate); WO needs an SES-stamped fixture synthesized from `WO_SES_HEADER` in test_ses_auth. +4. **web_ui (both, 0% today — auth is the mandatory-review surface):** fail-closed on unset ARN (requires module reload — ARN read at import), fail-closed on Secrets Manager exception, TTL cache refresh (reload fixture resets globals), Bearer/X-Auth-Token/case-insensitivity, wrong-token 401, **401 without table scan**, non-ASCII token (hmac.compare_digest TypeError → 500 today, worth pinning/fixing), hostile-field escaping (regression lock — escaping is currently correct on inspection). PO web_ui lacks `__init__.py` — use the loader, not package imports. +5. **WO merge semantics:** `tests/test_wo_merge.py` moto-backed mirror of test_po_merge (table name `WorkOrders`, not kebab): null-status never clobbers wo_status, created_at immutable via if_not_exists, status→wo_status mapping, None fields absent from SET, record_type only-when-present. +6. **site_extractor characterization (before Phase 6):** extract_site_code shapes incl. KLAL divergence and prefix-match overlong acceptance (`SNY55`), parse_address variants, stream routing (INSERT/MODIFY/REMOVE, pending-review fallback), upsert_site SS-ADD expression. +7. **Small pins:** PO-DC-02 64-char EMF clamp regression test (~10 lines in test_po_derived_wiring); multi-record event failure-isolation test per pipeline (documents the all-or-retry contract); reprocess.py synthetic-event-shape contract test (also pins "raw key, no URL-decoding"). + +### Fixtures/goldens +Golden .eml + expected-JSON suites are the model — extend, don't replace. Shared `tests/support/` loaders; per-pipeline fixtures stay per-pipeline (real scrubbed samples). WO gains one SES-stamped fixture (synthesized header block, not new scraped mail). + +### CI changes +- Add `--cov` with an explicit module list (or add `__init__.py` so `--cov=lambdas` stops silently skipping web_ui/site_extractor); fail-under once web_ui/site_extractor suites exist. +- Add the Phase 0 ast bundle-consistency test to the standard run. +- Add ruff config enabling C901/PLR so complexity ceilings are enforced, not decorative. +- Lint scope currently omits `scripts/` and `test_local.py` — include scripts/ (or delete test_local.py and moot half of it). +- Keep the strict `ci / ci` required check; **no admin-bypass pushes to main while migration PRs are in flight** (bypass_mode=always currently allows a zero-CI production deploy). + +--- + +## 5. Findings appendix + +55 confirmed (survived the verification pass, several with sharpening corrections noted in §3/§4), 16 **unverified** (plausible on the evidence given but not independently re-verified — treat as candidates, not commitments). No finding was refuted outright; verification corrections were incorporated into the plan above. + +### Duplication / drifted copies +| # | Finding | Verdict | Impact | +|---|---|---|---| +| D1 | AI-fallback hardening (#104) landed in WO only; PO fallback already drifted behind it | confirmed | high | +| D2 | ses_auth.py byte-identical 452-line copy, sync policed by test fixture | confirmed | high | +| D3 | web_ui fail-closed auth gate byte-identical ~61-line block in both handlers | confirmed | high | +| D4 | No lambdas/shared/ despite 4 duplicated modules (handbook location unused; "prescribes" softened to "provides, and the need condition is met") | confirmed | high | +| D5 | EMF ParseMethod emitter near-identical copy, already drifting (actually 3×, incl. derived-agreement emitter) | confirmed | medium | +| D6 | parse_raw_email drifted copy (WO Cc delta is intentional; poor standalone ROI — fold into D2's plumbing) | confirmed | medium | +| D7 | Bedrock-fallback test suites drifted mirroring source drift (WO has 8 tests PO lacks, not 4) | confirmed | medium | +| D8 | template_parser pair: extract only common substrate (_plain_lines etc.), do NOT merge parsers | **unverified** | low | +| D9 | save_new_po / save_revision byte-identical bodies | **unverified** | low | +| D10 | Three hand-rolled DynamoDB SET-expression builders | **unverified** | low | +| D11 | test_local.py + backfill_sites.py entrench single-pipeline layout | **unverified** | low | + +### Architecture +| # | Finding | Verdict | Impact | +|---|---|---|---| +| A1 | PO ai_fallback path has no validation-gate module (structural parity gap; unvalidated po_number becomes partition key; sticky-Cancel blast radius) | confirmed | high | +| A2 | Both email-processor handlers are God-modules (WO ~5 concerns, not 7) | confirmed | high | +| A3 | site_extractor is a third, contradictory site-code classifier (all-letter codes can never self-register) | confirmed | high | +| A4 | Module-import boto3 clients force the bespoke loader tax (paid 3×) | confirmed | medium | +| A5 | Trade/site/fiscal rules in two authoritative texts (prompt + derived_fields); keep independent during bake | confirmed | medium | +| A6 | Two test roots, three loading idioms, load-bearing moto import order | confirmed | medium | +| A7 | web_ui interleaves auth/data/HTML rendering | **unverified** | low | + +### Code quality / consistency +| # | Finding | Verdict | Impact | +|---|---|---|---| +| Q1 | PO fallback lacks fail-closed gate + injection hardening (consistency view of D1/A1; write path uses non-null-key spray vs WO's whitelist) | confirmed | high | +| Q2 | site_extractor superseded by derived_fields (skip-list sub-claim corrected: real divergence is prefix-match false-accepts + dead all-letter alternation) | confirmed | medium | +| Q3 | _validate_new_po_values 294-line monolith, 60 returns; noqa suppressions currently inert (no ruff config); extract_new_po C901=35 is the worse peer | confirmed | medium | +| Q4 | WO gate misleading reason codes ("malformed_site_code" for bad status — and validate_ai_fallback already returns "invalid_status" for the same failure, so one failure → two codes by path) | confirmed | medium | +| Q5 | Stale 44 MB package/ vendored tree (real cost: synth staging + docker mount + asset-hash divergence, not sys.path shadowing) | confirmed | medium | +| Q6 | web_ui full-table scan + in-memory sort + magic 500 (page-cap option rejected — scan order isn't recency; GSI or accept arbitrary order) | confirmed | medium | +| Q7 | test_local.py rotted (missing samples/, WO-only, bypasses gate) | **unverified** | low | +| Q8 | Type hints absent/degenerate on newest modules | **unverified** | low | +| Q9 | _classify_desc C901=21 priority ladder — defer past bake | **unverified** | low | +| Q10 | WO template_parser function-local imports / sentinel-after-use | **unverified** | low | +| Q11 | backfill_sites.py sys.path hack keeps stale extractor alive | **unverified** | low | + +### Tests +| # | Finding | Verdict | Impact | +|---|---|---|---| +| T1 | PO ai_fallback: zero negative tests for hostile model output | confirmed | high | +| T2 | Bedrock transport errors untested both pipelines; metric-ordering divergence unpinned (WO reorder would break its alarm contract — pin, don't move) | confirmed | high | +| T3 | Handler-level SES auth wiring never tested (every dispatch test bypasses) | confirmed | high | +| T4 | web_ui completely untested incl. auth gate (escaping itself verified correct — tests are regression locks) | confirmed | high | +| T5 | Three divergent test-bootstrap loaders + byte-identical fake-Dynamo helpers (only FakeDynamoResource/_stems identical; FakeTable/load_golden intentionally divergent — superset needed) | confirmed | medium | +| T6 | WO save_work_order merge semantics no direct tests | confirmed | medium | +| T7 | Loader choreography: moto import-order invariant enforced only by comments (not breakable by reordering today; maintenance landmine) | confirmed | medium | +| T8 | PO-DC-02 64-char clamp no regression test | **unverified** | low | +| T9 | Multi-record S3 event batch-abort semantics untested | **unverified** | low | +| T10 | test_rule7_bad_status locks in the wrong reason code | **unverified** | low | +| T11 | scripts/ untested and exclusion undocumented | **unverified** | low | + +### CDK / infrastructure +| # | Finding | Verdict | Impact | +|---|---|---|---| +| C1 | ~400 (measured 379) identical lines between stacks; extract plain-function cdk/common.py; Construct-wrapping forbidden (logical-ID replacement of RETAIN tables) | confirmed | high | +| C2 | PO fallback-rate alarm omits rejected series; no PO rejected alarm (retune for 57/day — WO's sparse idiom structurally dead at PO volume) | confirmed | high | +| C3 | Alarm helpers duplicated verbatim; per-function alarm boilerplate 13+9 sites (variance to preserve: p99/p95, per-function alarm subsets, wo web_ui has none) | confirmed | medium | +| C4 | Duration statistic drift p99 vs p95, no rationale | **unverified** | low | +| C5 | WorkOrders/WorkOrderComments naming violates kebab-case but replacement-locked → document as legacy, don't rename | **unverified** | low | +| C6 | constructs floor-pinned; stale version comments | **unverified** | low | +| C7 | cdk.Environment omits account; Bedrock ARN hardcodes it | **unverified** | low | +| C8 | No function-ARN CfnOutputs; WO stack has zero outputs | **unverified** | low | + +### Contracts & docs +| # | Finding | Verdict | Impact | +|---|---|---|---| +| X1 | README "Data is never corrupted" false for PO fallback path (docs/po-template-parser.md contradicts it via issue #101) | confirmed | high | +| X2 | Three inconsistent site_code definitions; stream field-contract subsection missing (README does list site_extractor as consumer — gap is field-level) | confirmed | high | +| X3 | WO emits ParseOutcome after Bedrock → throttle drops record from alarm denominator, contradicting README ("emit-before" fix rejected — double-counts; use except-and-reraise with bedrock_error reason) | confirmed | medium | +| X4 | Stale quantity/price prompt-type comments; doc self-contradicts §8 vs §10 | confirmed | medium | +| X5 | README omits derived-field suites/derived_fields.py/docs/; 64-char clamp undocumented | confirmed | medium | +| X6 | derived_fields "FAITHFUL v1 port" docstring false (contradicts its own line-100 backtest comments); reword to spec-superset + keep runtime-authority nuance | confirmed | medium | +| X7 | Web UI invocation contract stale ×3; README example guaranteed 401 | confirmed | medium | +| X8 | verified-sites contract: lat/long/notes provenance unknown; INFRA-138 by-state GSI breach live (restore would be a sparse GSI — relevant to the decision) | confirmed | medium | + +### Deploy pipeline & bundling (gap dimension) +| # | Finding | Verdict | Impact | +|---|---|---|---| +| P1 | Deploy runs zero tests; cd-cdk pre-flight/health-check dormant (no stack-name); admin bypass allows zero-CI production deploy | confirmed | high | +| P2 | cp allowlist invisible to every gate → production ImportError class (bit PR #105); glob + ast test | confirmed | high | +| P3 | Migration sequencing: guards → root-move → extraction, each deployed/verified (PR-1 corrections: WO zip not byte-identical; pip -r path change) | confirmed | high | +| P4 | Divergent bundling strategies; WO ships tests/ + __pycache__; from_asset ignores .gitignore (hash-poisoning is in WO cp -r and the three non-bundled assets, not PO's allowlist) | confirmed | medium | +| P5 | Broken-bundle detection traffic-dependent; no synthetic canary (even with stack-name set, CFN status check passes a broken bundle — sync invoke + FunctionError check is the load-bearing part) | confirmed | medium | + +### DLQ / recovery (gap dimension) +| # | Finding | Verdict | Impact | +|---|---|---|---| +| R1 | No WO recovery tooling — reprocess.py hardcoded PO-only; async-destination DLQ has no console redrive, so scripted re-invoke is the ONLY path | confirmed | high | +| R2 | No DLQ recovery runbook; 14-day-DLQ vs 90-day-S3 windows stated nowhere | confirmed | high | +| R3 | reprocess.py --execute: full-prefix replay not order-safe (Event invocation = concurrent; sorting alone doesn't serialize); same-object replay idempotent | confirmed | medium | +| R4 | Batch-abort → whole-event DLQ message; redrive re-runs succeeded records (safe by idempotency, undocumented; do NOT add per-record swallowing) | confirmed | low | + +### Dependency hygiene (gap dimension) +| # | Finding | Verdict | Impact | +|---|---|---|---| +| H1 | Floor-pinned boto3 in Docker bundles → non-reproducible artifacts; vendored copy entirely unnecessary (runtime boto3 suffices) | confirmed | high | +| H2 | Target-state manifest table; layer-vs-shared-dir Dependabot implication (shared/ source dir with no manifest needs no new entry) | confirmed | medium | +| H3 | po/web_ui + site_extractor: no requirements.txt, invisible to Dependabot (cite cdk-project-layout.md, not lambda-template's SAM rationale) | confirmed | low | +| H4 | wo/web_ui requirements.txt is a dead manifest (10 no-op Dependabot PRs merged; file ships as dead weight in the zip) | confirmed | low | +| H5 | tests/requirements.txt uncovered + floor pin (pin first or the dependabot entry is a no-op) | confirmed | low | + +--- + +## 6. Explicitly out of scope / rejected + +| Rejected | Why | +|---|---| +| **Merging the two template parsers** | 15.4% similarity; extractors and validation rules are genuinely domain-specific (Coupa PO vs Hexagon WO). Only the byte-identical low-level helpers are extraction candidates, and even that is low-impact/unverified (D8). | +| **CDK Construct-subclass composition** | Changes every child logical ID → CloudFormation replacement of RETAIN tables and create-failure on named buckets. Plain functions with unchanged scope/ids, gated on zero `cdk diff`, achieve the dedup safely. | +| **Lambda layer for shared code** | Creates a cross-stack export (layer owned by one stack, consumed by the other) that blocks independent updates under `cdk deploy --all`. Asset-root widening keeps each function self-contained. Revisit only if shared code grows real third-party deps. | +| **Renaming WorkOrders/WorkOrderComments/non-prefixed resources to kebab-case** | table_name/function_name changes force resource replacement (production data; cross-stack `fromTableName` consumers). Correct move is a handbook Legacy-section entry; revisit at the INFRA-6 CMK table touch. | +| **Reordering WO's emit_parse_metric before the Bedrock call ("PO parity")** | Would double-count gate-rejected emails against wo_stack's fallback-rate MathExpression and violate the documented "rejected emits nothing else" contract. Fix is except-and-reraise with a `bedrock_error` reason code + a pinning test. | +| **Moving PO's pre-Bedrock metric emission after the gate** | The pre-call emit is deliberate (Bedrock errors still record ai_fallback) and load-bearing for the alarm; ai_fallback_rejected is additive instead. | +| **Per-record exception swallowing in handler loops** | Would remove the retry→DLQ capture and weaken fail-closed behavior. If isolation is ever wanted: collect failures, re-raise at end. Documented in the runbook instead. | +| **Dedupe/skip logic inside handlers for replay safety** | Weakens the deliberate always-upsert semantics; replay safety handled in the reprocess script (targeted-by-default) and runbook. | +| **Generating EXTRACTION_PROMPT from derived_fields rule tables** | Defeats the shadow bake's purpose (comparing two independent implementations). Appropriate only after Python becomes authoritative and the prompt sections are deleted. | +| **Any derived_fields.py behavior change before the bake concludes** | Shadow-telemetry constraint: the module is under agreement measurement; restructuring (incl. the _classify_desc ladder refactor, Q9) confounds the telemetry. Docstring fix (X6) is text-only and allowed. | +| **Adding alarms to wo-web-ui as a side effect of the alarm helper** | New paging surface; propose separately and deliberately, never silently via refactor. | +| **Consolidating the fallback-rate alarms into one parameter-free helper** | PO's 6h/≥8-floor tuning is a deliberate, documented retune for ~57 emails/day vs WO's 15-min/≥10; helper parameterizes, per-stack values stay. | +| **fromTableName→CfnOutput coupling changes for slack-bot consumers** | Cross-repo; out of this refactor. CfnOutputs are added (C8) but consumer migration is seahaven-slack-bot's change. | +| **Full rewrite / repo split** | Nothing measured supports it: the tested tree is healthy (93%, fast suite, clean lint); every problem is addressable by consolidation with production continuity. | + +--- +*Cross-cutting compliance notes for execution: Phases 0, 1, 3 touch untrusted-input/auth surfaces → `/sh-security-review` mandatory before push. Phase 4 moves IAM policy construction and Phase 0 touches handler event contracts → cross-family `cross_review.py` review mandatory. Every PR: `ruff check` + `ruff format --check` locally before push; `gh pr checks` green before merge; deploy-then-merge for all deploy-affecting phases. README updates land in the same commits as the changes they describe; the Confluence "AWS Architecture Map" needs updating when Phase 4/7 alter the resource inventory (new outputs, runbook).* diff --git a/lambdas/po/email_processor/handler.py b/lambdas/po/email_processor/handler.py index 9f2e98d..ac4e897 100644 --- a/lambdas/po/email_processor/handler.py +++ b/lambdas/po/email_processor/handler.py @@ -633,6 +633,16 @@ def save_cancellation(parsed: dict): def handler(event, context): """Lambda entry point. Triggered by S3 ObjectCreated events.""" + # Direct-invoke healthcheck (post-deploy smoke). This MUST be the very first + # thing handler() does -- before any S3 fetch, before ses_auth, before the + # Records loop -- so it (a) creates no accept path for mail (real mail is an + # S3 ObjectCreated event whose top-level keys AWS controls; email content + # can never set a top-level "healthcheck" key), and (b) emits no EMF metric + # and no log line that could match the sender_auth_rejected metric-filter + # pattern, so two deploys in ~30 min never page that alarm. + if isinstance(event, dict) and event.get("healthcheck") is True: + return {"healthcheck": "ok"} + for record in event.get("Records", []): bucket = record["s3"]["bucket"]["name"] key = record["s3"]["object"]["key"] diff --git a/lambdas/po/email_processor/tests/test_po_healthcheck.py b/lambdas/po/email_processor/tests/test_po_healthcheck.py new file mode 100644 index 0000000..f56b91d --- /dev/null +++ b/lambdas/po/email_processor/tests/test_po_healthcheck.py @@ -0,0 +1,172 @@ +"""Direct-invoke healthcheck early-return tests for the PO handler. + +The post-deploy smoke script invokes the Lambda synchronously with +``{"healthcheck": true}`` and asserts the response is ``{"healthcheck": "ok"}``. +That branch is pinned to run as the VERY FIRST thing handler() does -- before +any S3 fetch, before ses_auth, before the Records loop -- so it neither creates +an accept path for mail nor emits any EMF metric / log line that could trip the +``sender_auth_rejected`` substring metric-filter alarm on a deploy. + +These tests import ``po_handler`` from ``_po_parser_support`` (moto imported +first, PO modules loaded by file path under unique names) exactly like the rest +of the PO suite, preserving the moto-before-handler import ordering. +""" + +import pytest + +from _po_parser_support import load_raw, po_handler + + +class _ExplodingS3: + """Any S3 access from the healthcheck path is a contract violation.""" + + def get_object(self, **kwargs): # noqa: N803 + raise AssertionError(f"healthcheck must not touch S3: {kwargs}") + + +def _exploding_auth(*args, **kwargs): + raise AssertionError("healthcheck must not call ses_auth") + + +def _exploding_metric(*args, **kwargs): + raise AssertionError("healthcheck must not emit any parse metric") + + +# --- (1) healthcheck early-return: ok payload, zero side effects --- + + +def test_healthcheck_returns_ok_with_zero_side_effects( + fake_dynamo, monkeypatch, capsys +): + monkeypatch.setattr(po_handler, "s3", _ExplodingS3()) + monkeypatch.setattr(po_handler, "authenticate_inbound_email", _exploding_auth) + monkeypatch.setattr(po_handler, "_emit_parse_method_metric", _exploding_metric) + monkeypatch.setattr(po_handler, "_emit_derived_agreement_metric", _exploding_metric) + + result = po_handler.handler({"healthcheck": True}, None) + + assert result == {"healthcheck": "ok"} + # ZERO DynamoDB writes: no table was ever fetched/updated. + assert fake_dynamo.tables == {} + # ZERO EMF metric emission: EMF records go to stdout via print(); the branch + # must not have printed anything the sender_auth_rejected filter could match. + # (Empty stdout subsumes any substring check on it; stderr is checked for + # the filter pattern specifically.) + captured = capsys.readouterr() + assert captured.out == "" + assert "sender_auth_rejected" not in captured.err + + +def test_healthcheck_branch_precedes_records_key_lookup(monkeypatch): + """The healthcheck return fires even when a hostile payload also carries a + top-level ``Records`` key: the branch is ordered before the loop, and the + exploding S3/auth prove the loop body never runs.""" + monkeypatch.setattr(po_handler, "s3", _ExplodingS3()) + monkeypatch.setattr(po_handler, "authenticate_inbound_email", _exploding_auth) + + event = { + "healthcheck": True, + "Records": [ + {"s3": {"bucket": {"name": "b"}, "object": {"key": "k"}}}, + ], + } + assert po_handler.handler(event, None) == {"healthcheck": "ok"} + + +@pytest.mark.parametrize( + "event", + [ + {"healthcheck": False}, + {"healthcheck": "true"}, + {"healthcheck": 1}, + {"healthcheck": {"nested": True}}, + {"Healthcheck": True}, # wrong case: not the contract key + {}, + ], +) +def test_non_healthcheck_payloads_do_not_early_return(event): + """Only a top-level ``healthcheck`` that is exactly ``True`` takes the + branch; anything else falls through to the (empty) Records loop and returns + the normal 200 body.""" + assert po_handler.handler(event, None) == {"statusCode": 200, "body": "OK"} + + +# --- (2) a normal S3 mail event is completely unaffected --- + + +class _RecordingS3: + def __init__(self, raw): + self._raw = raw + self.calls = [] + + def get_object(self, Bucket, Key): # noqa: N803 + self.calls.append((Bucket, Key)) + + class _Body: + def __init__(self, data): + self._data = data + + def read(self): + return self._data + + return {"Body": _Body(self._raw)} + + +def _s3_event(): + return { + "Records": [ + { + "s3": { + "bucket": {"name": "po-ingest-emails-x"}, + "object": {"key": "inbound/hc"}, + } + } + ] + } + + +def test_normal_mail_event_still_processed(fake_dynamo, monkeypatch): + """Golden-path guard: a real S3 ObjectCreated mail event is unaffected by + the healthcheck branch -- it still fetches from S3 and upserts the PO.""" + recording = _RecordingS3(load_raw("new-po", "new-po-01")) + monkeypatch.setattr(po_handler, "s3", recording) + monkeypatch.setattr(po_handler, "authenticate_inbound_email", lambda *a: True) + + result = po_handler.handler(_s3_event(), None) + + assert result == {"statusCode": 200, "body": "OK"} + # The branch was NOT taken: S3 was fetched and the PO was written through. + assert recording.calls == [("po-ingest-emails-x", "inbound/hc")] + assert fake_dynamo.tables[po_handler.PO_TABLE].updates + + +def test_healthcheck_string_in_email_body_does_not_take_branch( + fake_dynamo, monkeypatch +): + """A mail event whose EMAIL BODY contains the string 'healthcheck' must NOT + trigger the early return: the trigger is a top-level direct-invoke key that + AWS controls, and email content can never set a top-level event key. Proof: + S3 is still fetched (the branch would have skipped it).""" + raw_email = ( + b"From: buyer@amazon.coupahost.com\r\n" + b"To: po@seahavenind.com\r\n" + b"Subject: FYI healthcheck notes\r\n" + b"\r\n" + b"Please run a healthcheck on this order. healthcheck healthcheck.\r\n" + ) + recording = _RecordingS3(raw_email) + monkeypatch.setattr(po_handler, "s3", recording) + monkeypatch.setattr(po_handler, "authenticate_inbound_email", lambda *a: True) + # The synthetic body has no Coupa template match, so parsing would fall to + # the Bedrock extractor; stub it (returning no po_number) so the record is + # skipped cleanly. What matters here is only that S3 WAS fetched -- i.e. the + # healthcheck branch did not short-circuit on the body text. + monkeypatch.setattr(po_handler, "extract_with_claude", lambda *a: {}) + + result = po_handler.handler(_s3_event(), None) + + # Fell through to the Records loop (no early return): S3 WAS fetched. The + # synthetic body has no PO number, so it is skipped -- return is the normal + # 200 body, never the {"healthcheck": "ok"} smoke payload. + assert result == {"statusCode": 200, "body": "OK"} + assert recording.calls == [("po-ingest-emails-x", "inbound/hc")] diff --git a/lambdas/wo/email_processor/handler.py b/lambdas/wo/email_processor/handler.py index f94da64..4ae6773 100644 --- a/lambdas/wo/email_processor/handler.py +++ b/lambdas/wo/email_processor/handler.py @@ -349,6 +349,16 @@ def save_event( def handler(event, context): """Lambda entry point. Triggered by S3 ObjectCreated events.""" + # Deploy-guard healthcheck (Phase 0): a top-level direct-invoke + # {"healthcheck": true} probe returns immediately, BEFORE any S3 fetch, + # SES sender-auth gate, or Records iteration. Real mail arrives as S3 + # ObjectCreated events whose top-level keys ("Records") AWS controls, so + # email content can never set this key -- this creates no accept path for + # mail. It emits NO EMF and no log line matching the sender_auth_rejected + # metric-filter, so repeated post-deploy smoke invokes never page. + if isinstance(event, dict) and event.get("healthcheck") is True: + return {"healthcheck": "ok"} + for record in event.get("Records", []): bucket = record["s3"]["bucket"]["name"] key = record["s3"]["object"]["key"] diff --git a/lambdas/wo/email_processor/tests/test_healthcheck.py b/lambdas/wo/email_processor/tests/test_healthcheck.py new file mode 100644 index 0000000..352a328 --- /dev/null +++ b/lambdas/wo/email_processor/tests/test_healthcheck.py @@ -0,0 +1,69 @@ +"""Deploy-guard healthcheck branch (Phase 0). + +The handler must answer a top-level direct-invoke ``{"healthcheck": true}`` +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. +""" + +import handler +from _wo_parser_support import FakeDynamoResource + + +class _ExplodingS3: + """Any attribute access is a test failure: the healthcheck branch must + return before the handler ever reaches the S3 client.""" + + def get_object(self, *a, **k): # noqa: N803 + raise AssertionError("healthcheck branch touched S3") + + +def test_healthcheck_returns_ok(): + assert handler.handler({"healthcheck": True}, None) == {"healthcheck": "ok"} + + +def test_healthcheck_precedes_s3_and_auth(monkeypatch): + # If the early-return were missing or misplaced, the handler would call + # s3.get_object / authenticate_inbound_email; both are booby-trapped. + monkeypatch.setattr(handler, "s3", _ExplodingS3()) + monkeypatch.setattr(handler, "dynamodb", FakeDynamoResource()) + + def _boom_auth(*a, **k): + raise AssertionError("healthcheck branch reached SES auth") + + monkeypatch.setattr(handler, "authenticate_inbound_email", _boom_auth) + + assert handler.handler({"healthcheck": True}, None) == {"healthcheck": "ok"} + + +def test_healthcheck_emits_no_metric(monkeypatch): + # No EMF / ParseOutcome emission on the healthcheck path. + def _boom_metric(*a, **k): + raise AssertionError("healthcheck branch emitted a metric") + + monkeypatch.setattr(handler, "emit_parse_metric", _boom_metric) + + assert handler.handler({"healthcheck": True}, None) == {"healthcheck": "ok"} + + +def test_healthcheck_only_on_literal_true(): + # A truthy-but-not-True value must NOT trigger the branch: it falls through + # to the (empty) Records loop and returns the normal 200 envelope. This + # keeps the trigger to the exact direct-invoke contract. + assert handler.handler({"healthcheck": "yes"}, None) == { + "statusCode": 200, + "body": "OK", + } + + +def test_records_event_ignores_healthcheck_lookalike(): + # A real S3 event shape has no top-level healthcheck key; an empty Records + # list is processed normally without hitting the early return. + assert handler.handler({"Records": []}, None) == { + "statusCode": 200, + "body": "OK", + } diff --git a/scripts/post-deploy-smoke.sh b/scripts/post-deploy-smoke.sh new file mode 100755 index 0000000..9daffc2 --- /dev/null +++ b/scripts/post-deploy-smoke.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# Synchronous post-deploy smoke test for the PO and WO email-processor Lambdas. +# +# Invokes both functions with a {"healthcheck": true} payload using +# RequestResponse (synchronous) invocation and asserts: +# 1. The invoke response has no FunctionError field. A Lambda init failure +# (e.g. an ImportError from a broken bundle) still returns HTTP 200 from +# the Invoke API with FunctionError=Unhandled, so an exit-code-only check +# would false-pass -- this script inspects the response JSON explicitly. +# 2. The returned payload is exactly {"healthcheck": "ok"}. +# +# Exits non-zero on any failure (bad invoke, FunctionError set, wrong payload). + +set -euo pipefail + +REGION="us-east-1" +FUNCTION_NAMES=("po-email-processor" "workorder-email-processor") +HEALTHCHECK_PAYLOAD='{"healthcheck": true}' + +work_dir="$(mktemp -d)" +trap 'rm -rf "${work_dir}"' EXIT + +overall_status=0 + +for function_name in "${FUNCTION_NAMES[@]}"; do + echo "==> Smoke-testing ${function_name} (region ${REGION})" + + response_meta_file="${work_dir}/${function_name}.meta.json" + response_payload_file="${work_dir}/${function_name}.payload.json" + + if ! aws lambda invoke \ + --function-name "${function_name}" \ + --invocation-type RequestResponse \ + --payload "${HEALTHCHECK_PAYLOAD}" \ + --cli-binary-format raw-in-base64-out \ + --region "${REGION}" \ + "${response_payload_file}" \ + >"${response_meta_file}"; then + echo "FAIL: ${function_name}: aws lambda invoke command failed" >&2 + overall_status=1 + continue + fi + + function_error="$(python3 -c ' +import json, sys +with open(sys.argv[1]) as f: + meta = json.load(f) +print(meta.get("FunctionError", "")) +' "${response_meta_file}")" + + if [[ -n "${function_error}" ]]; then + echo "FAIL: ${function_name}: invoke returned FunctionError=${function_error}" >&2 + echo "----- response payload -----" >&2 + cat "${response_payload_file}" >&2 + echo >&2 + overall_status=1 + continue + fi + + payload_check="$(python3 -c ' +import json, sys +try: + with open(sys.argv[1]) as f: + payload = json.load(f) +except (json.JSONDecodeError, UnicodeDecodeError): + print("unparseable") + sys.exit(0) +print("ok" if payload == {"healthcheck": "ok"} else "mismatch") +' "${response_payload_file}")" + + if [[ "${payload_check}" == "unparseable" ]]; then + echo "FAIL: ${function_name}: response payload is not valid JSON:" >&2 + cat "${response_payload_file}" >&2 + echo >&2 + overall_status=1 + continue + fi + + if [[ "${payload_check}" != "ok" ]]; then + echo "FAIL: ${function_name}: unexpected healthcheck payload:" >&2 + cat "${response_payload_file}" >&2 + echo >&2 + overall_status=1 + continue + fi + + echo "OK: ${function_name} healthcheck passed" +done + +if [[ "${overall_status}" -ne 0 ]]; then + echo "post-deploy smoke: one or more functions failed the healthcheck" >&2 + exit 1 +fi + +echo "post-deploy smoke: all functions healthy" diff --git a/tests/test_bundle_consistency.py b/tests/test_bundle_consistency.py new file mode 100644 index 0000000..0f0c3fa --- /dev/null +++ b/tests/test_bundle_consistency.py @@ -0,0 +1,250 @@ +"""AST-based bundle-consistency test for the email-processor Lambdas. + +Verifies that each pipeline's CDK bundling `command` actually ships every +first-party sibling module handler.py imports into /asset-output. This +guards against a regression where the bundling `cp` step (whether an +explicit filename allowlist or a glob) silently drops a module the handler +depends on -- history: PR #105 shipped without template_parser.py, and PR #2 +nearly shipped without derived_fields.py, both allowlist-maintenance misses +that would ImportError at runtime. + +Pure ast + file reads -- no AWS/boto3/CDK synth, no handler import, no moto. +Fast and has no dependency on the moto-before-handler import-order invariant +that the rest of the suite relies on. +""" + +import ast +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] + +PO_HANDLER = REPO_ROOT / "lambdas" / "po" / "email_processor" / "handler.py" +WO_HANDLER = REPO_ROOT / "lambdas" / "wo" / "email_processor" / "handler.py" +PO_STACK = REPO_ROOT / "cdk" / "po_stack.py" +WO_STACK = REPO_ROOT / "cdk" / "wo_stack.py" + +# The complete set of top-level modules the PO email_processor ships via the +# non-recursive `cp ./*.py` glob. Pinned as an upper bound: a new top-level +# .py in that dir must be added here deliberately, which is the moment to +# decide whether it SHOULD ship (a real module) or must be excluded (a scratch +# or secrets file that from_asset would otherwise stage into the bundle on a +# local deploy). See test_po_bundle_ships_no_unexpected_top_level_modules. +PO_EXPECTED_TOP_LEVEL_MODULES = frozenset( + {"handler", "ses_auth", "template_parser", "derived_fields"} +) + + +def _first_party_sibling_imports(handler_path: Path) -> set[str]: + """Top-level module names handler.py imports that are first-party siblings. + + Parses only top-level (module-body) `import X` / `from X import ...` + statements -- not imports nested in functions -- and keeps a name only + if `/X.py` exists, which filters out stdlib/third-party + imports (json, os, boto3, ...) and keeps exactly the modules the + bundling step is obligated to ship. + """ + tree = ast.parse(handler_path.read_text()) + names: set[str] = set() + for node in tree.body: + if isinstance(node, ast.Import): + for alias in node.names: + names.add(alias.name.split(".")[0]) + elif isinstance(node, ast.ImportFrom): + if node.module: + names.add(node.module.split(".")[0]) + sibling_dir = handler_path.parent + return {name for name in names if (sibling_dir / f"{name}.py").exists()} + + +def _extract_bundling_command(stack_path: Path) -> str: + """Extract the bash -c bundling command string from a CDK stack file. + + Locates the `command=[...]` keyword argument to BundlingOptions and + returns its final list element's string value. Adjacent string-literal + concatenation (used in both stacks to keep the command readable across + lines, with `#` comments interspersed) is folded by the parser itself, + so this returns the exact single command string CDK will hand to bash. + + Each stack currently has exactly one bundled function; if a second one + is ever added, "the" bundling command becomes ambiguous and every + assertion in this file needs to pick its target explicitly — so demand + exactly one match rather than silently returning whichever ast.walk + happens to visit first. + """ + tree = ast.parse(stack_path.read_text()) + commands: list[str] = [] + for node in ast.walk(tree): + if isinstance(node, ast.keyword) and node.arg == "command": + list_node = node.value + if isinstance(list_node, ast.List) and list_node.elts: + last = list_node.elts[-1] + if isinstance(last, ast.Constant) and isinstance(last.value, str): + commands.append(last.value) + if len(commands) != 1: + raise AssertionError( + f"expected exactly one bundling command=[...] list in {stack_path}, " + f"found {len(commands)} — update this test to select the intended " + "bundling command explicitly" + ) + return commands[0] + + +def _executed_cp_commands(command: str) -> list[str]: + """The `cp ...` invocations bash would actually execute, comment-stripped. + + Splits the bundling command on shell separators (`&&`, `;`, `|`, newline), + drops any trailing `#` comment from each segment, and returns the segments + whose command word is `cp`. This is deliberately stricter than a substring + match: a glob or `cp -r` string that survives only inside a comment + (e.g. `cp handler.py /asset-output/ # was: cp ./*.py /asset-output/`) is + NOT returned, because bash would not execute it -- so a revert that strips + the real copy but leaves the old text commented cannot false-pass. + """ + segments = re.split(r"&&|\|\||;|\||\n", command) + cp_cmds: list[str] = [] + for seg in segments: + seg = seg.split("#", 1)[0].strip() + if re.match(r"cp\b", seg): + cp_cmds.append(seg) + return cp_cmds + + +def _bundling_ships_all(command: str, sibling_names: set[str]) -> bool: + """True if `command` is guaranteed to ship every name in `sibling_names`. + + Inspects only the `cp` commands bash actually executes (comments stripped + via `_executed_cp_commands`). An executed copy ships every top-level .py + sibling unconditionally in two shapes: + - a non-recursive glob copy, e.g. `cp ./*.py /asset-output/` (PO); + - a recursive copy of the WHOLE source dir, e.g. `cp -r . /asset-output/` + (WO) -- the source operand must be `.`/`./`, so a narrowed recursive + copy like `cp -r ./package /asset-output/` does NOT qualify. + Any other executed `cp` is treated as an explicit filename allowlist (the + legacy PO form) and is safe only if every required `.py` literally + appears among the copied filenames -- the branch that must reject a + reverted allowlist missing a sibling. + """ + cp_cmds = _executed_cp_commands(command) + if not cp_cmds: + return False + copied_files: set[str] = set() + for cp in cp_cmds: + if re.search(r"(?:^|\s)\.?/?\*\.py(?:\s|$)", cp): + return True + if re.search(r"cp\s+-r\s+\.\/?\s+/asset-output", cp): + return True + # Explicit-allowlist shape: collect the copied source filenames, + # ignoring any cp flags and the trailing /asset-output destination. + match = re.search( + r"cp\s+(?:-\S+\s+)*(.*?)\s*/asset-output/?\"?\s*$", cp.strip() + ) + if match: + copied_files.update(match.group(1).split()) + return all(f"{name}.py" in copied_files for name in sibling_names) + + +def test_po_bundling_ships_all_first_party_siblings(): + siblings = _first_party_sibling_imports(PO_HANDLER) + # Sanity: PO handler.py is known to import ses_auth, template_parser, + # and derived_fields as bare-name siblings. If this ever collapses to + # an empty set, the test below would vacuously pass -- guard against that. + assert siblings, "expected first-party sibling imports in PO handler.py" + + command = _extract_bundling_command(PO_STACK) + + # Pinned per the Phase 0 healthcheck/bundling contract: PO's bundling + # command must EXECUTE the non-recursive glob, not a hand-maintained + # allowlist. Checked against the executed cp (comments stripped) so the + # old glob text surviving only in a comment cannot satisfy this. + executed_cps = _executed_cp_commands(command) + assert any(re.search(r"(?:^|\s)\.?/?\*\.py(?:\s|$)", cp) for cp in executed_cps), ( + "cdk/po_stack.py bundling command must EXECUTE the non-recursive glob " + "'cp ./*.py /asset-output/' so every first-party sibling handler.py " + f"imports ships automatically. Executed cp commands: {executed_cps}" + ) + assert _bundling_ships_all(command, siblings), ( + f"cdk/po_stack.py bundling command does not ship all of {sorted(siblings)}: " + f"{command!r}" + ) + + +def test_wo_bundling_ships_all_first_party_siblings(): + siblings = _first_party_sibling_imports(WO_HANDLER) + assert siblings, "expected first-party sibling imports in WO handler.py" + + command = _extract_bundling_command(WO_STACK) + + # WO is out of scope for the Phase 0 glob change -- it ships everything + # via a recursive `cp -r` of the whole source dir, which is a different + # (broader, not narrower) mechanism that also guarantees every sibling + # ships. Assert the executed cp recursively copies the WHOLE dir (source + # `.`/`./`), so a narrowed `cp -r ./package /asset-output/` does not pass. + executed_cps = _executed_cp_commands(command) + assert any( + re.search(r"cp\s+-r\s+\.\/?\s+/asset-output", cp) for cp in executed_cps + ), ( + "cdk/wo_stack.py bundling command is expected to recursively copy the " + f"whole source dir so every first-party sibling ships. Executed cp " + f"commands: {executed_cps}" + ) + assert _bundling_ships_all(command, siblings), ( + f"cdk/wo_stack.py bundling command does not ship all of {sorted(siblings)}: " + f"{command!r}" + ) + + +def test_po_bundle_ships_no_unexpected_top_level_modules(): + """Upper bound: the PO glob ships EXACTLY the expected top-level modules. + + The sibling-import tests above prove the glob ships every module the + handler needs (shipped >= required). This proves the other direction + (shipped <= expected): because `cp ./*.py` copies every top-level .py in + the dir -- and `Code.from_asset` stages untracked files too (it does not + honor .gitignore) -- a stray scratch/secrets .py left in this dir would + silently ship into the production zip on a local deploy. Pinning the set + forces any new top-level module to be added to + PO_EXPECTED_TOP_LEVEL_MODULES deliberately, at which point the author + decides whether it should ship or be excluded from bundling. + """ + top_level = {p.stem for p in PO_HANDLER.parent.glob("*.py")} + assert top_level == set(PO_EXPECTED_TOP_LEVEL_MODULES), ( + "unexpected top-level .py set in lambdas/po/email_processor -- the " + "'cp ./*.py' glob would ship exactly these into the Lambda zip. " + f"Found {sorted(top_level)}, expected " + f"{sorted(PO_EXPECTED_TOP_LEVEL_MODULES)}. If a new module is " + "intended, add it to PO_EXPECTED_TOP_LEVEL_MODULES; if it is a scratch " + "or secrets file, remove it (or exclude it from bundling) before deploy." + ) + + +def test_detection_logic_catches_allowlist_missing_a_sibling(): + """Unit-level check on `_bundling_ships_all` itself. + + Simulates the historical regression shape directly: someone reverts the + PO glob back to an explicit filename allowlist that omits + derived_fields.py (the near-miss from PR #2). `_bundling_ships_all` must + detect the gap so that, combined with the "cp ./*.py" pin above, + test_po_bundling_ships_all_first_party_siblings fails loudly on any such + revert rather than silently passing. + """ + siblings = _first_party_sibling_imports(PO_HANDLER) + assert "derived_fields" in siblings # sanity: this is the PR #2 near-miss module + + reverted_allowlist_command = ( + "pip install --platform manylinux2014_aarch64 --only-binary=:all: " + "-r requirements.txt -t /asset-output && " + "cp handler.py ses_auth.py template_parser.py /asset-output/" + ) + + assert not _bundling_ships_all(reverted_allowlist_command, siblings) + + # Control: the same allowlist shape WITH derived_fields.py added back in + # is correctly recognized as complete, proving the failure above is + # about the missing file and not a false-positive-prone regex. + complete_allowlist_command = ( + "pip install --platform manylinux2014_aarch64 --only-binary=:all: " + "-r requirements.txt -t /asset-output && " + "cp handler.py ses_auth.py template_parser.py derived_fields.py /asset-output/" + ) + assert _bundling_ships_all(complete_allowlist_command, siblings)