mirror of
https://github.com/Sea-Haven-Industries/procurement-ingest.git
synced 2026-09-30 04:53:12 +00:00
feat: deploy-pipeline guards — healthcheck, smoke gate, bundle glob + AST test (refactor phase 0) (#107)
Some checks are pending
Deploy / deploy (push) Waiting to run
Some checks are pending
Deploy / deploy (push) Waiting to run
* 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.
This commit is contained in:
parent
8d52cfadef
commit
cb5539bd68
11 changed files with 1388 additions and 14 deletions
428
.claude/workflows/phase-0-deploy-guards.js
Normal file
428
.claude/workflows/phase-0-deploy-guards.js
Normal file
|
|
@ -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',
|
||||
],
|
||||
}
|
||||
1
.github/workflows/deploy.yaml
vendored
1
.github/workflows/deploy.yaml
vendored
|
|
@ -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 }}
|
||||
|
|
|
|||
42
README.md
42
README.md
|
|
@ -146,6 +146,30 @@ The `<fn>-duration` and `<fn>-throttles` alarms for `po-email-processor` and `wo
|
|||
|
||||
**DynamoDB alarms** (`AWS/DynamoDB`): each owned table gets `<table>-throttles` (`ThrottledRequests`) and `<table>-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 `<fn>-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 <token>`), 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
|
||||
```
|
||||
|
|
|
|||
|
|
@ -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/",
|
||||
],
|
||||
),
|
||||
),
|
||||
|
|
|
|||
305
docs/refactor-evaluation.md
Normal file
305
docs/refactor-evaluation.md
Normal file
|
|
@ -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 `<email>` data-block wrapping + `_EMAIL_TAG_RE` tag neutralization + `temperature=0` into `extract_with_claude`.
|
||||
- **Preserve the deliberate PO metric ordering:** PO emits `ai_fallback` BEFORE the Bedrock call (so Bedrock-side errors still record the outcome — handler.py:656-669). Do not move it. `ai_fallback_rejected` is an additive second datapoint on rejection; document the intentional double-count.
|
||||
- `cdk/po_stack.py` in the same PR: (a) fold `FILL(rej,0)` into the existing fallback-rate MathExpression numerator+denominator **inside** the IF volume floor (in-place property update to the existing alarm logical ID — safe; respect the post-#102 no-element-wise-MAX rule at po_stack.py:416-418); **exclude the rejected series from the rate numerator or account for the pre-call double-count** — do not copy WO's fb+rej math verbatim, it double-counts PO rejections. (b) net-new `EmailProcessorAiFallbackRejectedAlarm` — **retune for ~57 emails/day** (WO's 5-min/30-min sparse idiom is structurally dead at PO volume; use 1h–6h periods like the existing PO fallback-rate retune).
|
||||
- Tests (mirroring WO one-for-one): non-dict model output, injected po_number (`123#x`, fullwidth digits), injected email_type (assert no save_* AND no misroute into `save_new_po` via the else branch), injected status, XML-wrap, forged-tag neutralization, linear-time regex.
|
||||
- README: fix the false "Data is never corrupted" PO claim (line 29) and add `ai_fallback_rejected` to the PO ParseMethod list (line 145).
|
||||
- **Gates:** `/sh-security-review` (untrusted-input, mandatory), full suite, `npx cdk synth po-ingest`, deploy-then-merge with live-email verification of both metric series.
|
||||
|
||||
### Phase 2 — Bundling-root move, no code move (PR-1 of migration) · effort: S · risk: medium (deploy-mechanics only)
|
||||
**Goal:** make `lambdas/shared/` reachable before anything moves into it.
|
||||
- Both stacks: `Code.from_asset("../lambdas")` with `command: cp po/email_processor/*.py /asset-output/` (resp. wo) and `pip install -r po/email_processor/requirements.txt` (the `-r` path change is required — bundling cwd is the asset root). Add `exclude=['**/__pycache__/**','**/tests/**','**/package/**']` — `from_asset` does NOT honor .gitignore, and the stale 44 MB `package/` dir would otherwise diverge local vs CI asset hashes.
|
||||
- WO note: its current `cp -r .` ships tests/ (real scrubbed .eml fixtures), `__pycache__`, and requirements.txt in the prod zip. Accept and document the file-list shrinkage as deliberate cleanup; verification criterion for WO is "runtime-imported module set unchanged + smoke", byte-identical zip diff applies to PO only.
|
||||
- Also add `exclude` to the three plain `from_asset` calls (po web_ui :502, site_extractor :581, wo web_ui :548) — local `__pycache__` currently makes their hashes nondeterministic.
|
||||
- **Gates:** `aws lambda get-function` zip file-list diff (PO identical; WO shrinkage reviewed), smoke, one real email per pipeline. Zero handler diff.
|
||||
|
||||
### Phase 3 — `lambdas/shared/` extraction (PR-2) · effort: M · risk: medium
|
||||
**Goal:** one canonical copy per bare module name where copies are byte-identical or trivially superset-able.
|
||||
- Create `lambdas/shared/` (handbook `cdk-project-layout.md`). Move in dependency-risk order:
|
||||
1. **`ses_auth.py`** (byte-identical; the prime target — security-critical auth that currently requires every hardening fix to land twice). Add `cp shared/*.py /asset-output/` to both bundling commands; module lands flat so `from ses_auth import authenticate_inbound_email` is unchanged — zero handler diff keeps fail-closed auth byte-identical.
|
||||
2. **`web_ui_auth.py`**: the byte-identical block (`_get_auth_token`/`_header`/`is_authenticated` + the four cache globals). The web_ui functions have no bundling today — add the same staging mechanism. Keep the per-stack INFRA-74 comments in each handler (they've already drifted in wording; they're stack-specific).
|
||||
3. **`email_parsing.py`**: `parse_raw_email` superset returning `cc` unconditionally (PO ignores it — harmless).
|
||||
4. **`emf.py`**: generic emitter parameterized by namespace/dimension-sets/properties, serving all **three** hand-built `_aws` envelopes (both ParseMethod emitters + `_emit_derived_agreement_metric`). Dimension-set list `[["ParseMethod"],["ParseMethod","TemplateId"]]` is load-bearing for the alarms; a shared function makes one-sided dimension fixes impossible.
|
||||
- **Do NOT move:** `template_parser.py` (990 vs 508 lines, genuinely divergent — stays per-pipeline in `_SIBLING_MODULES`), the Bedrock extraction functions (divergent contracts; share only the invocation/prompt-delimiting layer, parameterizing `json.loads` kwargs, `max_tokens` 2048/1024, prompt), per-pipeline `validate_ai_fallback` gates.
|
||||
- Test plumbing in the same PR: update `_SIBLING_MODULES` resolution, `_po_parser_support.py:71`, the fixture-hygiene test, drop the `ses_auth` fixture params (halves the 448-line test_ses_auth run). The sys.modules save/restore dance **survives for template_parser** — it does not shrink to nothing.
|
||||
- **Gates:** full suite, smoke, deploy-then-merge watching both sender-auth-rejected alarms through live mail. The post-merge CI redeploy being a no-op (unchanged asset hash) is itself a verification signal. Auth code moved → `/sh-security-review`.
|
||||
|
||||
### Phase 4 — `cdk/common.py` dedup · effort: M · risk: medium (logical-ID discipline)
|
||||
**Goal:** collapse the 379 identical CDK lines.
|
||||
- Extract as **plain functions taking `(scope, id, ...)` called with the SAME scope (the Stack) and SAME construct ids** — 100% logical-ID-safe. **Do NOT wrap in Construct subclasses**: that inserts a tree node, changes every child logical ID, and would attempt replacement of the RETAIN-protected `purchase-orders`/`WorkOrders` tables and named buckets.
|
||||
- Contents: `_DDB_ALARM_OPERATIONS` + `add_ddb_alarms`, `add_sender_auth_rejected_alarm`, `add_standard_lambda_alarms(scope, id_prefix, fn, name_prefix, topic, *, duration_statistic, errors=True, dlq=None, descriptions=...)` (variance to preserve: PO p99 vs wo-email-processor p95, po-web-ui throttles+duration only, site_extractor no-DLQ, **wo web_ui has zero alarms — do not silently add any**; bespoke description strings passed verbatim), `make_bedrock_invoke_statement` (derive the inference-profile ARN from `Stack.account`/`Stack.region` instead of hardcoding 328440206208), `make_email_bucket`, `make_processor_dlq`, `make_fallback_rate_alarm(namespace, rejected_included, period, threshold, floor, evaluation_periods, datapoints_to_alarm)` reproducing expression strings/FILL/labels byte-for-byte. WO's rejected alarm stays a WO-only call (until Phase 1's PO twin).
|
||||
- Do not import stack-specific services (kms/ssm/event_sources) into common.py.
|
||||
- **Gates:** `cdk diff` on BOTH stacks showing **zero changes** (the acceptance test); moving IAM PolicyStatement construction → mandatory **cross-family review** (`cross_review.py`) even though semantics are identical.
|
||||
- Same PR: add `account='328440206208'` to both `cdk.Environment` calls, pin `constructs==` exact, fix the stale "2.259.0" comments, add CfnOutputs for the five function ARNs + consumed table names (net-new — ID-safe).
|
||||
|
||||
### Phase 5 — Handler decomposition + lazy boto3 clients · effort: M · risk: medium
|
||||
**Goal:** break the God-modules along the seams that already work (ses_auth/template_parser/derive_all prove the flat-sibling pattern).
|
||||
- PO: `handler.py` (event loop + auth + routing) / `extraction.py` (parse_raw_email shim, extract_with_claude, prompt import) / `enrichment.py` (enrich_parsed, pad_zip — PO-only, WO has no enrichment stage) / `telemetry.py` / `persistence.py` (`_write_fields`/`_merge_update`/save_*; collapse the byte-identical `save_new_po`/`save_revision` into one `_save_merge`). WO: ~5 concerns, not 7 — its split must keep `validate_ai_fallback` + the `[0-9]+` work_order_id key guard in the handler loop ahead of both saves, and keep `_header_date_iso`/comment_id determinism with persistence.
|
||||
- Move `EXTRACTION_PROMPT` (181/693 lines of po handler) to `prompts.py` with cross-reference headers to derived_fields; keep a re-export since 4 tests dereference `handler.EXTRACTION_PROMPT`.
|
||||
- Lazy cached boto3 accessors in I/O modules; pure modules import no boto3. Update the `monkeypatch.setattr(handler, "dynamodb", fake)` patch surface in the same change. Preserve the **moto-before-handler import ordering** (`_po_parser_support.py:26-33`) or moto-backed suites hit real AWS.
|
||||
- **Behavior-preservation obligations stated per pipeline:** PO metric before Bedrock call; WO metric after gate with mutually-exclusive `ai_fallback`/`ai_fallback_rejected`; shadow telemetry ai_fallback-only. Bundling: the glob from Phase 0 already ships new siblings automatically; the ast test verifies.
|
||||
- **Gates:** full suite (goldens unchanged), smoke, one live email per pipeline. Handler decomposition keeps signatures — no cross-family review needed unless the event/return contract changes.
|
||||
|
||||
### Phase 6 — site_extractor reconciliation · effort: M · risk: medium · **timing: after the shadow bake concludes**
|
||||
**Goal:** end the three-way site_code contradiction (KLAL-class all-letter codes currently can NEVER self-register — permanent pending-review rows).
|
||||
- `extract_site_code` already prefers `record["site_code"]` (line 36) — the defect is the digit-requiring `SITE_CODE_PATTERN.match` (prefix-anchored, rejects KLAL, accepts overlong junk like `DLI6X`). Fix: validate the direct field with the canonical `derived_fields` shape + skip-list semantics (fullmatch), import `derive_site_code` for the fallback path (ship `derived_fields.py` into the site_extractor asset via the Phase 2/3 mechanism), delete the bespoke regex ladder (line 62's `[A-Z]{4,5}` alternation is dead code). Keep shape/skip validation on the ai_fallback-sourced field rather than blind trust (LLM value is authoritative during the bake). Keep parse_address + pending-review flow as-is.
|
||||
- Repoint `scripts/backfill_sites.py` at `derive_site_code` (or delete it — one-time script).
|
||||
- Characterization tests FIRST (pin current behavior, including the KLAL divergence, before changing it) — see §4.
|
||||
- **Gates:** new site_extractor test suite, deploy, watch pending-site-review write rate drop.
|
||||
|
||||
### Phase 7 — Ops/recovery + dependency hygiene · effort: S · risk: low (can run parallel to 4–6)
|
||||
- Generalize `scripts/reprocess.py`: `--pipeline po|wo`, `--key/--prefix/--since`; targeted replay primary, full-prefix demoted behind `--all` with documented caveats (Event-type invocation is concurrent → sorting doesn't serialize; switch to RequestResponse if order matters; metrics double-counted; Bedrock re-billed; out-of-order replay regresses merged fields).
|
||||
- `docs/runbook-dlq-recovery.md`: async-destination DLQ has **no redrive-to-source**; procedure = receive-message → key from event body → targeted re-invoke → verify → purge. State the windows: 14 d DLQ breadcrumb, 90 d raw-email S3 (lifecycle overrides RETAIN). Document that sender-auth and ai_fallback_rejected drops intentionally never reach the DLQ. Link from README alarms section.
|
||||
- Dependency hygiene: drop vendored boto3 (both email-processor requirements → handbook empty-with-comment form; only-boto3 functions correctly use the runtime copy per `lambda-template.md`); simplify bundling to cp-only; exact-pin `moto==` and add `/tests`, `/lambdas/po/web_ui`, `/lambdas/po/site_extractor` dependabot entries (pin first — floor pins make dependabot entries no-ops); reduce wo/web_ui's dead manifest to empty-with-comment.
|
||||
- Delete the 44 MB `package/` dir (one benign asset-hash redeploy; do it before/with Phase 2's excludes).
|
||||
|
||||
### Phase 8 — Test-root consolidation (last PR — validates the new boundaries) · effort: M · risk: low
|
||||
See §4. Also the small correctness/doc items batched here: WO `invalid_status` reason-code fix (+ grep dashboards for `malformed_site_code` first), WO Bedrock-error metric fix (wrap the call: emit `ai_fallback`/`bedrock_error` in an except-and-reraise — NOT a naive reorder, which double-counts against wo_stack's "rejected emits nothing else" alarm contract), `_validate_new_po_values` per-rule split (V4 anchor-frame dataclass must carry `summary_matches`/`price` for V13; add a ruff config enabling PLR/C901 or the noqa-drop is meaningless; include `extract_new_po` C901=35), docstring/README drift batch (findings 33–38 appendix refs).
|
||||
|
||||
---
|
||||
|
||||
## 4. Test-hardening plan
|
||||
|
||||
### Target layout — resolving the two-roots question
|
||||
**Keep both roots; fix the loading.** The split itself is principled (per-lambda golden suites beside the code; cross-pipeline suites at top). Changes:
|
||||
- **One loader.** New **repo-root `conftest.py`** (required anyway for package mapping; `tests/conftest.py` does not load for standalone `pytest lambdas/po` runs, so it cannot carry session invariants): dummy AWS env, **moto BUILTIN_HANDLERS registration before any handler import** (with the explanatory comment currently buried in `_po_parser_support.py:33`), and a single `load_lambda_module(pipeline, name)` keeping one copy of the sys.modules save/restore (still needed for template_parser's duplicated bare name).
|
||||
- Rewrite `_wo_parser_support.py` off the bare-import strategy (it is the source of the collision the other two loaders defend against). Shared `tests/support/` package: superset `FakeTable` (update_item + WO's put_item/keyed store), `FakeDynamoResource`, `load_email`, `load_golden` **keeping PO's `parse_float=Decimal`** (load-bearing for exact money comparison).
|
||||
- File moves: `test_po_merge.py`, `test_pad_zip.py` → `lambdas/po/email_processor/tests/`. `test_parse_raw_email.py` and `test_ses_auth.py` **stay at root** (genuinely cross-pipeline, parameterized over both handlers).
|
||||
- Delete `test_local.py` (globs a nonexistent `samples/`, WO-only, bypasses the gate; the golden suites cover its role) or rewrite with `--pipeline` — deletion preferred.
|
||||
|
||||
### Missing scenarios by module (priority order)
|
||||
1. **PO AI-fallback negatives** (with Phase 1): non-dict output (list/str/int/None — today AttributeErrors at the logger f-string into retries/DLQ), injected po_number/email_type/status, XML-wrap + neutralization + linear-time regex, markdown-fence stripping (currently untested in PO).
|
||||
2. **Bedrock transport errors, both pipelines:** ThrottlingException (assert PO's pre-call metric survived + no partial write + exception propagates into the errors-alarm/DLQ path — this doubles as the only real proof of PO's metric-before-call ordering), missing `content` key, empty content list, non-JSON model text. **Pin WO's no-datapoint-on-throttle behavior with a documenting test** (do not "fix" by reordering — see Phase 8 note).
|
||||
3. **Handler-level SES auth seam:** today every dispatch test monkeypatches `authenticate_inbound_email=True`; deleting the gate line would pass all 568 tests. Add per pipeline: reject-path with no auth monkeypatch + empty `ALLOWED_DKIM_DOMAINS` (assert zero Bedrock calls, zero writes, no raise; env is read at call time so setenv suffices; FakeS3 still needed — the gate sits after get_object). Accept-path is turnkey for PO (scrubbed Coupa fixtures authenticate); WO needs an SES-stamped fixture synthesized from `WO_SES_HEADER` in test_ses_auth.
|
||||
4. **web_ui (both, 0% today — auth is the mandatory-review surface):** fail-closed on unset ARN (requires module reload — ARN read at import), fail-closed on Secrets Manager exception, TTL cache refresh (reload fixture resets globals), Bearer/X-Auth-Token/case-insensitivity, wrong-token 401, **401 without table scan**, non-ASCII token (hmac.compare_digest TypeError → 500 today, worth pinning/fixing), hostile-field escaping (regression lock — escaping is currently correct on inspection). PO web_ui lacks `__init__.py` — use the loader, not package imports.
|
||||
5. **WO merge semantics:** `tests/test_wo_merge.py` moto-backed mirror of test_po_merge (table name `WorkOrders`, not kebab): null-status never clobbers wo_status, created_at immutable via if_not_exists, status→wo_status mapping, None fields absent from SET, record_type only-when-present.
|
||||
6. **site_extractor characterization (before Phase 6):** extract_site_code shapes incl. KLAL divergence and prefix-match overlong acceptance (`SNY55`), parse_address variants, stream routing (INSERT/MODIFY/REMOVE, pending-review fallback), upsert_site SS-ADD expression.
|
||||
7. **Small pins:** PO-DC-02 64-char EMF clamp regression test (~10 lines in test_po_derived_wiring); multi-record event failure-isolation test per pipeline (documents the all-or-retry contract); reprocess.py synthetic-event-shape contract test (also pins "raw key, no URL-decoding").
|
||||
|
||||
### Fixtures/goldens
|
||||
Golden .eml + expected-JSON suites are the model — extend, don't replace. Shared `tests/support/` loaders; per-pipeline fixtures stay per-pipeline (real scrubbed samples). WO gains one SES-stamped fixture (synthesized header block, not new scraped mail).
|
||||
|
||||
### CI changes
|
||||
- Add `--cov` with an explicit module list (or add `__init__.py` so `--cov=lambdas` stops silently skipping web_ui/site_extractor); fail-under once web_ui/site_extractor suites exist.
|
||||
- Add the Phase 0 ast bundle-consistency test to the standard run.
|
||||
- Add ruff config enabling C901/PLR so complexity ceilings are enforced, not decorative.
|
||||
- Lint scope currently omits `scripts/` and `test_local.py` — include scripts/ (or delete test_local.py and moot half of it).
|
||||
- Keep the strict `ci / ci` required check; **no admin-bypass pushes to main while migration PRs are in flight** (bypass_mode=always currently allows a zero-CI production deploy).
|
||||
|
||||
---
|
||||
|
||||
## 5. Findings appendix
|
||||
|
||||
55 confirmed (survived the verification pass, several with sharpening corrections noted in §3/§4), 16 **unverified** (plausible on the evidence given but not independently re-verified — treat as candidates, not commitments). No finding was refuted outright; verification corrections were incorporated into the plan above.
|
||||
|
||||
### Duplication / drifted copies
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| D1 | AI-fallback hardening (#104) landed in WO only; PO fallback already drifted behind it | confirmed | high |
|
||||
| D2 | ses_auth.py byte-identical 452-line copy, sync policed by test fixture | confirmed | high |
|
||||
| D3 | web_ui fail-closed auth gate byte-identical ~61-line block in both handlers | confirmed | high |
|
||||
| D4 | No lambdas/shared/ despite 4 duplicated modules (handbook location unused; "prescribes" softened to "provides, and the need condition is met") | confirmed | high |
|
||||
| D5 | EMF ParseMethod emitter near-identical copy, already drifting (actually 3×, incl. derived-agreement emitter) | confirmed | medium |
|
||||
| D6 | parse_raw_email drifted copy (WO Cc delta is intentional; poor standalone ROI — fold into D2's plumbing) | confirmed | medium |
|
||||
| D7 | Bedrock-fallback test suites drifted mirroring source drift (WO has 8 tests PO lacks, not 4) | confirmed | medium |
|
||||
| D8 | template_parser pair: extract only common substrate (_plain_lines etc.), do NOT merge parsers | **unverified** | low |
|
||||
| D9 | save_new_po / save_revision byte-identical bodies | **unverified** | low |
|
||||
| D10 | Three hand-rolled DynamoDB SET-expression builders | **unverified** | low |
|
||||
| D11 | test_local.py + backfill_sites.py entrench single-pipeline layout | **unverified** | low |
|
||||
|
||||
### Architecture
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| A1 | PO ai_fallback path has no validation-gate module (structural parity gap; unvalidated po_number becomes partition key; sticky-Cancel blast radius) | confirmed | high |
|
||||
| A2 | Both email-processor handlers are God-modules (WO ~5 concerns, not 7) | confirmed | high |
|
||||
| A3 | site_extractor is a third, contradictory site-code classifier (all-letter codes can never self-register) | confirmed | high |
|
||||
| A4 | Module-import boto3 clients force the bespoke loader tax (paid 3×) | confirmed | medium |
|
||||
| A5 | Trade/site/fiscal rules in two authoritative texts (prompt + derived_fields); keep independent during bake | confirmed | medium |
|
||||
| A6 | Two test roots, three loading idioms, load-bearing moto import order | confirmed | medium |
|
||||
| A7 | web_ui interleaves auth/data/HTML rendering | **unverified** | low |
|
||||
|
||||
### Code quality / consistency
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| Q1 | PO fallback lacks fail-closed gate + injection hardening (consistency view of D1/A1; write path uses non-null-key spray vs WO's whitelist) | confirmed | high |
|
||||
| Q2 | site_extractor superseded by derived_fields (skip-list sub-claim corrected: real divergence is prefix-match false-accepts + dead all-letter alternation) | confirmed | medium |
|
||||
| Q3 | _validate_new_po_values 294-line monolith, 60 returns; noqa suppressions currently inert (no ruff config); extract_new_po C901=35 is the worse peer | confirmed | medium |
|
||||
| Q4 | WO gate misleading reason codes ("malformed_site_code" for bad status — and validate_ai_fallback already returns "invalid_status" for the same failure, so one failure → two codes by path) | confirmed | medium |
|
||||
| Q5 | Stale 44 MB package/ vendored tree (real cost: synth staging + docker mount + asset-hash divergence, not sys.path shadowing) | confirmed | medium |
|
||||
| Q6 | web_ui full-table scan + in-memory sort + magic 500 (page-cap option rejected — scan order isn't recency; GSI or accept arbitrary order) | confirmed | medium |
|
||||
| Q7 | test_local.py rotted (missing samples/, WO-only, bypasses gate) | **unverified** | low |
|
||||
| Q8 | Type hints absent/degenerate on newest modules | **unverified** | low |
|
||||
| Q9 | _classify_desc C901=21 priority ladder — defer past bake | **unverified** | low |
|
||||
| Q10 | WO template_parser function-local imports / sentinel-after-use | **unverified** | low |
|
||||
| Q11 | backfill_sites.py sys.path hack keeps stale extractor alive | **unverified** | low |
|
||||
|
||||
### Tests
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| T1 | PO ai_fallback: zero negative tests for hostile model output | confirmed | high |
|
||||
| T2 | Bedrock transport errors untested both pipelines; metric-ordering divergence unpinned (WO reorder would break its alarm contract — pin, don't move) | confirmed | high |
|
||||
| T3 | Handler-level SES auth wiring never tested (every dispatch test bypasses) | confirmed | high |
|
||||
| T4 | web_ui completely untested incl. auth gate (escaping itself verified correct — tests are regression locks) | confirmed | high |
|
||||
| T5 | Three divergent test-bootstrap loaders + byte-identical fake-Dynamo helpers (only FakeDynamoResource/_stems identical; FakeTable/load_golden intentionally divergent — superset needed) | confirmed | medium |
|
||||
| T6 | WO save_work_order merge semantics no direct tests | confirmed | medium |
|
||||
| T7 | Loader choreography: moto import-order invariant enforced only by comments (not breakable by reordering today; maintenance landmine) | confirmed | medium |
|
||||
| T8 | PO-DC-02 64-char clamp no regression test | **unverified** | low |
|
||||
| T9 | Multi-record S3 event batch-abort semantics untested | **unverified** | low |
|
||||
| T10 | test_rule7_bad_status locks in the wrong reason code | **unverified** | low |
|
||||
| T11 | scripts/ untested and exclusion undocumented | **unverified** | low |
|
||||
|
||||
### CDK / infrastructure
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| C1 | ~400 (measured 379) identical lines between stacks; extract plain-function cdk/common.py; Construct-wrapping forbidden (logical-ID replacement of RETAIN tables) | confirmed | high |
|
||||
| C2 | PO fallback-rate alarm omits rejected series; no PO rejected alarm (retune for 57/day — WO's sparse idiom structurally dead at PO volume) | confirmed | high |
|
||||
| C3 | Alarm helpers duplicated verbatim; per-function alarm boilerplate 13+9 sites (variance to preserve: p99/p95, per-function alarm subsets, wo web_ui has none) | confirmed | medium |
|
||||
| C4 | Duration statistic drift p99 vs p95, no rationale | **unverified** | low |
|
||||
| C5 | WorkOrders/WorkOrderComments naming violates kebab-case but replacement-locked → document as legacy, don't rename | **unverified** | low |
|
||||
| C6 | constructs floor-pinned; stale version comments | **unverified** | low |
|
||||
| C7 | cdk.Environment omits account; Bedrock ARN hardcodes it | **unverified** | low |
|
||||
| C8 | No function-ARN CfnOutputs; WO stack has zero outputs | **unverified** | low |
|
||||
|
||||
### Contracts & docs
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| X1 | README "Data is never corrupted" false for PO fallback path (docs/po-template-parser.md contradicts it via issue #101) | confirmed | high |
|
||||
| X2 | Three inconsistent site_code definitions; stream field-contract subsection missing (README does list site_extractor as consumer — gap is field-level) | confirmed | high |
|
||||
| X3 | WO emits ParseOutcome after Bedrock → throttle drops record from alarm denominator, contradicting README ("emit-before" fix rejected — double-counts; use except-and-reraise with bedrock_error reason) | confirmed | medium |
|
||||
| X4 | Stale quantity/price prompt-type comments; doc self-contradicts §8 vs §10 | confirmed | medium |
|
||||
| X5 | README omits derived-field suites/derived_fields.py/docs/; 64-char clamp undocumented | confirmed | medium |
|
||||
| X6 | derived_fields "FAITHFUL v1 port" docstring false (contradicts its own line-100 backtest comments); reword to spec-superset + keep runtime-authority nuance | confirmed | medium |
|
||||
| X7 | Web UI invocation contract stale ×3; README example guaranteed 401 | confirmed | medium |
|
||||
| X8 | verified-sites contract: lat/long/notes provenance unknown; INFRA-138 by-state GSI breach live (restore would be a sparse GSI — relevant to the decision) | confirmed | medium |
|
||||
|
||||
### Deploy pipeline & bundling (gap dimension)
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| P1 | Deploy runs zero tests; cd-cdk pre-flight/health-check dormant (no stack-name); admin bypass allows zero-CI production deploy | confirmed | high |
|
||||
| P2 | cp allowlist invisible to every gate → production ImportError class (bit PR #105); glob + ast test | confirmed | high |
|
||||
| P3 | Migration sequencing: guards → root-move → extraction, each deployed/verified (PR-1 corrections: WO zip not byte-identical; pip -r path change) | confirmed | high |
|
||||
| P4 | Divergent bundling strategies; WO ships tests/ + __pycache__; from_asset ignores .gitignore (hash-poisoning is in WO cp -r and the three non-bundled assets, not PO's allowlist) | confirmed | medium |
|
||||
| P5 | Broken-bundle detection traffic-dependent; no synthetic canary (even with stack-name set, CFN status check passes a broken bundle — sync invoke + FunctionError check is the load-bearing part) | confirmed | medium |
|
||||
|
||||
### DLQ / recovery (gap dimension)
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| R1 | No WO recovery tooling — reprocess.py hardcoded PO-only; async-destination DLQ has no console redrive, so scripted re-invoke is the ONLY path | confirmed | high |
|
||||
| R2 | No DLQ recovery runbook; 14-day-DLQ vs 90-day-S3 windows stated nowhere | confirmed | high |
|
||||
| R3 | reprocess.py --execute: full-prefix replay not order-safe (Event invocation = concurrent; sorting alone doesn't serialize); same-object replay idempotent | confirmed | medium |
|
||||
| R4 | Batch-abort → whole-event DLQ message; redrive re-runs succeeded records (safe by idempotency, undocumented; do NOT add per-record swallowing) | confirmed | low |
|
||||
|
||||
### Dependency hygiene (gap dimension)
|
||||
| # | Finding | Verdict | Impact |
|
||||
|---|---|---|---|
|
||||
| H1 | Floor-pinned boto3 in Docker bundles → non-reproducible artifacts; vendored copy entirely unnecessary (runtime boto3 suffices) | confirmed | high |
|
||||
| H2 | Target-state manifest table; layer-vs-shared-dir Dependabot implication (shared/ source dir with no manifest needs no new entry) | confirmed | medium |
|
||||
| H3 | po/web_ui + site_extractor: no requirements.txt, invisible to Dependabot (cite cdk-project-layout.md, not lambda-template's SAM rationale) | confirmed | low |
|
||||
| H4 | wo/web_ui requirements.txt is a dead manifest (10 no-op Dependabot PRs merged; file ships as dead weight in the zip) | confirmed | low |
|
||||
| H5 | tests/requirements.txt uncovered + floor pin (pin first or the dependabot entry is a no-op) | confirmed | low |
|
||||
|
||||
---
|
||||
|
||||
## 6. Explicitly out of scope / rejected
|
||||
|
||||
| Rejected | Why |
|
||||
|---|---|
|
||||
| **Merging the two template parsers** | 15.4% similarity; extractors and validation rules are genuinely domain-specific (Coupa PO vs Hexagon WO). Only the byte-identical low-level helpers are extraction candidates, and even that is low-impact/unverified (D8). |
|
||||
| **CDK Construct-subclass composition** | Changes every child logical ID → CloudFormation replacement of RETAIN tables and create-failure on named buckets. Plain functions with unchanged scope/ids, gated on zero `cdk diff`, achieve the dedup safely. |
|
||||
| **Lambda layer for shared code** | Creates a cross-stack export (layer owned by one stack, consumed by the other) that blocks independent updates under `cdk deploy --all`. Asset-root widening keeps each function self-contained. Revisit only if shared code grows real third-party deps. |
|
||||
| **Renaming WorkOrders/WorkOrderComments/non-prefixed resources to kebab-case** | table_name/function_name changes force resource replacement (production data; cross-stack `fromTableName` consumers). Correct move is a handbook Legacy-section entry; revisit at the INFRA-6 CMK table touch. |
|
||||
| **Reordering WO's emit_parse_metric before the Bedrock call ("PO parity")** | Would double-count gate-rejected emails against wo_stack's fallback-rate MathExpression and violate the documented "rejected emits nothing else" contract. Fix is except-and-reraise with a `bedrock_error` reason code + a pinning test. |
|
||||
| **Moving PO's pre-Bedrock metric emission after the gate** | The pre-call emit is deliberate (Bedrock errors still record ai_fallback) and load-bearing for the alarm; ai_fallback_rejected is additive instead. |
|
||||
| **Per-record exception swallowing in handler loops** | Would remove the retry→DLQ capture and weaken fail-closed behavior. If isolation is ever wanted: collect failures, re-raise at end. Documented in the runbook instead. |
|
||||
| **Dedupe/skip logic inside handlers for replay safety** | Weakens the deliberate always-upsert semantics; replay safety handled in the reprocess script (targeted-by-default) and runbook. |
|
||||
| **Generating EXTRACTION_PROMPT from derived_fields rule tables** | Defeats the shadow bake's purpose (comparing two independent implementations). Appropriate only after Python becomes authoritative and the prompt sections are deleted. |
|
||||
| **Any derived_fields.py behavior change before the bake concludes** | Shadow-telemetry constraint: the module is under agreement measurement; restructuring (incl. the _classify_desc ladder refactor, Q9) confounds the telemetry. Docstring fix (X6) is text-only and allowed. |
|
||||
| **Adding alarms to wo-web-ui as a side effect of the alarm helper** | New paging surface; propose separately and deliberately, never silently via refactor. |
|
||||
| **Consolidating the fallback-rate alarms into one parameter-free helper** | PO's 6h/≥8-floor tuning is a deliberate, documented retune for ~57 emails/day vs WO's 15-min/≥10; helper parameterizes, per-stack values stay. |
|
||||
| **fromTableName→CfnOutput coupling changes for slack-bot consumers** | Cross-repo; out of this refactor. CfnOutputs are added (C8) but consumer migration is seahaven-slack-bot's change. |
|
||||
| **Full rewrite / repo split** | Nothing measured supports it: the tested tree is healthy (93%, fast suite, clean lint); every problem is addressable by consolidation with production continuity. |
|
||||
|
||||
---
|
||||
*Cross-cutting compliance notes for execution: Phases 0, 1, 3 touch untrusted-input/auth surfaces → `/sh-security-review` mandatory before push. Phase 4 moves IAM policy construction and Phase 0 touches handler event contracts → cross-family `cross_review.py` review mandatory. Every PR: `ruff check` + `ruff format --check` locally before push; `gh pr checks` green before merge; deploy-then-merge for all deploy-affecting phases. README updates land in the same commits as the changes they describe; the Confluence "AWS Architecture Map" needs updating when Phase 4/7 alter the resource inventory (new outputs, runbook).*
|
||||
|
|
@ -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"]
|
||||
|
|
|
|||
172
lambdas/po/email_processor/tests/test_po_healthcheck.py
Normal file
172
lambdas/po/email_processor/tests/test_po_healthcheck.py
Normal file
|
|
@ -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")]
|
||||
|
|
@ -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"]
|
||||
|
|
|
|||
69
lambdas/wo/email_processor/tests/test_healthcheck.py
Normal file
69
lambdas/wo/email_processor/tests/test_healthcheck.py
Normal file
|
|
@ -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",
|
||||
}
|
||||
95
scripts/post-deploy-smoke.sh
Executable file
95
scripts/post-deploy-smoke.sh
Executable file
|
|
@ -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"
|
||||
250
tests/test_bundle_consistency.py
Normal file
250
tests/test_bundle_consistency.py
Normal file
|
|
@ -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 `<sibling_dir>/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 `<name>.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)
|
||||
Loading…
Add table
Reference in a new issue