fix(s3): apply the 3-axis review fix-backs to the lifecycle rule-key work

Plural transitions now WIN over the legacy singular instead of being concatenated. Concatenating can emit two transitions with the same StorageClass, which S3 rejects outright and fails the whole PutBucketLifecycleConfiguration - a regression, since pre-fix such a template deployed with the singular ignored. Plural-wins also matches the NoncurrentVersionExpiration policy already chosen 30 lines above.

Rule-level ExpiredObjectDeleteMarker now gates on whether the built Expiration actually carries Days/Date rather than on its existence: the nested branch emits an all-undefined object for an empty Expiration, so the existence check dropped the marker AND left the rule action-less, the exact failure the block exists to prevent. A genuine Days+marker conflict now warns instead of dropping in silence.

readLifecycle reverse-maps the noncurrent transition day count to the CFn spelling TransitionInDays, matching its Transitions sibling. Emitting the SDK NoncurrentDays made cdkd drift report a permanent phantom diff on every versioned bucket with a noncurrent transition - latent until this PR, since the write side never delivered the value and both sides were empty.

Adds isPlainObject (typeof x === 'object' accepts arrays and null; a Transition: [] became an entry with no StorageClass and S3 answered MalformedXML for the whole config) and coerceCfnNumber / coerceCfnBoolean (CFn is stringly typed and these legacy branches exist for hand-written templates, where '365' / 'true' is exactly what shows up).

8 new unit tests (22 total) plus a re-run of the s3-lifecycle integ: PASS, 2 deleted / 0 errors / 0 orphans.

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:29:41Z	PASS	58	verify.sh	issue #1388/#1424 rule-level keys asserted live (tags, NoncurrentDays, 4 legacy keys); 2 del/0 err, 0 orphans
+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-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

@@ -68,6 +68,35 @@ import type {
   ResourceImportResult,
 } from '../../types/resource.js';
 
+/**
+ * A plain (non-array) object. `typeof x === 'object'` alone accepts arrays and
+ * `null`; a `Transition: []` or an unresolved intrinsic pushed through as a
+ * single transition entry makes S3 answer `MalformedXML` for the WHOLE
+ * lifecycle configuration, so the legacy singular readers screen with this.
+ */
+function isPlainObject(value: unknown): value is Record<string, unknown> {
+  return typeof value === 'object' && value !== null && !Array.isArray(value);
+}
+
+/**
+ * Coerce a CFn numeric to a finite number. CloudFormation is stringly typed —
+ * a hand-written or imported template (the audience the legacy lifecycle
+ * 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;
+  const n = Number(value);
+  return Number.isFinite(n) ? n : undefined;
+}
+
+/** 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;
+  return undefined;
+}
+
 /**
  * SDK Provider for AWS::S3::Bucket
  *
@@ -398,9 +427,26 @@ export class S3BucketProvider implements ResourceProvider {
       // whose ONLY action is the delete-marker cleanup produced no `Expiration`
       // at all and S3 rejects the action-less rule (issue #1388). S3 forbids
       // combining it with Days / Date, so it only fills an empty Expiration.
-      const ruleLevelDeleteMarker = rule['ExpiredObjectDeleteMarker'];
-      if (typeof ruleLevelDeleteMarker === 'boolean' && sdkRule.Expiration === undefined) {
-        sdkRule.Expiration = { ExpiredObjectDeleteMarker: ruleLevelDeleteMarker };
+      // Gate on whether the built `Expiration` actually carries a Days / Date
+      // rather than on its mere existence: the nested branch above emits an
+      // 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.
+      const ruleLevelDeleteMarker = coerceCfnBoolean(rule['ExpiredObjectDeleteMarker']);
+      if (ruleLevelDeleteMarker !== undefined) {
+        const exp = sdkRule.Expiration as Record<string, unknown> | undefined;
+        const hasDaysOrDate = exp?.['Days'] !== undefined || exp?.['Date'] !== undefined;
+        if (!hasDaysOrDate) {
+          sdkRule.Expiration = { ExpiredObjectDeleteMarker: ruleLevelDeleteMarker };
+        } else {
+          // S3 rejects ExpiredObjectDeleteMarker combined with Days / Date, so
+          // one of the two has to go. Warn instead of dropping in silence.
+          this.logger.warn(
+            `Lifecycle rule '${(rule['Id'] as string) ?? '<unnamed>'}' on ${bucketName} sets ` +
+              `ExpiredObjectDeleteMarker alongside an expiration Days/Date; S3 forbids ` +
+              `combining them, so the delete-marker cleanup was not applied.`
+          );
+        }
       }
 
       // NoncurrentVersionExpiration. The CFn schema ALSO still accepts the
@@ -413,8 +459,11 @@ export class S3BucketProvider implements ResourceProvider {
           NoncurrentDays: nve['NoncurrentDays'] as number | undefined,
           NewerNoncurrentVersions: nve['NewerNoncurrentVersions'] as number | undefined,
         };
-      } else if (typeof legacyNveDays === 'number') {
-        sdkRule.NoncurrentVersionExpiration = { NoncurrentDays: legacyNveDays };
+      } else if (coerceCfnNumber(legacyNveDays) !== undefined) {
+        // Coerced, not `typeof === 'number'`: CFn is stringly typed and this
+        // branch exists specifically for hand-written / imported templates,
+        // which is exactly where `"365"` shows up.
+        sdkRule.NoncurrentVersionExpiration = { NoncurrentDays: coerceCfnNumber(legacyNveDays) };
       }
 
       // NoncurrentVersionTransitions, plus the legacy singular
@@ -440,10 +489,19 @@ export class S3BucketProvider implements ResourceProvider {
       const singularNvt = rule['NoncurrentVersionTransition'] as
         | Record<string, unknown>
         | undefined;
-      const allNvts = [
-        ...(Array.isArray(nvts) ? nvts : []),
-        ...(singularNvt && typeof singularNvt === 'object' ? [singularNvt] : []),
-      ];
+      // 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]
+            : [];
       if (allNvts.length > 0) {
         sdkRule.NoncurrentVersionTransitions = allNvts.map(toSdkNvt);
       }
@@ -460,12 +518,14 @@ export class S3BucketProvider implements ResourceProvider {
       });
       const transitions = rule['Transitions'] as Array<Record<string, unknown>> | undefined;
       const singularTransition = rule['Transition'] as Record<string, unknown> | undefined;
-      const allTransitions = [
-        ...(Array.isArray(transitions) ? transitions : []),
-        ...(singularTransition && typeof singularTransition === 'object'
-          ? [singularTransition]
-          : []),
-      ];
+      // 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]
+            : [];
       if (allTransitions.length > 0) {
         sdkRule.Transitions = allTransitions.map(toSdkTransition);
       }
@@ -2073,7 +2133,15 @@ export class S3BucketProvider implements ResourceProvider {
           if (r.NoncurrentVersionTransitions && r.NoncurrentVersionTransitions.length > 0) {
             out['NoncurrentVersionTransitions'] = r.NoncurrentVersionTransitions.map((nvt) => {
               const item: Record<string, unknown> = {};
-              if (nvt.NoncurrentDays !== undefined) item['NoncurrentDays'] = nvt.NoncurrentDays;
+              // Reverse-map to the CFn spelling `TransitionInDays`, matching
+              // the `Transitions` sibling 20 lines above. Emitting the SDK's
+              // `NoncurrentDays` here made `cdkd drift` report a permanent
+              // phantom diff on every versioned bucket with a noncurrent
+              // transition, because the template baseline carries
+              // `TransitionInDays` and `Rules` is compared array-wholesale.
+              // Latent until this PR: the write side never delivered the day
+              // count, so the two sides were both empty and agreed by accident.
+              if (nvt.NoncurrentDays !== undefined) item['TransitionInDays'] = nvt.NoncurrentDays;
               if (nvt.StorageClass !== undefined) item['StorageClass'] = nvt.StorageClass;
               if (nvt.NewerNoncurrentVersions !== undefined)
                 item['NewerNoncurrentVersions'] = nvt.NewerNoncurrentVersions;

tests/integration/s3-lifecycle/verify.sh

@@ -11,11 +11,11 @@
 #   - an in-place UPDATE that shortens a transition + adds a Filter-based rule
 #
 # Phases:
-#   1. Deploy; assert both rules reached AWS, none carries a top-level Prefix
+#   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.
 #   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 3 rules, and the bucket was NOT replaced.
+#      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.
 #
 # Required env vars:

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

@@ -17,7 +17,7 @@ import { PutBucketLifecycleConfigurationCommand } from '@aws-sdk/client-s3';
  * not just legacy-template trivia.
  */
 
-const mockSend = vi.fn();
+const { mockSend, warnSpy } = vi.hoisted(() => ({ mockSend: vi.fn(), warnSpy: vi.fn() }));
 
 vi.mock('../../../src/utils/aws-clients.js', () => ({
   getAwsClients: () => ({
@@ -29,7 +29,7 @@ vi.mock('../../../src/utils/logger.js', () => {
   const childLogger = {
     debug: vi.fn(),
     info: vi.fn(),
-    warn: vi.fn(),
+    warn: warnSpy,
     error: vi.fn(),
     child: vi.fn().mockReturnThis(),
   };
@@ -38,7 +38,7 @@ vi.mock('../../../src/utils/logger.js', () => {
       child: () => childLogger,
       debug: vi.fn(),
       info: vi.fn(),
-      warn: vi.fn(),
+      warn: warnSpy,
       error: vi.fn(),
     }),
   };
@@ -255,20 +255,20 @@ describe('S3 lifecycle rule-level + legacy singular keys (issue #1388)', () => {
       expect(n.NoncurrentVersionExpiration).toEqual({ NoncurrentDays: 365 });
     });
 
-    it('concatenates the plural and singular forms when a rule carries both', async () => {
+    it('uses the singular form only when the plural array is absent', async () => {
+      // The both-present case is covered in the fix-back block below: the
+      // plural wins, because concatenating can emit two transitions with the
+      // same StorageClass and S3 rejects the whole configuration.
       const rules = await putRules([
         {
-          Id: 'mixed',
+          Id: 'singular-only',
           Status: 'Enabled',
           Prefix: 'm/',
-          Transitions: [{ StorageClass: 'STANDARD_IA', TransitionInDays: 30 }],
           Transition: { StorageClass: 'GLACIER', TransitionInDays: 90 },
         },
       ]);
-      expect(rules[0]!.Transitions).toHaveLength(2);
-      expect(rules[0]!.Transitions.map((t: { StorageClass: string }) => t.StorageClass)).toEqual([
-        'STANDARD_IA',
-        'GLACIER',
+      expect(rules[0]!.Transitions).toEqual([
+        { Days: 90, Date: undefined, StorageClass: 'GLACIER' },
       ]);
     });
 
@@ -289,6 +289,136 @@ 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.
+      const rules = await putRules([
+        {
+          Id: 'both',
+          Status: 'Enabled',
+          Prefix: 'm/',
+          Transitions: [{ StorageClass: 'GLACIER', TransitionInDays: 30 }],
+          Transition: { StorageClass: 'GLACIER', TransitionInDays: 90 },
+        },
+      ]);
+      expect(rules[0]!.Transitions).toEqual([
+        { Days: 30, Date: undefined, StorageClass: 'GLACIER' },
+      ]);
+    });
+
+    it('lets the plural NoncurrentVersionTransitions win over the singular too', async () => {
+      const rules = await putRules([
+        {
+          Id: 'both-nvt',
+          Status: 'Enabled',
+          Prefix: 'm/',
+          NoncurrentVersionTransitions: [{ StorageClass: 'GLACIER', TransitionInDays: 10 }],
+          NoncurrentVersionTransition: { StorageClass: 'GLACIER', TransitionInDays: 20 },
+        },
+      ]);
+      expect(rules[0]!.NoncurrentVersionTransitions).toHaveLength(1);
+      expect(rules[0]!.NoncurrentVersionTransitions[0].NoncurrentDays).toBe(10);
+    });
+
+    it('ignores a non-object singular Transition (array / unresolved intrinsic)', async () => {
+      // `typeof [] === 'object'`, so an unguarded check pushed it through as a
+      // transition with no StorageClass and S3 answered MalformedXML for the
+      // whole config.
+      const rules = await putRules([
+        { Id: 'bad', Status: 'Enabled', Prefix: 'b/', Transition: [], ExpirationInDays: 5 },
+      ]);
+      expect(rules[0]!.Transitions).toBeUndefined();
+    });
+
+    it('still applies the delete marker when Expiration is present but empty', async () => {
+      // The nested branch emits an all-undefined object for an empty
+      // `Expiration`, so gating on mere existence dropped the marker AND left
+      // the rule action-less — the very failure the block exists to prevent.
+      const rules = await putRules([
+        {
+          Id: 'degenerate',
+          Status: 'Enabled',
+          Prefix: 'd/',
+          Expiration: {},
+          ExpiredObjectDeleteMarker: true,
+        },
+      ]);
+      expect(rules[0]!.Expiration).toEqual({ ExpiredObjectDeleteMarker: true });
+    });
+
+    it('warns instead of silently dropping a delete marker that conflicts with Days', async () => {
+      await putRules([
+        {
+          Id: 'conflict',
+          Status: 'Enabled',
+          Prefix: 'c/',
+          ExpirationInDays: 30,
+          ExpiredObjectDeleteMarker: true,
+        },
+      ]);
+      expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('ExpiredObjectDeleteMarker'));
+    });
+
+    it('coerces stringly-typed CFn scalars from hand-written templates', async () => {
+      // CFn is stringly typed and these legacy branches exist precisely for
+      // hand-written / imported templates, where `"365"` / `"true"` show up.
+      const rules = await putRules([
+        {
+          Id: 'stringly',
+          Status: 'Enabled',
+          Prefix: 's/',
+          NoncurrentVersionExpirationInDays: '365',
+        },
+        {
+          Id: 'stringly-marker',
+          Status: 'Enabled',
+          Prefix: 'sm/',
+          ExpiredObjectDeleteMarker: 'true',
+        },
+      ]);
+      expect(rules[0]!.NoncurrentVersionExpiration).toEqual({ NoncurrentDays: 365 });
+      expect(rules[1]!.Expiration).toEqual({ ExpiredObjectDeleteMarker: true });
+    });
+
+    it('combines rule-level tags with a size constraint under And', async () => {
+      // A real synth emits tags AND ObjectSizeGreaterThan at the rule level on
+      // one rule; that hits the multi-component `And` branch.
+      const rules = await putRules([
+        {
+          Id: 'tags-and-size',
+          Status: 'Enabled',
+          TagFilters: [{ Key: 'env', Value: 'prod' }],
+          ObjectSizeGreaterThan: 1024,
+          ExpirationInDays: 30,
+        },
+      ]);
+      expect(rules[0]!.Filter.And).toMatchObject({
+        Tags: [{ Key: 'env', Value: 'prod' }],
+        ObjectSizeGreaterThan: 1024,
+      });
+    });
+
+    it('applies the delete marker alongside an ExpirationDate-free rule', async () => {
+      const rules = await putRules([
+        {
+          Id: 'date-marker',
+          Status: 'Enabled',
+          Prefix: 'dm/',
+          ExpirationDate: '2030-01-01T00:00:00Z',
+          ExpiredObjectDeleteMarker: true,
+        },
+      ]);
+      // Date is set, so the marker must NOT be smuggled in beside it.
+      expect(rules[0]!.Expiration.Date).toBeInstanceOf(Date);
+      expect(rules[0]!.Expiration.ExpiredObjectDeleteMarker).toBeUndefined();
+    });
+  });
+
   describe('no regression on the shapes that already worked', () => {
     it('keeps a prefix-only config in V1 form (bare top-level Prefix, no Filter)', async () => {
       const rules = await putRules([