feat(observability): wire SNS notification action to DLQ CloudWatch alarms (#629)

[nizar-lahlali]

Summary

Follow-up to #117 / #208 (§11.5 "notification channel wiring"). The DLQ-depth alarms shipped without an addAlarmAction, so poison-pill accumulation was only visible by polling the CloudWatch Alarms console. This adds a push notification channel.

• New reusable OperationalAlerts construct (cdk/src/constructs/operational-alerts.ts): a single stack-wide SNS topic.
• Wires all three DLQ-depth alarms to it via addAlarmAction(new SnsAction(topic)):
  • FanOutConsumer.dlqDepthAlarm
  • ApprovalMetricsPublisherConsumer.dlqAlarm
  • GitHubScreenshotIntegration.processorDlqDepthAlarm
• Topic ARN exported as OperationalAlertsTopicArn.

Design notes

• Customer-managed KMS key, not alias/aws/sns. This is load-bearing: CloudWatch cannot publish to a topic encrypted with the AWS-managed key (its policy can't be edited to grant cloudwatch.amazonaws.com kms:Decrypt/GenerateDataKey*), so the alarm action would fail silently at delivery. The CMK grants CloudWatch exactly those actions and satisfies cdk-nag AwsSolutions-SNS2. TLS-on-publish is enforced via an explicit DenyInsecureTransport topic policy (AwsSolutions-SNS3).
• Configurable delivery target. -c [email protected] creates an email subscription (AWS sends a confirmation link). With no context set, the topic ships with no subscription — operators wire Slack / PagerDuty / email manually against the exported ARN. Not hard-coded.

Deploy-role changes (required)

For a fresh cdk deploy to create these resources, the CFN execution role needs SNS + CMK create/lifecycle permissions:

• Application policy: new SNS statement (arn:aws:sns:*:*:backgroundagent-dev-*).
• Observability policy: new KMSCustomerManagedKeys statement.
• Bumped bootstrap bundle 1.3.0 → 1.4.0, regenerated JSON artifacts + template + hash, updated DEPLOYMENT_ROLES.md golden baseline, and the resource-action-map synth-coverage guard.

Docs

• docs/design/CEDAR_HITL_GATES.md §11.5 + deferred note updated; Starlight mirrors regenerated.

Testing

• mise run build — green (3839 cdk tests pass).
• New operational-alerts.test.ts (topic/CMK/key-policy/TLS/subscription/alarm-action, 100% coverage) + 3 new agent.test.ts stack assertions (single topic, all-3-alarms-wired, no-email-by-default).
• Bootstrap policy/golden/synth-coverage/artifact-sync tests updated and passing.

Note: mise run security:sast:masking fails on this branch, but it fails identically on the clean base (42 pre-existing findings, none in files touched here) — not introduced by this PR.

Closes #629.

[ayushtr-aws]

PR #739 Review — feat(observability): wire SNS notification action to DLQ CloudWatch alarms (#629)

1. Verdict

Approve with nits. The change is correct, well-tested for its core behavior, CI is green, docs and the bootstrap bundle are complete and in sync. Two things should be tightened before
merge — both are polish, neither is functional: a code comment that overstates a security bound it doesn't actually deliver, and two untested security properties. No blocking defects.

2. Vision alignment

Directly advances bounded blast radius & observability: DLQ poison-pill accumulation was silent (console-poll only); this closes the §11.5 "notification channel wiring" follow-up so
alarms push. It's purely additive, preserves fire-and-forget, and keeps outcomes reviewable (exported topic ARN, documented in CEDAR_HITL_GATES.md). Issue #629 is approved + assigned to
the author; branch name conforms. Fits cleanly.

3. Blocking issues

None.

4. Non-blocking suggestions / nits

N1 — Comment overstates the KMS blast-radius bound (fix wording or tighten the grant). cdk/src/bootstrap/policies/observability.ts:158-166,179-201
The lifecycle statement gates kms:PutKeyPolicy / kms:ScheduleKeyDeletion on aws:ResourceTag/ABCA=operational-alerts, but the sibling KMSCustomerManagedKeys statement grants
kms:TagResource unconditioned on *. The same role can therefore stamp ABCA=operational-alerts onto any account CMK and then satisfy the gate to take over or schedule-delete that foreign
key. So the comments "none can escalate privilege on an existing key" and "bounds the blast radius to keys this solution owns" are not true against a misused/compromised role — the
tag-gate is an accident-guard (works for normal CloudFormation operation), not a security boundary.
In practice the incremental risk is bounded: this is a CloudFormation execution role that already holds iam:CreateRole/PutRolePolicy (infrastructure.ts:63-84), so it is already able to
escalate to KMS admin by other means — which is why I score this a nit, not a blocker. But the misleading wording shouldn't land as-is. Pick one:

• (a) Reword to state honestly that the tag scopes against accidental CloudFormation targeting, not a compromised-role bound; or
• (b) Drop kms:ScheduleKeyDeletion (and ideally PutKeyPolicy) from the identity grant and let the key's own resource policy + RemovalPolicy govern deletion — that actually shrinks the
  exposure the comment claims to bound. If TagResource stays, condition it on aws:RequestTag/ABCA/aws:TagKeys to at least document intent.

N2 — DenyInsecureTransport covers only sns:Publish. cdk/src/constructs/operational-alerts.ts:136-143 — canonical secure-transport guardrails deny all actions;
Subscribe/SetTopicAttributes/etc. over non-TLS are still allowed. Belt-and-suspenders (SNS endpoints are HTTPS in practice), but broaden the Deny to sns:* so it's as strong as it reads.

N3 — kms:GenerateDataKey* wildcard is broader than needed. operational-alerts.ts:118 — SNS envelope encryption needs only kms:GenerateDataKey. The * also matches the data-key-pair
variants. Common CDK idiom, key-scoped, low risk — optional tighten.

N4 — No warning when the topic ships with zero subscribers. operational-alerts.ts — with no alertEmail and nothing wired later, alarms publish to a subscriber-less topic and notify no
one while looking wired — the exact silent-DLQ failure #629 exists to fix. Consider Annotations.of(this).addWarning(...) at synth when no subscription exists. (Fail-closed on bad email
input is already correctly handled — good.)

5. Documentation

Complete and in sync. CEDAR_HITL_GATES.md §11.5 rewritten from "follow-up — deferred" to "shipped (#629)", the §17 future-work item struck through, and DEPLOYMENT_ROLES.md golden
baseline updated with both new statements. I regenerated the Starlight mirror locally and confirmed zero drift — Cedar-hitl-gates.md and Deployment-roles.md mirrors match their sources;
CI's "Fail build on mutation" will pass.

6. Tests & CI

All 8 CI checks green (build incl. 3844 cdk tests, CodeQL ×3, secrets/deps, PR-lint). The pre-existing security:sast:masking noise is not run in CI here and, as the author notes, is
baseline (unrelated files).

Bootstrap synth-coverage: PASS / complete. This is the highest-risk area for this repo (ADR-002 / #350) and it's done right — BOOTSTRAP_VERSION 1.3.0→1.4.0 (correct minor bump for added
actions), regenerated application.json/observability.json/bootstrap-template.yaml, BOOTSTRAP_HASH matches the version.test.ts snapshot (verified: 4591304…d77d in both),
resource-action-map.ts registers AWS::SNS::Topic/Subscription/TopicPolicy and AWS::KMS::Key, and policies.test.ts adds strong per-verb guards. I verified the SNS resource pattern
arn:aws:sns:::backgroundagent-dev-* matches the auto-generated topic name (stack name defaults to backgroundagent-dev, main.ts:35).

Test gaps (from pr-test-analyzer, both worth adding):

• The ABCA=operational-alerts tag on the key is never asserted on the construct that stamps it. policies.test.ts asserts the policy expects the tag, but nothing asserts the key carries
  it — the linchpin of the whole scoping model. Drop/rename the tag and every test stays green while production key-lifecycle silently breaks. Add hasResourceProperties('AWS::KMS::Key', {
  Tags: Match.arrayWith([{ Key: 'ABCA', Value: 'operational-alerts' }]) }).
• The aws:SourceAccount confused-deputy condition is not asserted. operational-alerts.test.ts:72-81 matches only Principal+Action; deleting the conditions block keeps the test green
  (AI005 — tests what it does, not what it should). Add the condition to the objectLike.
• Minor: the DenyInsecureTransport test omits Principal/Resource (a deny scoped to the wrong ARN would pass); and the 7 construct tests re-synth per-test rather than caching in beforeAll
  (cdk/AGENTS.md convention, #366) — 5 use the identical no-props config. No bundling is re-enabled (correct).

7. Review agents run

• security-review (IAM/KMS/SNS least-privilege, confused-deputy, fail-open) — ran. Surfaced N1 (its one MEDIUM), N2, N3, N4; cleared aws:SourceAccount sufficiency, the SNS action set,
  CreateKey-on-*, and dual removalPolicy.
• pr-test-analyzer (coverage) — ran. Surfaced the three test gaps in §6.
• comment-analyzer (comment accuracy) — ran. Verified all five load-bearing claims (CloudWatch-can't-use-alias/aws/sns; CMK satisfies SNS2 and SNS3 short-circuits; addAlarmAction on
  AlarmBase; atomic tag-at-create; Resource:'*' is key-local) against cdk-nag source and aws-cdk-lib type defs — all accurate. (Note the tension: comment-analyzer confirmed the
  AWS-behavior claims true, while security-review flagged the blast-radius claim as overstated — N1. Both hold: the mechanics are described correctly; the security guarantee is not.)
• code-reviewer / silent-failure-hunter / type-design-analyzer — folded into the hand-review rather than run as separate agents: the diff is a single small construct + IAM/doc deltas
  already covered by security + comment + test agents. Error handling is one synth-time throw (fail-closed, tested); the one new type (OperationalAlertsProps) is minimal and
  well-documented. No separate findings.

8. Human heuristics

• Proportionality — Pass. A reusable ~172-line construct for a stack-wide channel is right-sized; not over-abstracted (it's genuinely shared by 3 alarms and designed for more).
• Coherence — Pass. Belongs in cdk/src/constructs/; aws:SourceAccount pinning mirrors the lambda-microvm-compute.ts precedent; bootstrap changes follow the established split.
• Clarity — Concern (observability.ts:158-166). Names and structure are excellent, but the KMS-lifecycle comment asserts a bound the unconditioned kms:TagResource doesn't deliver (N1).
  This is the one place a future reader is invited to trust something false.
• Appropriateness — Pass. Maintainable; the load-bearing CMK rationale is documented so the next contributor won't "simplify" it back to alias/aws/sns. Verified against real cdk-nag/AWS
  behavior, not just self-written mocks.