feat(lib): canonical asset hashes behind the canonicalAssetHashes feature flag by vincenthsh · Pull Request #323 · open-constructs/cdk-terrain

vincenthsh · 2026-07-11T09:26:16Z

Description

Closes #322. ~~Stacked on #321~~ — #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 #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 #322

Each has a dedicated test in packages/cdktn/test/canonical-asset-hash.test.ts:

File rename changes the canonical hash (and a paired assertion that legacy cannot see it)
Entry-boundary shifts change the canonical hash
File/symlink swaps and symlink retargeting change the canonical hash
0644 → 0755 changes the canonical hash
Adding/removing an empty directory changes the canonical hash
Identical logical trees hash deterministically (two locations + repeated runs)
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 #324 was closed in favor of that).

Checklist

I have updated the PR title to match CDKTN's style guide
I have run the linter on my code locally
I have performed a self-review of my code
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)
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works if applicable
New and existing unit tests pass locally with my changes

🤖 Generated with Claude Code

sakul-learning

Two emitted-artifact/hash consistency issues need resolution; details are inline. I’ll post the configured-check and test-gap summary separately.

sakul-learning · 2026-07-11T10:41:27Z

+ *               copySync preserves directory permissions,
+ *
+ * in sorted directory order with `/`-separated relative paths, so renames,
+ * entry-boundary shifts, permission changes, empty-directory changes, and


Blocking: Canonical hashing records every non-root directory, including empty ones, but archiveSync() only recurses into directories and never emits ZIP directory entries. For AssetType.ARCHIVE, adding an empty directory therefore changes assetHash and the synthesized asset path while producing identical ZIP bytes. That conflicts with #322’s criterion limiting this sensitivity to assets where the directory is emitted. Please either emit explicit directory entries in archives or make canonical hashing asset-type-aware and omit directory records for archives that do not emit them; an archive-level regression should prove the hash tracks the emitted artifact.

sakul-learning · 2026-07-11T10:41:27Z

-      assetHash: staticModuleAssetHash ?? hashPath(relativeAssetPath),
+      assetHash:
+        staticModuleAssetHash ??
+        hashPath(relativeAssetPath, {


Blocking: The emitted module asset contains only moduleSources copied into tmpDir, but this hashes their lowest common source ancestor. Unrelated siblings under that ancestor can affect the automatic hash without appearing in the emitted asset; canonical path/mode/directory framing adds new churn from unrelated renames, mode changes, and empty directories. Please hash tmpDir after copying (or frame exactly the selected module sources), and add a regression proving unrelated siblings do not change the module asset hash.

sakul-learning · 2026-07-11T10:41:36Z

Testing and coverage evaluation

All configured checks passed at 9d97256b344349da0a8218cb369c83e0892f0625:

pnpm exec jest packages/cdktn/test/validations.test.ts --runInBand — 1 suite, 31 tests, 9 snapshots passed.
pnpm exec nx build cdktn — passed with 0 errors and 0 warnings.
pnpm exec nx test cdktn --runInBand — 40 suites, 465 tests, and 288 snapshots passed.

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:

The empty-directory test covers copySync/directory semantics but not AssetType.ARCHIVE, where archiveSync currently omits directory entries. An archive regression should establish that hash changes correspond to emitted ZIP changes.
The feature integration test covers TerraformAsset, but not TerraformModuleAsset, whose emitted tree is assembled separately in tmpDir. A module test should prove that only selected/copied module sources affect the automatic hash and that unrelated siblings do not cause asset-path churn.

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>

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>

sakul-learning

The rebase onto merged #321 leaves both previously identified emitted-artifact/hash consistency blockers unresolved. Reposting them on the current diff.

sakul-learning · 2026-07-12T23:57:44Z

+      hash.update(data);
+    } else if (stat.isDirectory()) {
+      if (relPath) {
+        hash.update(`D ${relPath}\0`);


Blocking: This canonical directory record applies to AssetType.ARCHIVE too, but archiveSync() only recurses into directories and never emits ZIP directory entries. Adding an empty directory therefore changes the archive’s assetHash and synthesized path while leaving the emitted ZIP unchanged. This conflicts with #322’s criterion limiting empty-directory sensitivity to assets where the directory is emitted. Please either emit explicit directory entries in archives or make canonical hashing asset-type-aware and omit directory records for archives that do not emit them; add an archive-level regression that correlates the hash with emitted ZIP content.

sakul-learning · 2026-07-12T23:57:44Z

-      assetHash: staticModuleAssetHash ?? hashPath(relativeAssetPath),
+      assetHash:
+        staticModuleAssetHash ??
+        hashPath(relativeAssetPath, {


Blocking: The emitted module asset contains only moduleSources copied into tmpDir, but this hashes their entire lowest common source ancestor. Unrelated siblings can affect the automatic hash without appearing in the emitted asset; canonical path/mode/directory framing additionally makes unrelated renames, mode changes, and empty directories cause asset-path churn. Please hash tmpDir after copying (or frame exactly the selected module sources), and add a regression proving unrelated siblings do not change the module asset hash.

vincenthsh requested a review from a team as a code owner July 11, 2026 09:26

This was referenced Jul 11, 2026

chore(docs): document feature flags and the canonicalAssetHashes scheme #324

Closed

fix(lib)!: preserve symlinks in TerraformAsset walkers #321

Merged

sakul-learning suggested changes Jul 11, 2026

View reviewed changes

jsteinich approved these changes Jul 12, 2026

View reviewed changes

vincenthsh force-pushed the feat/322-canonical-asset-hashes branch from 9d97256 to 11bc329 Compare July 12, 2026 23:47

sakul-learning suggested changes Jul 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(lib): canonical asset hashes behind the canonicalAssetHashes feature flag#323

feat(lib): canonical asset hashes behind the canonicalAssetHashes feature flag#323
vincenthsh wants to merge 1 commit into
open-constructs:mainfrom
vincenthsh:feat/322-canonical-asset-hashes

vincenthsh commented Jul 11, 2026 •

edited

Loading

Uh oh!

sakul-learning left a comment

Uh oh!

sakul-learning Jul 11, 2026

Uh oh!

sakul-learning Jul 11, 2026

Uh oh!

sakul-learning commented Jul 11, 2026

Uh oh!

sakul-learning left a comment

Uh oh!

sakul-learning Jul 12, 2026

Uh oh!

sakul-learning Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

vincenthsh commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

The canonical representation

Acceptance criteria from #322

Testing

Checklist

Uh oh!

sakul-learning left a comment

Choose a reason for hiding this comment

Uh oh!

sakul-learning Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

sakul-learning Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

sakul-learning commented Jul 11, 2026

Testing and coverage evaluation

Uh oh!

sakul-learning left a comment

Choose a reason for hiding this comment

Uh oh!

sakul-learning Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

sakul-learning Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vincenthsh commented Jul 11, 2026 •

edited

Loading