fix(s3): apply the delta-review findings on the lifecycle fix-backs

mergeLegacySingular replaces plural-wins: keeping only the plural dropped a legitimate template, since a plural [{GLACIER,30}] alongside a singular {DEEP_ARCHIVE,90} is two different storage classes that S3 accepts. Now both are kept, the plural wins on a StorageClass COLLISION (which S3 rejects outright), and the collision warns rather than dropping silently - the same standard this PR applies to the delete marker.

ExpiredObjectDeleteMarker now engages only on TRUE. coerceCfnBoolean returns false for an explicit false, which is a legal synth (CDK's own validation is truthy-gated), so the previous check warned about a cleanup nobody requested and, on a marker-only rule, emitted Expiration { ExpiredObjectDeleteMarker: false } - action-less again, the exact failure the block exists to prevent.

coerceCfnNumber screens the type first: Number([]) is 0 and Number([5]) is 5, so an unresolved-intrinsic array coerced to a plausible day count - the same class isPlainObject blocks on the object path. NoncurrentVersionExpiration and AbortIncompleteMultipartUpload are screened with isPlainObject too, and the plural arrays now filter their elements.

3 further unit tests (25 total) plus a third clean s3-lifecycle integ run.

Author: go-to-k

docs/_generated/integ-last-run.tsv

@@ -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-09T13:48:40Z	PASS	57	verify.sh	post-review re-run (#1388/#1424); rule-level keys asserted live; 2 del/0 err, 0 orphans
+s3-lifecycle	2026-08-09T13:59:22Z	PASS	57	verify.sh	post-delta-review re-run (#1388/#1424); rule-level keys asserted live; 2 del/0 err, 0 orphans
 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

src/provisioning/providers/s3-bucket-provider.ts

@@ -84,11 +84,40 @@ function isPlainObject(value: unknown): value is Record<string, unknown> {
  * branches exist for) can carry `"365"` where the schema says number.
  */
 function coerceCfnNumber(value: unknown): number | undefined {
-  if (value === undefined || value === null || value === '') return undefined;
+  // Screen the TYPE first: `Number([])` is 0 and `Number([5])` is 5, so an
+  // unresolved-intrinsic array would coerce to a plausible-looking day count.
+  // That is the same class `isPlainObject` blocks on the object path.
+  if (typeof value !== 'number' && typeof value !== 'string') return undefined;
+  if (value === '') return undefined;
   const n = Number(value);
   return Number.isFinite(n) ? n : undefined;
 }
 
+/**
+ * Merge a legacy SINGULAR action object into its modern plural array.
+ *
+ * S3 rejects two transitions with the same `StorageClass` and fails the whole
+ * `PutBucketLifecycleConfiguration`, so a blind concatenation is unsafe. But
+ * dropping the singular wholesale loses a legitimate template: a plural
+ * `[{GLACIER, 30}]` alongside a singular `{DEEP_ARCHIVE, 90}` is two different
+ * classes, which S3 accepts. So: keep both, let the PLURAL win on a
+ * StorageClass collision, and say so rather than dropping in silence.
+ */
+function mergeLegacySingular(
+  plural: unknown,
+  singular: unknown,
+  onCollision: (storageClass: string) => void
+): Array<Record<string, unknown>> {
+  const out = (Array.isArray(plural) ? plural : []).filter(isPlainObject);
+  if (!isPlainObject(singular)) return out;
+  const sc = singular['StorageClass'];
+  if (typeof sc === 'string' && out.some((e) => e['StorageClass'] === sc)) {
+    onCollision(sc);
+    return out;
+  }
+  return [...out, singular];
+}
+
 /** Coerce a CFn boolean, which may arrive as the string `"true"` / `"false"`. */
 function coerceCfnBoolean(value: unknown): boolean | undefined {
   if (typeof value === 'boolean') return value;
@@ -432,12 +461,16 @@ export class S3BucketProvider implements ResourceProvider {
       // object with every member `undefined` for an empty / unresolved
       // `Expiration`, so an existence check would drop the marker AND leave the
       // rule action-less — the exact failure this block exists to prevent.
+      // Only TRUE engages. `false` is a legal synth (CDK's own validation is
+      // truthy-gated), and treating it as a request would both warn about a
+      // cleanup nobody asked for and, on a marker-only rule, emit
+      // `Expiration: { ExpiredObjectDeleteMarker: false }` — action-less again.
       const ruleLevelDeleteMarker = coerceCfnBoolean(rule['ExpiredObjectDeleteMarker']);
-      if (ruleLevelDeleteMarker !== undefined) {
+      if (ruleLevelDeleteMarker === true) {
         const exp = sdkRule.Expiration as Record<string, unknown> | undefined;
         const hasDaysOrDate = exp?.['Days'] !== undefined || exp?.['Date'] !== undefined;
         if (!hasDaysOrDate) {
-          sdkRule.Expiration = { ExpiredObjectDeleteMarker: ruleLevelDeleteMarker };
+          sdkRule.Expiration = { ExpiredObjectDeleteMarker: true };
         } else {
           // S3 rejects ExpiredObjectDeleteMarker combined with Days / Date, so
           // one of the two has to go. Warn instead of dropping in silence.
@@ -452,7 +485,9 @@ export class S3BucketProvider implements ResourceProvider {
       // NoncurrentVersionExpiration. The CFn schema ALSO still accepts the
       // legacy scalar `NoncurrentVersionExpirationInDays` alongside the modern
       // object form; the object wins when both are present (issue #1388).
-      const nve = rule['NoncurrentVersionExpiration'] as Record<string, unknown> | undefined;
+      const nve = isPlainObject(rule['NoncurrentVersionExpiration'])
+        ? rule['NoncurrentVersionExpiration']
+        : undefined;
       const legacyNveDays = rule['NoncurrentVersionExpirationInDays'];
       if (nve) {
         sdkRule.NoncurrentVersionExpiration = {
@@ -468,8 +503,8 @@ export class S3BucketProvider implements ResourceProvider {
 
       // NoncurrentVersionTransitions, plus the legacy singular
       // `NoncurrentVersionTransition` object the CFn schema still accepts.
-      // Both may appear on one rule, so they are concatenated rather than
-      // treated as alternatives.
+      // Both may appear on one rule; they are MERGED, with the plural winning
+      // on a StorageClass collision (see `mergeLegacySingular`).
       const toSdkNvt = (nvt: Record<string, unknown>): Record<string, unknown> => ({
         // CFn spells the day count `TransitionInDays` on BOTH the singular
         // `NoncurrentVersionTransition` and the plural
@@ -489,25 +524,19 @@ export class S3BucketProvider implements ResourceProvider {
       const singularNvt = rule['NoncurrentVersionTransition'] as
         | Record<string, unknown>
         | undefined;
-      // The PLURAL array wins when both forms are present, matching the
-      // NoncurrentVersionExpiration policy 30 lines above. Concatenating them
-      // instead would be a REGRESSION: S3 rejects two transitions with the same
-      // StorageClass (`InvalidRequest: Found two transitions with the same
-      // storage class`) and fails the whole PutBucketLifecycleConfiguration,
-      // whereas pre-fix such a template deployed because the singular form was
-      // simply ignored.
-      const allNvts =
-        Array.isArray(nvts) && nvts.length > 0
-          ? nvts
-          : isPlainObject(singularNvt)
-            ? [singularNvt]
-            : [];
+      const allNvts = mergeLegacySingular(nvts, singularNvt, (sc) =>
+        this.logger.warn(
+          `Lifecycle rule '${(rule['Id'] as string) ?? '<unnamed>'}' on ${bucketName} declares ` +
+            `both NoncurrentVersionTransitions and the legacy NoncurrentVersionTransition for ` +
+            `storage class ${sc}; S3 rejects duplicates, so the legacy singular was ignored.`
+        )
+      );
       if (allNvts.length > 0) {
         sdkRule.NoncurrentVersionTransitions = allNvts.map(toSdkNvt);
       }
 
-      // Transitions, plus the legacy singular `Transition` object. Same
-      // concatenation rule as the noncurrent-version pair above.
+      // Transitions, plus the legacy singular `Transition` object. Same merge
+      // rule as the noncurrent-version pair above.
       const toSdkTransition = (t: Record<string, unknown>): Record<string, unknown> => ({
         Days: (t['TransitionInDays'] ?? t['Days']) as number | undefined,
         Date:
@@ -518,20 +547,21 @@ export class S3BucketProvider implements ResourceProvider {
       });
       const transitions = rule['Transitions'] as Array<Record<string, unknown>> | undefined;
       const singularTransition = rule['Transition'] as Record<string, unknown> | undefined;
-      // Plural wins over the legacy singular — see the NVT note above for why
-      // concatenating is not safe.
-      const allTransitions =
-        Array.isArray(transitions) && transitions.length > 0
-          ? transitions
-          : isPlainObject(singularTransition)
-            ? [singularTransition]
-            : [];
+      const allTransitions = mergeLegacySingular(transitions, singularTransition, (sc) =>
+        this.logger.warn(
+          `Lifecycle rule '${(rule['Id'] as string) ?? '<unnamed>'}' on ${bucketName} declares ` +
+            `both Transitions and the legacy Transition for storage class ${sc}; S3 rejects ` +
+            `duplicates, so the legacy singular was ignored.`
+        )
+      );
       if (allTransitions.length > 0) {
         sdkRule.Transitions = allTransitions.map(toSdkTransition);
       }
 
       // AbortIncompleteMultipartUpload
-      const abort = rule['AbortIncompleteMultipartUpload'] as Record<string, unknown> | undefined;
+      const abort = isPlainObject(rule['AbortIncompleteMultipartUpload'])
+        ? rule['AbortIncompleteMultipartUpload']
+        : undefined;
       if (abort) {
         sdkRule.AbortIncompleteMultipartUpload = {
           DaysAfterInitiation: abort['DaysAfterInitiation'] as number | undefined,

tests/unit/provisioning/s3-bucket-provider-lifecycle-rule-keys.test.ts

@@ -290,16 +290,14 @@ describe('S3 lifecycle rule-level + legacy singular keys (issue #1388)', () => {
   });
 
   describe('review fix-backs (PR #1426)', () => {
-    it('lets the plural array win over the legacy singular instead of concatenating', async () => {
-      // Concatenating emits two transitions with the same StorageClass, which
-      // S3 rejects outright ("Found two transitions with the same storage
-      // class") and fails the WHOLE PutBucketLifecycleConfiguration — a
-      // regression, since pre-fix such a template deployed with the singular
-      // simply ignored. Plural-wins also matches the NoncurrentVersionExpiration
-      // policy.
+    it('lets the plural win — and warns — when the singular COLLIDES on StorageClass', async () => {
+      // S3 rejects two transitions with the same StorageClass and fails the
+      // WHOLE PutBucketLifecycleConfiguration, so they cannot both be sent.
+      // Dropping the loser silently would be the very thing this PR argues
+      // against, so it warns.
       const rules = await putRules([
         {
-          Id: 'both',
+          Id: 'collide',
           Status: 'Enabled',
           Prefix: 'm/',
           Transitions: [{ StorageClass: 'GLACIER', TransitionInDays: 30 }],
@@ -309,20 +307,76 @@ describe('S3 lifecycle rule-level + legacy singular keys (issue #1388)', () => {
       expect(rules[0]!.Transitions).toEqual([
         { Days: 30, Date: undefined, StorageClass: 'GLACIER' },
       ]);
+      expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('storage class GLACIER'));
+    });
+
+    it('KEEPS both when the singular is a DIFFERENT StorageClass', async () => {
+      // S3 accepts them; dropping the singular here would lose a legitimate
+      // template for no reason.
+      const rules = await putRules([
+        {
+          Id: 'distinct',
+          Status: 'Enabled',
+          Prefix: 'm/',
+          Transitions: [{ StorageClass: 'GLACIER', TransitionInDays: 30 }],
+          Transition: { StorageClass: 'DEEP_ARCHIVE', TransitionInDays: 90 },
+        },
+      ]);
+      expect(rules[0]!.Transitions.map((t: { StorageClass: string }) => t.StorageClass)).toEqual([
+        'GLACIER',
+        'DEEP_ARCHIVE',
+      ]);
+      expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining('storage class'));
     });
 
-    it('lets the plural NoncurrentVersionTransitions win over the singular too', async () => {
+    it('merges the noncurrent pair the same way', async () => {
       const rules = await putRules([
         {
           Id: 'both-nvt',
           Status: 'Enabled',
           Prefix: 'm/',
           NoncurrentVersionTransitions: [{ StorageClass: 'GLACIER', TransitionInDays: 10 }],
-          NoncurrentVersionTransition: { StorageClass: 'GLACIER', TransitionInDays: 20 },
+          NoncurrentVersionTransition: { StorageClass: 'DEEP_ARCHIVE', TransitionInDays: 20 },
+        },
+      ]);
+      expect(rules[0]!.NoncurrentVersionTransitions).toHaveLength(2);
+      expect(rules[0]!.NoncurrentVersionTransitions[1].NoncurrentDays).toBe(20);
+    });
+
+    it('ignores ExpiredObjectDeleteMarker: false instead of warning or emitting it', async () => {
+      // `false` is a legal synth (CDK's validation is truthy-gated). Treating
+      // it as a request warned about a cleanup nobody asked for, and on a
+      // marker-only rule emitted an action-less Expiration.
+      const rules = await putRules([
+        { Id: 'off', Status: 'Enabled', Prefix: 'o/', ExpiredObjectDeleteMarker: false },
+        {
+          Id: 'off-with-days',
+          Status: 'Enabled',
+          Prefix: 'od/',
+          ExpirationInDays: 30,
+          ExpiredObjectDeleteMarker: false,
+        },
+      ]);
+      expect(rules[0]!.Expiration).toBeUndefined();
+      expect(rules[1]!.Expiration).toEqual({ Days: 30 });
+      expect(warnSpy).not.toHaveBeenCalledWith(
+        expect.stringContaining('ExpiredObjectDeleteMarker')
+      );
+    });
+
+    it('rejects a non-scalar CFn numeric instead of coercing it to 0', async () => {
+      // `Number([])` is 0 and `Number([5])` is 5, so an unresolved-intrinsic
+      // array would become a plausible-looking day count.
+      const rules = await putRules([
+        {
+          Id: 'bad-num',
+          Status: 'Enabled',
+          Prefix: 'n/',
+          NoncurrentVersionExpirationInDays: [],
+          ExpirationInDays: 5,
         },
       ]);
-      expect(rules[0]!.NoncurrentVersionTransitions).toHaveLength(1);
-      expect(rules[0]!.NoncurrentVersionTransitions[0].NoncurrentDays).toBe(10);
+      expect(rules[0]!.NoncurrentVersionExpiration).toBeUndefined();
     });
 
     it('ignores a non-object singular Transition (array / unresolved intrinsic)', async () => {