feat(lib): canonical asset hashes behind the canonicalAssetHashes feature flag - #323
Conversation
sakul-learning
left a comment
There was a problem hiding this comment.
Two emitted-artifact/hash consistency issues need resolution; details are inline. I’ll post the configured-check and test-gap summary separately.
Testing and coverage evaluationAll configured checks passed at
The canonical hash suite runs through the normal Nx/Jest target and provides useful behavioral coverage for renames, entry-boundary shifts, permission changes, empty directories, file/symlink distinctions, symlink retargeting, and deterministic trees. Two output-level gaps remain and correspond to the inline correctness findings:
Ponytail/artifact-value pass: the core framing tests earn their maintenance cost and run in normal CI. Some canonical symlink cases overlap the stacked #321 coverage, but they still exercise a separate implementation. I would prioritize the two missing emitted-artifact regressions over adding further helper-level cases. No workflow files changed, and I found no new security issue in the touched code. |
### Description Closes #320. `TerraformAsset` walked source trees with `fs.statSync`, which follows symlinks. On symlinked trees (e.g. pnpm's `node_modules`) this meant: - **Archive bloat**: a target reachable through N symlink paths was copied N times into the zip (15 MB → 34 MB on a real Lambda bundle for byte-identical logical content). - **No symlink fidelity**: archives contained zero symlink entries — links became full copies. - **Hard synth crash**: circular symlinks (legitimate in pnpm trees) hit `ELOOP`. Notably this crashed for **every** asset type, not just `ARCHIVE`, because `hashPath` runs in the `TerraformAsset` constructor. This is a fork regression, not inherited from cdktf: upstream archives with `archiver`, which walks with `lstat` and emits real symlink entries (cycles are structurally impossible there). The regression entered when `archiver` was replaced by yazl in #95 — yazl has no symlink awareness at all (`addFile` stats-and-follows) — and survived the yazl → fflate rewrite in #148, which kept the hand-rolled `statSync` walk. ### The fix All three walkers in `packages/cdktn/src/private/fs.ts` now use `lstatSync` and treat symlinks first-class, restoring `archiver`/cdktf parity: - **`archiveSync`** emits real symlink entries: the `readlink` target as STORED entry data with `S_IFLNK | perms` in the unix external attributes and version-made-by host = Unix — exactly what `archiver` produces and what Info-ZIP `unzip`, Go `archive/zip`, and the AWS Lambda runtime (verified live on `nodejs22.x` in #320) recreate as symlinks. Never recursing through links makes cycles unreachable by construction. fflate 0.8.2 supports this natively via per-file `[data, { os, attrs, level }]` tuples (attrs must be caller-pre-shifted, `(mode << 16) >>> 0`). - **`hashPath`** hashes a symlink by its target path instead of following it — retargeting a link still changes the asset hash, but shared targets are no longer double-counted and cycles no longer crash the constructor. - **`copySync`** (`AssetType.DIRECTORY`) recreates symlinks with `fs.symlinkSync`. Two latent zip-metadata gaps fixed along the way (both also `archiver`/cdktf parity): - Regular-file unix modes are now preserved (executable bits were silently stripped — matters for Lambda binaries and `.bin` scripts). - Entry mtimes are pinned to a fixed 1980 date, making archives byte-reproducible across synths (fflate defaults `mtime` to `Date.now()`, so every synth previously produced different zip bytes — perpetual drift for anyone hashing the archive, e.g. `filebase64sha256`). The pin uses the local-time `Date` constructor deliberately: fflate encodes DOS dates from local getters and rejects years < 1980, so a UTC midnight date would underflow to 1979 in timezones west of UTC. Prior art considered and rejected: `terraform-provider-archive` always dereferences and has no cycle detection — it accumulated the opt-in `exclude_symlink_directories` band-aid (v2.4.0, hashicorp/terraform-provider-archive#183; follow-up defect fixed in v2.4.2, #298) and still `ELOOP`s on cycles. Reverting to `archiver` would reintroduce the `execSync` child-process bridge that #148 removed (its API is async-only). ### Hash compatibility (updated after review — downscoped) Review surfaced a domain collision in the first cut of the `hashPath` change (file bytes and symlink-target bytes shared one unframed stream). Per the requested downscope this PR now carries only the compatibility-preserving resolution (reviewer's option 3): - the legacy content hash is kept byte-identical for symlink-free trees (proven by a test that recomputes the historical md5 independently); - symlink metadata (relative path + target) is framed into a separate digest and combined under a tagged outer hash **only when symlinks exist** — so hashes still change only for symlink-bearing trees, which today either bloat 2-3× or crash outright. The canonical entry-framed scheme and its `canonicalAssetHashes` feature flag moved to the stack: design issue #322 → implementation #323 → docs #324. `TerraformModuleAsset` stays in scope per review: it shares the corrected copier (its private copier sent directory symlinks into `copyFileSync` → `EISDIR`) with a regression test. ### Testing - `packages/cdktn/test/archive-symlink.test.ts`: 10 tests — the issue's repros (file/dir symlink preservation via system-`unzip` round-trip + `lstat`, dedup of shared targets, `ELOOP` on cycles for both `archiveSync` and `hashPath`, hash-changes-on-retarget), `copySync` symlink recreation, executable-bit preservation, and byte-identical archives across runs. 5 of the 6 core repros fail on `main`. - Full `cdktn` package suite green (453 passed; the one pre-existing `matchers.test.ts` → `toPlanSuccessfully` failure shells out to a real `terraform plan` and fails identically without this change). - Issue #320's literal repro script against the built lib: `zipinfo` shows `lrwxr-xr-x ... stor` entries for `link-a`/`link-b`, one payload copy (538-byte zip vs 3× 200 KiB before), and the cyclic tree archives to a 120-byte zip instead of aborting synth. ### Checklist - [x] I have updated the PR title to match [CDKTN's style guide](https://github.com/open-constructs/cdk-terrain/blob/main/CONTRIBUTING.md#pull-requests-1) - [x] I have run the linter on my code locally - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation if applicable - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works if applicable - [x] New and existing unit tests pass locally with my changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
9d97256 to
11bc329
Compare
sakul-learning
left a comment
There was a problem hiding this comment.
The rebase onto merged #321 leaves both previously identified emitted-artifact/hash consistency blockers unresolved. Reposting them on the current diff.
…type Addresses the two blocking review findings on open-constructs#323: - ARCHIVE assets: archiveSync never emits ZIP directory entries, so canonical hashing now omits directory records when framing for an archive (HashPathOptions.archive). Empty directories no longer churn the archive hash/path while ZIP bytes stay identical; regressions correlate hash changes with emitted ZIP bytes in both directions. - TerraformModuleAsset: the emitted asset is only the module sources copied into tmpDir, so the canonical scheme hashes that exact tree instead of the sources' lowest common ancestor. Unrelated siblings under the ancestor can no longer affect the hash; legacy keeps hashing the ancestor to preserve historical hashes.
Introduces the canonical entry-framed asset hash from open-constructs#322 as an opt-in FUTURE_FLAGS entry (on for new projects, opt-in for existing ones, default at the next major). The scheme is modeled on git trees and Nix NAR and frames everything that affects the emitted artifact: - files: 'F <mode> <relPath>NUL<size>NUL' + content, where mode is the octal permission mask archiveSync preserves in zip external attributes - symlinks: 'L <mode> <relPath>NUL<target length>NUL' + target - directories: 'D <relPath>NUL' including empty ones (copySync materializes them); no mode, since neither emitter preserves directory permissions Entries are framed in sorted order with /-separated relative paths, so renames, entry-boundary shifts, permission changes, empty-directory changes, file-vs-symlink swaps, and symlink retargeting all change the hash -- closing the metadata hole identified in review (0644 -> 0755 previously changed archive bytes but not the hash). The legacy scheme remains the default for existing projects and is untouched by the flag. Closes open-constructs#322 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…type Addresses the two blocking review findings on open-constructs#323: - ARCHIVE assets: archiveSync never emits ZIP directory entries, so canonical hashing now omits directory records when framing for an archive (HashPathOptions.archive). Empty directories no longer churn the archive hash/path while ZIP bytes stay identical; regressions correlate hash changes with emitted ZIP bytes in both directions. - TerraformModuleAsset: the emitted asset is only the module sources copied into tmpDir, so the canonical scheme hashes that exact tree instead of the sources' lowest common ancestor. Unrelated siblings under the ancestor can no longer affect the hash; legacy keeps hashing the ancestor to preserve historical hashes.
554fcbe to
da4bb1d
Compare
… hash order canonicalHashPath walks directories sorted but archiveSync inserted zip entries in filesystem enumeration order, so two logically identical trees could share a canonical archive hash while emitting differently ordered ZIP byte streams (e.g. across filesystems or create/delete histories). Sorting the traversal makes archive emission deterministic and aligned with the canonical walk, completing the byte-reproducible intent of the pinned entry mtime; no hash changes, legacy included. Regressions: equivalent trees built with different creation histories emit identical bytes and hashes, and a reversed-enumeration simulation leaves the emitted bytes and hash unchanged.
sakul-learning
left a comment
There was a problem hiding this comment.
All previously requested changes are resolved at c79fc67.
- Canonical archive hashes now omit directory records that ZIP emission omits.
- Canonical TerraformModuleAsset hashing uses the staged emitted tree.
- archiveSync traversal is sorted to match canonical hash ordering, with deterministic reversed-enumeration coverage.
- Configured validation, build, and full cdktn test checks pass (40 suites, 473 tests, 288 snapshots).
Approved.
🤖 Release PR — merge to cut a new release. Kept open and rebased as commits land on `main`. --- <details><summary>0.24.0</summary> ## [0.24.0](v0.23.4...v0.24.0) (2026-08-06) ### ⚠ BREAKING CHANGES * **lib:** validate Terraform function versions by default ([#362](#362)) * **deps:** Require Node 22 minimum ([#345](#345)) * **lib:** preserve symlinks in TerraformAsset walkers ([#321](#321)) * **cli:** replace node-fetch with undici ([#306](#306)) ### Features * **lib:** canonical asset hashes behind the canonicalAssetHashes feature flag ([#323](#323)) ([76dd4ff](76dd4ff)) * **lib:** validate Terraform function versions by default ([#362](#362)) ([4ac0736](4ac0736)) * support newer provider plugin-protocol features via targetVersions (RFC-04) ([#296](#296)) ([90322f9](90322f9)) ### Bug Fixes * **cli:** don't downgrade prebuilt providers on a transient registry failure ([#298](#298)) ([a960c5c](a960c5c)) * **cli:** include dev dependencies in npm version lookup ([#280](#280)) ([955204a](955204a)) * **docs:** fix stale constructs pin breaking with()/IMixin docs ([#305](#305)) ([605cf63](605cf63)) * **gha:** Allow pnpm to update the lockfile after package updates ([#318](#318)) ([a899b7c](a899b7c)) * **gha:** Fix pnpm upgrade workflow ([#335](#335)) ([e1a69fc](e1a69fc)) * **gha:** flip merged release PR label to autorelease: tagged ([#302](#302)) ([8d64f6c](8d64f6c)) * **gha:** mint the Go-publish token from the CDKTN Maintainers app ([#369](#369)) ([71921ce](71921ce)) * **gha:** mint the Go-publish token from the open-constructs-cdktn App ([#368](#368)) ([179f10c](179f10c)) * **gha:** pass the Go-publish App token as x-access-token userinfo ([#370](#370)) ([4e3ff19](4e3ff19)) * **lib:** Disallow constructs 10.8 until support can be added ([#363](#363)) ([8bdae0d](8bdae0d)) * **lib:** preserve symlinks in TerraformAsset walkers ([#321](#321)) ([6360e20](6360e20)) * typo in `moveFromId` JSDoc ([#355](#355)) ([e1cf8ce](e1cf8ce)) ### Miscellaneous Chores * **cli:** replace Ink + React with smaller-tree CLI libraries ([#264](#264)) ([a6aff7e](a6aff7e)) * **cli:** replace node-fetch with undici ([#306](#306)) ([1317141](1317141)) * **deps:** bump glob to 13.0.6 ([#307](#307)) ([47ee2bb](47ee2bb)) * **deps:** bump the github-actions-backward-compatible group with 2 updates ([#295](#295)) ([eab2a01](eab2a01)) * **deps:** replace lerna with nx ([#315](#315)) ([94999fc](94999fc)) * **deps:** Require Node 22 minimum ([#345](#345)) ([2bf315d](2bf315d)) * **deps:** update ci-info to 4.4.0 across all packages ([#329](#329)) ([557a163](557a163)) * **deps:** update fs-extra to 11.3.6 across all packages ([#328](#328)) ([95753a9](95753a9)) * **deps:** update minimatch to 10.2.5 ([#330](#330)) ([d1c1c53](d1c1c53)) * **deps:** Update sscaff to v2.0.388 ([#331](#331)) ([38ea0ba](38ea0ba)) * **deps:** Update zod to v4.4.3 ([#332](#332)) ([c10ee35](c10ee35)) * **deps:** Upgrade dependencies ([#347](#347)) ([45d3a66](45d3a66)) * remove cdktf from tests ([#277](#277)) ([dc9a8e9](dc9a8e9)) * ship Terraform 1.15.8 in the jsii-terraform image ([#367](#367)) ([deaa9b0](deaa9b0)) * Upgrade dependencies for lib ([#348](#348)) ([d616f17](d616f17)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: open-constructs-cdktn[bot] <291052431+open-constructs-cdktn[bot]@users.noreply.github.com>
…ture flag (open-constructs#323) ### Description Closes open-constructs#322. ~~Stacked on open-constructs#321~~ — open-constructs#321 has merged and this branch is rebased onto current `main`; the diff is now the single canonical-hash commit. Adds the canonical entry-framed asset hash as a `canonicalAssetHashes` feature flag (`FUTURE_FLAGS`: enabled for new projects by `cdktn init`, opt-in via `cdktf.json` context for existing projects, becomes the default at the next major). The legacy scheme — including open-constructs#321's compatibility-preserving symlink handling — stays untouched as the default. ### The canonical representation Modeled on git trees and Nix NAR, which serialize a full metadata model rather than payload framing alone. Every record contains what affects the emitted artifact: | entry | frame | | --- | --- | | file | `F <mode> <relPath>\0<size>\0` + content | | symlink | `L <mode> <relPath>\0<target byte length>\0` + target | | directory (incl. empty) | `D <relPath>\0` | - `<mode>` is the octal permission mask (`0o7777` bits) — exactly the bits `archiveSync` preserves in zip external attributes, closing the hole found in review where `0644` → `0755` changed archive bytes but not the hash. - Directories contribute explicit records (no mode — neither `archiveSync` nor `copySync` preserves directory permissions), so adding/removing an empty directory is visible. - Entries are framed in sorted directory order with `/`-separated relative paths; a symlink at the root is followed, matching how the asset source path is opened when the artifact is emitted. ### Acceptance criteria from open-constructs#322 Each has a dedicated test in `packages/cdktn/test/canonical-asset-hash.test.ts`: - [x] File rename changes the canonical hash (and a paired assertion that legacy cannot see it) - [x] Entry-boundary shifts change the canonical hash - [x] File/symlink swaps and symlink retargeting change the canonical hash - [x] `0644` → `0755` changes the canonical hash - [x] Adding/removing an empty directory changes the canonical hash - [x] Identical logical trees hash deterministically (two locations + repeated runs) - [x] Legacy hashing remains available (flag off; `Testing.app({ enableFutureFlags: false })` covers the compat case per the `features.ts` test rule) ### Testing Full `cdktn` suite green (464 passing; the pre-existing environmental `matchers.test.ts` → `toPlanSuccessfully` failure shells out to a real `terraform plan` and is unrelated). jsii build green. Docs for the flag are being handled separately in the docs repository (the stacked docs PR open-constructs#324 was closed in favor of that). ### Checklist - [x] I have updated the PR title to match [CDKTN's style guide](https://github.com/open-constructs/cdk-terrain/blob/main/CONTRIBUTING.md#pull-requests-1) - [x] I have run the linter on my code locally - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation if applicable (stacked docs PR follows) - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works if applicable - [x] New and existing unit tests pass locally with my changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: vincent de smet <vincent.drl@gmail.com>
🤖 Release PR — merge to cut a new release. Kept open and rebased as commits land on `main`. --- <details><summary>0.24.0</summary> ## [0.24.0](open-constructs/cdk-terrain@v0.23.4...v0.24.0) (2026-08-06) ### ⚠ BREAKING CHANGES * **lib:** validate Terraform function versions by default ([open-constructs#362](open-constructs#362)) * **deps:** Require Node 22 minimum ([open-constructs#345](open-constructs#345)) * **lib:** preserve symlinks in TerraformAsset walkers ([open-constructs#321](open-constructs#321)) * **cli:** replace node-fetch with undici ([open-constructs#306](open-constructs#306)) ### Features * **lib:** canonical asset hashes behind the canonicalAssetHashes feature flag ([open-constructs#323](open-constructs#323)) ([76dd4ff](open-constructs@76dd4ff)) * **lib:** validate Terraform function versions by default ([open-constructs#362](open-constructs#362)) ([4ac0736](open-constructs@4ac0736)) * support newer provider plugin-protocol features via targetVersions (RFC-04) ([open-constructs#296](open-constructs#296)) ([90322f9](open-constructs@90322f9)) ### Bug Fixes * **cli:** don't downgrade prebuilt providers on a transient registry failure ([open-constructs#298](open-constructs#298)) ([a960c5c](open-constructs@a960c5c)) * **cli:** include dev dependencies in npm version lookup ([open-constructs#280](open-constructs#280)) ([955204a](open-constructs@955204a)) * **docs:** fix stale constructs pin breaking with()/IMixin docs ([open-constructs#305](open-constructs#305)) ([605cf63](open-constructs@605cf63)) * **gha:** Allow pnpm to update the lockfile after package updates ([open-constructs#318](open-constructs#318)) ([a899b7c](open-constructs@a899b7c)) * **gha:** Fix pnpm upgrade workflow ([open-constructs#335](open-constructs#335)) ([e1a69fc](open-constructs@e1a69fc)) * **gha:** flip merged release PR label to autorelease: tagged ([open-constructs#302](open-constructs#302)) ([8d64f6c](open-constructs@8d64f6c)) * **gha:** mint the Go-publish token from the CDKTN Maintainers app ([open-constructs#369](open-constructs#369)) ([71921ce](open-constructs@71921ce)) * **gha:** mint the Go-publish token from the open-constructs-cdktn App ([open-constructs#368](open-constructs#368)) ([179f10c](open-constructs@179f10c)) * **gha:** pass the Go-publish App token as x-access-token userinfo ([open-constructs#370](open-constructs#370)) ([4e3ff19](open-constructs@4e3ff19)) * **lib:** Disallow constructs 10.8 until support can be added ([open-constructs#363](open-constructs#363)) ([8bdae0d](open-constructs@8bdae0d)) * **lib:** preserve symlinks in TerraformAsset walkers ([open-constructs#321](open-constructs#321)) ([6360e20](open-constructs@6360e20)) * typo in `moveFromId` JSDoc ([open-constructs#355](open-constructs#355)) ([e1cf8ce](open-constructs@e1cf8ce)) ### Miscellaneous Chores * **cli:** replace Ink + React with smaller-tree CLI libraries ([open-constructs#264](open-constructs#264)) ([a6aff7e](open-constructs@a6aff7e)) * **cli:** replace node-fetch with undici ([open-constructs#306](open-constructs#306)) ([1317141](open-constructs@1317141)) * **deps:** bump glob to 13.0.6 ([open-constructs#307](open-constructs#307)) ([47ee2bb](open-constructs@47ee2bb)) * **deps:** bump the github-actions-backward-compatible group with 2 updates ([open-constructs#295](open-constructs#295)) ([eab2a01](open-constructs@eab2a01)) * **deps:** replace lerna with nx ([open-constructs#315](open-constructs#315)) ([94999fc](open-constructs@94999fc)) * **deps:** Require Node 22 minimum ([open-constructs#345](open-constructs#345)) ([2bf315d](open-constructs@2bf315d)) * **deps:** update ci-info to 4.4.0 across all packages ([open-constructs#329](open-constructs#329)) ([557a163](open-constructs@557a163)) * **deps:** update fs-extra to 11.3.6 across all packages ([open-constructs#328](open-constructs#328)) ([95753a9](open-constructs@95753a9)) * **deps:** update minimatch to 10.2.5 ([open-constructs#330](open-constructs#330)) ([d1c1c53](open-constructs@d1c1c53)) * **deps:** Update sscaff to v2.0.388 ([open-constructs#331](open-constructs#331)) ([38ea0ba](open-constructs@38ea0ba)) * **deps:** Update zod to v4.4.3 ([open-constructs#332](open-constructs#332)) ([c10ee35](open-constructs@c10ee35)) * **deps:** Upgrade dependencies ([open-constructs#347](open-constructs#347)) ([45d3a66](open-constructs@45d3a66)) * remove cdktf from tests ([open-constructs#277](open-constructs#277)) ([dc9a8e9](open-constructs@dc9a8e9)) * ship Terraform 1.15.8 in the jsii-terraform image ([open-constructs#367](open-constructs#367)) ([deaa9b0](open-constructs@deaa9b0)) * Upgrade dependencies for lib ([open-constructs#348](open-constructs#348)) ([d616f17](open-constructs@d616f17)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: open-constructs-cdktn[bot] <291052431+open-constructs-cdktn[bot]@users.noreply.github.com>
Description
Closes #322.
Stacked on #321— #321 has merged and this branch is rebased onto currentmain; the diff is now the single canonical-hash commit.Adds the canonical entry-framed asset hash as a
canonicalAssetHashesfeature flag (FUTURE_FLAGS: enabled for new projects bycdktn init, opt-in viacdktf.jsoncontext for existing projects, becomes the default at the next major). The legacy scheme — including #321's compatibility-preserving symlink handling — stays untouched as the default.The canonical representation
Modeled on git trees and Nix NAR, which serialize a full metadata model rather than payload framing alone. Every record contains what affects the emitted artifact:
F <mode> <relPath>\0<size>\0+ contentL <mode> <relPath>\0<target byte length>\0+ targetD <relPath>\0<mode>is the octal permission mask (0o7777bits) — exactly the bitsarchiveSyncpreserves in zip external attributes, closing the hole found in review where0644→0755changed archive bytes but not the hash.archiveSyncnorcopySyncpreserves directory permissions), so adding/removing an empty directory is visible./-separated relative paths; a symlink at the root is followed, matching how the asset source path is opened when the artifact is emitted.Acceptance criteria from #322
Each has a dedicated test in
packages/cdktn/test/canonical-asset-hash.test.ts:0644→0755changes the canonical hashTesting.app({ enableFutureFlags: false })covers the compat case per thefeatures.tstest rule)Testing
Full
cdktnsuite green (464 passing; the pre-existing environmentalmatchers.test.ts→toPlanSuccessfullyfailure shells out to a realterraform planand is unrelated). jsii build green. Docs for the flag are being handled separately in the docs repository (the stacked docs PR #324 was closed in favor of that).Checklist
🤖 Generated with Claude Code