fix(s3): correct the would-have-caught claim, harden the strip-probes, and guard a null EventBridge block

Review fix-back for #1437. The headline correction: this target's central claim
was FALSE, and the tests that were supposed to prove it were passing for the
wrong reason.

WHAT THE CRITIC WOULD ACTUALLY HAVE CAUGHT. Measured by running it against the
REAL pre-#1426 provider (`git show 768cd44^`) rather than predicted: it flags
`Transition`, `NoncurrentVersionTransition` and `NoncurrentVersionExpirationInDays`
-- the legacy-singular defect. Issue #1430 predicted `TagFilters`,
`NoncurrentVersionTransitions[].TransitionInDays` and rule-level
`ExpiredObjectDeleteMarker`, and was wrong on all three:

- `TagFilters` (16 literal sites pre-#1426) and `TransitionInDays` (2) are also
  named by `readCurrentState`'s reverse map, so the file-global literal
  heuristic reports `provider-handled` however broken the write path is. That
  is item 2 of #1393.
- `ExpiredObjectDeleteMarker` is not shape-audited at all: the shape pass
  matches CFn definitions to same-named SDK interfaces, and
  `@aws-sdk/client-s3` spells it `LifecycleRule`, so CFn's `Rule` sits in
  `unmatchedDefinitions` and the whole lifecycle-rule blob is unaudited.

The strip-probes inherited the wrong list, and `replaceAll` hid it by deleting
EVERY occurrence -- a regression shape that cannot occur. They now probe only
keys a realistic single-site regression can actually silence, and each probe
ASSERTS the key left `collectStringLiterals`'s evidence set before asserting
the bucket. That self-validation immediately caught a second miss: the read-side
fix makes `EventBridgeEnabled` an object-literal property name as well as a
quoted literal, so stripping only `'EventBridgeEnabled'` left it in evidence. A
new test pins the honest limit -- removing ONLY TagFilters' write-side
conversion must still classify `provider-handled` -- so nobody re-adds the
false claim.

Provider fixes:
- `EventBridgeConfiguration: null` threw. The branch now reads a member off the
  block, so an explicit null (hand-written JSON, or an intrinsic resolving to
  null) hit `Cannot read properties of null` where the pre-change
  `eb !== undefined` test merely emitted. Guarded with the file's existing
  `isPlainObject`; non-objects stay on the enable-on-presence side.
- `coerceCfnBoolean` is now case-insensitive. This is the one call site where
  "not false" means "turn it on", so `'False'` falling through to `undefined`
  would silently ENABLE delivery -- the exact inversion this PR fixes.
- The `readCurrentState` comment claimed the always-emit is invisible to a state
  record with no `EventBridgeConfiguration` key. Not true: `cdkd drift` uses
  `unionWalkObjects: true`, so upgrading reports ONE cosmetic diff per bucket
  until the next `state refresh-observed` / `drift --accept` / real UPDATE. The
  comment now says so, matching the convention in ec2 / ssm / s3-tables.

Tests: +6 units (case variants, null block, EventBridge-false alongside a Topic
config, and the true -> false UPDATE flip, which no existing test covered since
they all start from a bare previous state). The round-trip test was vacuous --
with the read fix reverted the reader returns `{}`, which the write path also
emits as `{}` -- so it is split, and the DISCRIMINATING direction is now
covered: the disabled shape fed back through the write path must NOT re-enable
delivery, which is what a `drift --revert` would do.

The `#1378` definitionShapes fence sorts before picking its stand-in fixture
(`readdirSync` order differs between macOS and CI) and requires a string
`resourceType`, so it cannot pick the bookkeeping file and throw a TypeError
instead of the assertion it means to make.

Integ: the EventBridge + drift assertions are factored into functions and run
after BOTH phases, so the `diffSubConfig` -> `applyNotificationConfiguration`
UPDATE call site is covered against real AWS too. Re-run: PASS, destroy 3
deleted / 0 errors / 0 orphans.

Also corrects "the largest target" (CloudFront Distribution is 121 keys, S3 is
115) in `.claude/rules/code-layout.md`.

Author: go-to-k

.claude/rules/code-layout.md

@@ -230,7 +230,7 @@ s3-asset-deploy	2026-07-26T19:45:58Z	PASS	58	verify.sh	0727b sweep-b12 staleness
 s3-cloudfront	2026-08-08T17:42:05Z	PASS	228	verify.sh	1373 OriginCustomHeaders create+update-survive asserts green; destroy 0 errors 0 orphans
 s3-directory-bucket	2026-08-02T15:38:47Z	PASS	50	verify.sh	re-run for #1347 wontdo-comment PR; guard + clean destroy, 0 orphans
 s3-event-notification	2026-07-21T05:28:05Z	PASS	79	verify.sh	rc ok, orph clean
-s3-lifecycle	2026-08-09T15:54:29Z	PASS	150	verify.sh	#1430 EventBridgeEnabled true/false + clean-drift asserts added; destroy 0 err, 0 orph
+s3-lifecycle	2026-08-09T16:35:24Z	PASS	84	verify.sh	#1430 EventBridge pair + drift asserted after BOTH phases; destroy 0 err, 0 orph
 s3-object-lock	2026-07-21T14:42:20Z	PASS	53	verify.sh	rc ok, orph clean
 s3-replication-and-filter	2026-07-21T14:43:45Z	PASS	63	verify.sh	rc ok, orph clean
 s3-tables	2026-07-27T16:53:47Z	PASS	45	verify.sh	issue #1270/#1272 post-review re-run; 5 del 0 err, 0 orphans

docs/_generated/integ-last-run.tsv

@@ -237,11 +237,28 @@ export const NESTED_KEY_TARGETS: readonly NestedKeyTarget[] = [
     // inventory, analytics, metrics, intelligent-tiering, object-lock,
     // website routing) in `handledProperties` and re-shapes each for the SDK —
     // the forwarding shape this critic exists to audit. It was NOT a target
-    // when the #1388 / #1424 lifecycle defects were fixed by hand in PR #1426,
-    // and three of those six would have been caught mechanically here
-    // (`TagFilters` + `NoncurrentVersionTransitions[].TransitionInDays` as
-    // `no-sdk-member` on the key pass, rule-level `ExpiredObjectDeleteMarker`
-    // as `definition-member-missing` on the shape pass).
+    // when the #1388 / #1424 lifecycle defects were fixed by hand in PR #1426.
+    //
+    // WHAT THIS TARGET WOULD ACTUALLY HAVE CAUGHT, measured rather than
+    // predicted: running this critic against the REAL pre-#1426 provider
+    // (`git show 768cd44b^:…/s3-bucket-provider.ts`) flags exactly three
+    // lifecycle keys as `no-sdk-member` — `Transition`,
+    // `NoncurrentVersionTransition` and `NoncurrentVersionExpirationInDays`,
+    // i.e. the LEGACY SINGULAR FORMS defect.
+    //
+    // Issue #1430 predicted a different three, and it was wrong on all of
+    // them. `TagFilters` (16 literal occurrences pre-#1426) and
+    // `TransitionInDays` (2) were already named elsewhere in the file — by
+    // `readCurrentState`'s reverse map — so the file-global literal heuristic
+    // classifies both `provider-handled` no matter how broken the WRITE path
+    // is. That is exactly the blind spot tracked as item 2 of #1393, and it is
+    // why the strip-probes in the unit test use only single-occurrence keys: a
+    // probe that deletes EVERY occurrence of a 16-site literal tests a
+    // regression shape that cannot occur. Rule-level `ExpiredObjectDeleteMarker`
+    // is not reachable either — the shape pass matches CFn definitions to
+    // same-named SDK interfaces, and `@aws-sdk/client-s3` spells it
+    // `LifecycleRule`, so CFn's `Rule` sits in `unmatchedDefinitions` and the
+    // whole lifecycle-rule blob is shape-unaudited.
     resourceType: 'AWS::S3::Bucket',
     providerFile: 's3-bucket-provider.ts',
     sdkClientPackage: '@aws-sdk/client-s3',

docs/changelog-cdkd.md

@@ -121,8 +121,16 @@ function mergeLegacySingular(
 /** Coerce a CFn boolean, which may arrive as the string `"true"` / `"false"`. */
 function coerceCfnBoolean(value: unknown): boolean | undefined {
   if (typeof value === 'boolean') return value;
-  if (value === 'true') return true;
-  if (value === 'false') return false;
+  // Case-insensitive on purpose. CDK renders lowercase, but this also feeds
+  // `NotificationConfiguration.EventBridgeConfiguration` (issue #1430), where
+  // "not false" means "enable" — so a hand-written / imported `'False'` that
+  // fell through to `undefined` would silently ENABLE EventBridge delivery,
+  // the exact inversion #1430 fixed.
+  if (typeof value === 'string') {
+    const lowered = value.toLowerCase();
+    if (lowered === 'true') return true;
+    if (lowered === 'false') return false;
+  }
   return undefined;
 }
 
@@ -873,8 +881,15 @@ export class S3BucketProvider implements ResourceProvider {
           Filter: this.cfnNotifFilterToSdk(l['Filter']),
         }));
       }
-      const eb = notifConfig['EventBridgeConfiguration'] as Record<string, unknown> | undefined;
-      if (eb !== undefined && coerceCfnBoolean(eb['EventBridgeEnabled']) !== false) {
+      const eb = notifConfig['EventBridgeConfiguration'];
+      // `isPlainObject`, not `!== undefined`: this branch now READS a member off
+      // the block, so an explicit `null` (hand-written JSON / an intrinsic that
+      // resolved to null) would throw where it previously just emitted the
+      // block. A non-object stays on the pre-change enable-on-presence side.
+      if (
+        eb !== undefined &&
+        (!isPlainObject(eb) || coerceCfnBoolean(eb['EventBridgeEnabled']) !== false)
+      ) {
         // The SDK's `EventBridgeConfiguration` is an EMPTY structure — presence
         // enables EventBridge delivery, absence disables it. CFn instead carries
         // a REQUIRED boolean `EventBridgeEnabled` inside the block (that is what
@@ -2352,11 +2367,22 @@ export class S3BucketProvider implements ResourceProvider {
     }
     // Always-emit, and in the CFn shape (`{EventBridgeEnabled: <bool>}`) rather
     // than the SDK's empty-structure shape — cdkd's state baseline holds the
-    // CFn spelling, and `drift-calculator` descends only into keys present in
-    // state, so returning `{}` reported the boolean as permanently missing on
-    // every EventBridge-enabled bucket (issue #1430). Emitting `false` when the
-    // response omits the block keeps the disabled side comparable too; a state
-    // record with no `EventBridgeConfiguration` key is unaffected either way.
+    // CFn spelling, so returning `{}` reported the boolean as permanently
+    // missing on every EventBridge-enabled bucket (issue #1430). Emitting
+    // `false` when the response omits the block keeps the disabled side
+    // comparable too.
+    //
+    // ONE-TIME DRIFT ON UPGRADE, accepted deliberately. `cdkd drift` runs the
+    // observed-properties path with `unionWalkObjects: true`, which walks the
+    // union of baseline+AWS keys inside a nested object, so a state record
+    // captured by an older binary reports one diff on the first run after
+    // upgrading: `EventBridgeConfiguration: {}` vs `{EventBridgeEnabled: true}`
+    // on an enabled bucket, or an added `EventBridgeConfiguration` on a bucket
+    // with no such key at all. It is cosmetic — `--revert` re-sends the old
+    // observed blob and the write side re-derives the same AWS state — and it
+    // clears on the next `cdkd state refresh-observed`, `drift --accept`, or
+    // real UPDATE of the bucket. The alternative (emit only when present) is
+    // what caused the PERMANENT phantom drift this fixes.
     out['EventBridgeConfiguration'] = {
       EventBridgeEnabled: resp.EventBridgeConfiguration !== undefined,
     };

scripts/gen-nested-key-coverage.ts

@@ -13,10 +13,14 @@
 # Phases:
 #   1. Deploy; assert all three rules reached AWS, none carries a top-level Prefix
 #      (all normalized to V2 Filter form), and the archive rule's expiration=730.
+#      Also assert the legacy singular lifecycle keys (issue #1388 / #1424) and
+#      the issue #1430 EventBridgeEnabled pair (true -> block present, false ->
+#      block absent, matching CloudFormation), then `cdkd drift` clean.
 #   2. Re-deploy with CDKD_TEST_UPDATE=true (expiration 730 -> 365, GLACIER
 #      transition 90 -> 60, + a new big-objects Filter rule). Assert the new
 #      values reached AWS, there are 4 rules, and the bucket was NOT replaced.
-#   3. Destroy; assert the bucket is gone and the state file is removed.
+#      Re-assert the EventBridge pair + drift, so the UPDATE path is covered too.
+#   3. Destroy; assert all three buckets are gone and the state file is removed.
 #
 # Required env vars:
 #   STATE_BUCKET — cdkd state bucket (e.g. cdkd-state-{accountId})
@@ -216,53 +220,66 @@ echo "    legacy singular Transition + NoncurrentVersionTransition + NoncurrentV
 # ENABLED notifications.
 #
 # Expected values are CloudFormation ground truth, not a guess: a real CFn A/B
-# of this exact shape (stack Cdkd1430EbProbe, us-east-1, 2026-08-10) returned
-# an EMPTY body for `false` and `{"EventBridgeConfiguration": {}}` for `true`.
+# of this exact shape (stack Cdkd1430EbProbe, us-east-1, 2026-08-10) gave an
+# EMPTY response for `false` and `{"EventBridgeConfiguration": {}}` for `true`.
 #
-# An unconfigured bucket returns an EMPTY body, not `{}`, so both captures are
-# normalized before jq sees them -- without that the `false` assertion fails on
-# cdkd's CORRECT output. (The first real run of this assertion did exactly
-# that.) The captures are unguarded on purpose: `set -e` aborts on a genuine
-# API failure, so reaching the normalization with an empty string means a
-# successful call on an unconfigured bucket.
-EB_TRUE_JSON="$(aws s3api get-bucket-notification-configuration \
-  --bucket "${EB_TRUE_BUCKET}" --region "${REGION}" --output json)"
-EB_FALSE_JSON="$(aws s3api get-bucket-notification-configuration \
-  --bucket "${LEGACY_BUCKET}" --region "${REGION}" --output json)"
-[ -n "${EB_TRUE_JSON//[[:space:]]/}" ] || EB_TRUE_JSON='{}'
-[ -n "${EB_FALSE_JSON//[[:space:]]/}" ] || EB_FALSE_JSON='{}'
-
-EB_TRUE_HAS="$(printf '%s' "${EB_TRUE_JSON}" | jq -r 'has("EventBridgeConfiguration")')"
-EB_FALSE_HAS="$(printf '%s' "${EB_FALSE_JSON}" | jq -r 'has("EventBridgeConfiguration")')"
-
-# The `true` side runs FIRST and is the vacuity guard: asserting only that the
-# `false` bucket lacks the block would pass just as happily if cdkd stopped
-# applying NotificationConfiguration altogether.
-if [ "${EB_TRUE_HAS}" != "true" ]; then
-  echo "FAIL: ${EB_TRUE_BUCKET} (EventBridgeEnabled: true) has NO EventBridgeConfiguration" >&2
-  echo "      response: ${EB_TRUE_JSON}" >&2
-  exit 1
-fi
-if [ "${EB_FALSE_HAS}" != "false" ]; then
-  echo "FAIL: ${LEGACY_BUCKET} (EventBridgeEnabled: false) HAS an EventBridgeConfiguration" >&2
-  echo "      this is the issue #1430 inversion: an explicit false enabled delivery" >&2
-  echo "      response: ${EB_FALSE_JSON}" >&2
-  exit 1
-fi
-echo "    EventBridgeEnabled true -> block present, false -> block absent (matches CloudFormation)"
+# Run after BOTH phases: Phase 1 covers create(), Phase 2 covers the
+# diffSubConfig -> applyNotificationConfiguration UPDATE path, which is a
+# different call site and was previously unexercised against real AWS.
+assert_eventbridge_pair() { # $1 = phase label
+  local phase="$1"
+  # An unconfigured bucket returns an EMPTY body, not `{}`, so both captures
+  # are normalized before jq sees them -- without that the `false` assertion
+  # fails on cdkd's CORRECT output. (The first real run of this assertion did
+  # exactly that.) The captures are unguarded on purpose: `set -e` aborts on a
+  # genuine API failure, so reaching the normalization with an empty string
+  # means a successful call on an unconfigured bucket.
+  local eb_true_json eb_false_json eb_true_has eb_false_has
+  eb_true_json="$(aws s3api get-bucket-notification-configuration \
+    --bucket "${EB_TRUE_BUCKET}" --region "${REGION}" --output json)" || return 1
+  eb_false_json="$(aws s3api get-bucket-notification-configuration \
+    --bucket "${LEGACY_BUCKET}" --region "${REGION}" --output json)" || return 1
+  [ -n "${eb_true_json//[[:space:]]/}" ] || eb_true_json='{}'
+  [ -n "${eb_false_json//[[:space:]]/}" ] || eb_false_json='{}'
+
+  eb_true_has="$(printf '%s' "${eb_true_json}" | jq -r 'has("EventBridgeConfiguration")')" || return 1
+  eb_false_has="$(printf '%s' "${eb_false_json}" | jq -r 'has("EventBridgeConfiguration")')" || return 1
+
+  # The `true` side is the vacuity guard: asserting only that the `false`
+  # bucket lacks the block would pass just as happily if cdkd stopped applying
+  # NotificationConfiguration altogether. Both assertions run unconditionally;
+  # it is the PRESENCE of the true-side check that is load-bearing, not the
+  # order in which they appear.
+  if [ "${eb_true_has}" != "true" ]; then
+    echo "FAIL [${phase}]: ${EB_TRUE_BUCKET} (EventBridgeEnabled: true) has NO EventBridgeConfiguration" >&2
+    echo "      response: ${eb_true_json}" >&2
+    exit 1
+  fi
+  if [ "${eb_false_has}" != "false" ]; then
+    echo "FAIL [${phase}]: ${LEGACY_BUCKET} (EventBridgeEnabled: false) HAS an EventBridgeConfiguration" >&2
+    echo "      this is the issue #1430 inversion: an explicit false enabled delivery" >&2
+    echo "      response: ${eb_false_json}" >&2
+    exit 1
+  fi
+  echo "    [${phase}] EventBridgeEnabled true -> block present, false -> block absent (matches CloudFormation)"
+}
 
 # Read side: `readCurrentState` must return the CFn shape
 # (`{EventBridgeEnabled: <bool>}`), not the SDK's `{}` -- the state baseline
-# holds the CFn spelling and drift-calculator only descends into keys present
-# in state, so the SDK shape reported permanent phantom drift on every
-# EventBridge-enabled bucket. `cdkd drift` exits 0 only when it finds none.
-if ! node "${LOCAL_DIST}" drift "${STACK}" \
-  --state-bucket "${STATE_BUCKET}" --region "${REGION}"; then
-  echo "FAIL: cdkd drift reported drift on a clean, freshly-deployed stack" >&2
-  echo "      (pre-#1430 the EventBridge boolean read back as permanently missing)" >&2
-  exit 1
-fi
-echo "    no drift on a clean stack (#1430 read side)"
+# holds the CFn spelling, so the SDK shape reported permanent phantom drift on
+# every EventBridge-enabled bucket. `cdkd drift` exits 0 only when it finds none.
+assert_no_drift() { # $1 = phase label
+  if ! node "${LOCAL_DIST}" drift "${STACK}" \
+    --state-bucket "${STATE_BUCKET}" --region "${REGION}"; then
+    echo "FAIL [$1]: cdkd drift reported drift on a clean, freshly-deployed stack" >&2
+    echo "      (pre-#1430 the EventBridge boolean read back as permanently missing)" >&2
+    exit 1
+  fi
+  echo "    [$1] no drift on a clean stack (#1430 read side)"
+}
+
+assert_eventbridge_pair "phase 1"
+assert_no_drift "phase 1"
 
 CREATION_P1="$(aws s3api list-buckets \
   --query "Buckets[?Name=='${BUCKET_NAME}'].CreationDate | [0]" --output text)"
@@ -293,6 +310,9 @@ if [ "${CREATION_P1}" != "${CREATION_P2}" ]; then
 fi
 echo "    bucket identity preserved (CreationDate unchanged) — no replacement"
 
+assert_eventbridge_pair "phase 2"
+assert_no_drift "phase 2"
+
 # --- Phase 3: destroy --------------------------------------------------
 echo "==> Phase 3: destroy"
 node "${LOCAL_DIST}" destroy "${STACK}" --state-bucket "${STATE_BUCKET}" --region "${REGION}" --force
@@ -302,7 +322,7 @@ echo "    bucket deleted"
 
 assert_gone_eventually "legacy bucket ${LEGACY_BUCKET} still exists after destroy" aws s3api head-bucket --bucket "${LEGACY_BUCKET}" --region "${REGION}"
 assert_gone_eventually "EventBridge bucket ${EB_TRUE_BUCKET} still exists after destroy" aws s3api head-bucket --bucket "${EB_TRUE_BUCKET}" --region "${REGION}"
-echo "    legacy bucket deleted"
+echo "    legacy + EventBridge buckets deleted"
 
 assert_gone "state file ${STATE_KEY} still exists after destroy" aws s3api head-object --bucket "${STATE_BUCKET}" --key "${STATE_KEY}"
 echo "    cdkd state removed"