fix: byte-stable, non-destructive generate (#674) - #802
Conversation
A second generate with unchanged inputs now rewrites nothing: every generated file keeps its bytes, inode and mtime, and .labelle/deps/ is no longer wiped. - write_if_changed: generated-file writes (scanner.writeFile, mirrored copies, app icon, params/build-hook staging, constants/i18n/flow modules, asset copies) compare before writing. - deps_sync: staged packages are reconciled in place (hardlink identity or equal bytes = kept; changed = relinked; orphans swept). Tool-made zig-pkg/.zig-cache in a stage and link overlays (native scripting) survive. Top-level prune keeps the tests target's backend. - rewriteZonPaths reads the source zon and writes only on change, so it is idempotent and runs on both passes. - pack prefab/hook rewrites materialize from source instead of copy-then-rewrite. - generated build.zig.zon minimum_zig_version comes from the assembler's own build.zig.zon (was hardcoded 0.15.2); fingerprint rule is tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (19)
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe generator now derives the generated manifest’s minimum Zig version from package build options. Dependency staging reconciles existing directories instead of recreating them. Generated and mirrored files use conditional writes, and new tests check staging and output stability. ChangesGeneration Stability
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant BuildZigZonGenerator
participant createDepsLinks
participant syncTree
participant Filesystem
BuildZigZonGenerator->>createDepsLinks: pass prune and keep options
createDepsLinks->>syncTree: reconcile each staged package
syncTree->>Filesystem: stage, retain, or remove entries
syncTree-->>createDepsLinks: return sync results or errors
Merge Risk: ⚪ Minimal · up to No identified issue blocks merging after normal checks. The reported 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation Issue Resolution Make all remaining generated
✨ Finishing Touches🧪 Generate unit tests (beta)
A rabbit checks each file in place Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62bd41e037
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Never wiped (#674): each package below is reconciled in place, and | ||
| // only top-level entries no pass stages are swept (`prune`). | ||
| try cwd.createDirPath(io, deps_dir); | ||
| if (opts.prune) try pruneStaleDeps(allocator, deps_dir, deps.items, opts.keep); |
There was a problem hiding this comment.
When a previously staged local plugin or backend is deleted or moved, pruneStaleDeps treats it as live because it remains in deps.items, and the later missing-source branch merely warns and skips synchronization. The generated zon therefore continues pointing at the old .labelle/deps/<name> tree, allowing builds to silently use stale package code instead of surfacing the intended missing-package error. Remove that destination when its source fails the existence check, or exclude missing sources from the prune keep-set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b7665d, with a regression test (deps_sync.zig / generate_stability_tests.zig).
| if (isLink(dest_sub)) { | ||
| // Overlay placed by another phase — keep, never descend. | ||
| stats.kept += 1; | ||
| continue; |
There was a problem hiding this comment.
When a native-language project has already overlaid the plugin's placeholder directory and the user subsequently removes scripts/ or all native source files, this unconditional link preservation keeps the old overlay. stageNativeSources returns early for exactly those zero-source cases, so nothing replaces it: a deleted directory leaves a broken link, while a now Zig-only directory remains mounted without the required native module root and breaks the plugin's native build. Preserve the overlay only while the current generation still requires native source staging, or explicitly restore the source placeholder on the no-op path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b7665d, with a regression test (deps_sync.zig / generate_stability_tests.zig).
| if (ss.inode == ds.inode and ss.mtime.nanoseconds == ds.mtime.nanoseconds) return true; | ||
| return write_if_changed.sameFiles(io, cwd, src, cwd, dest); |
There was a problem hiding this comment.
On POSIX systems where hardlinking falls back to copying, this byte-only equality check treats files as current even when their permission bits changed. For example, updating a plugin version solely to mark a packaged build helper executable leaves the staged copy non-executable because its bytes still match; the inverse can also retain permissions that were intentionally removed. Compare the relevant permissions before returning true so a mode-only source change refreshes the staged file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b7665d, with a regression test (deps_sync.zig / generate_stability_tests.zig).
| if (ss.inode == ds.inode and ss.mtime.nanoseconds == ds.mtime.nanoseconds) return true; | ||
| return write_if_changed.sameFiles(io, cwd, src, cwd, dest); |
There was a problem hiding this comment.
When a dependency pin is switched to a different source tree whose files initially have identical bytes—common when moving between local checkouts or forks—this returns true and leaves each staged file hardlinked to the old tree. Subsequent edits to the newly selected local dependency do not reach .labelle/deps or wake the long-lived build watcher, while edits to the old checkout can unexpectedly alter the staged package. Byte equality is sufficient only for a known copy fallback; a hardlink-backed destination must be relinked when it is not linked to the currently resolved source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b7665d, with a regression test (deps_sync.zig / generate_stability_tests.zig).
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…perms (#674) - A staged package whose source vanished is deleted (the zon still points there; a stale tree must not build silently). - A link overlay over a source dir is unlinked and the dir restored; stageNativeSources re-places it while native sources exist, so the placeholder comes back when they are gone. - Byte equality only keeps a standalone copy (nlink == 1) with equal permissions; a stage hardlinked to another tree is relinked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b7665d0b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ds.kind != .file) return false; | ||
| const ss = cwd.statFile(io, src, .{}) catch return false; | ||
| if (ss.size != ds.size) return false; | ||
| if (ss.inode == ds.inode and ss.mtime.nanoseconds == ds.mtime.nanoseconds) return true; |
There was a problem hiding this comment.
When a local dependency pin moves between filesystems, inode numbers are only unique within each filesystem, so the new source and the staged file can coincidentally have the same inode, size, and preserved mtime without being the same file. This early return then leaves the stage hardlinked to the old checkout, so later edits to the newly selected source are missed. Fresh evidence after the earlier fix is that this implementation still compares inode without a device/filesystem identifier; use a same-file primitive or include device identity in this check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not addressable portably on Zig 0.16: File.Stat has no device id and std has no same-file primitive. A collision also needs the same size and the same nanosecond mtime. Tracked in #806.
| // (assets routinely exceed any fixed readFileAlloc limit). | ||
| // Skipped when the copy is already current (#674): an | ||
| // unchanged asset keeps its identity and mtime. | ||
| if (write_if_changed.sameFiles(io, src_dir, entry.name, dst_dir, entry.name)) continue; |
There was a problem hiding this comment.
On POSIX, a mode-only source change is ignored because sameFiles compares only size and bytes. For a recursively copied pack asset or fallback-staged directory, changing a helper from non-executable to executable therefore leaves the generated copy with its old permissions, whereas the previous unconditional copyFile refreshed them. Compare the relevant permissions before taking this fast path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f67727e: sameFiles now compares permissions too, so both the recursive copy and the deps copy-fallback refresh on a mode-only change.
…opies) (#674) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f67727e9a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ds.nlink != 1) return false; | ||
| return write_if_changed.sameFiles(io, cwd, src, cwd, dest); |
There was a problem hiding this comment.
When a source file is atomically replaced with identical bytes and permissions, the old staged hardlink loses the source pathname and its link count drops to 1. This branch then mistakes that detached hardlink for a copy fallback and keeps it, so subsequent in-place edits to the new source no longer reach .labelle/deps or wake a long-lived build watcher. Fresh evidence beyond the earlier source-switch fix is that nlink == 1 does not prove the destination was copied: unlinking/replacing the source side of a hardlink produces exactly that state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc830e2 by simplifying: a stage counts as current only when it IS the source file (same inode, size and mtime). Anything else is re-hardlinked (link to a temp name, then rename over). Byte and permission equality is consulted only when hardlinking fails (the copy fallback). Regression test: syncTree: a stage detached from an atomically replaced source (same bytes) is relinked.
…for the copy fallback (#674) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc830e2927
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const tmp = try std.fmt.allocPrint(allocator, "{s}.labelle-link", .{dest}); | ||
| defer allocator.free(tmp); | ||
| cwd.deleteFile(io, tmp) catch {}; |
There was a problem hiding this comment.
When a dependency contains both a regular file x and a legitimate file named x.labelle-link, and directory iteration visits the suffixed file first, staging x deletes the already-staged sibling here before using that pathname as scratch space. Because both source names were added to seen, the orphan sweep does not restore the deleted file, so the first generated dependency tree is incomplete. Use a unique temporary path outside the mirrored package namespace, or otherwise ensure it cannot collide with a source entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but very unlikely: it needs a package file literally named *.labelle-link, and the next generate self-heals it. Tracked in #809.
| if (isLink(dest_sub)) { | ||
| try removeEntry(dest_sub); |
There was a problem hiding this comment.
For every native-language project that still has source files, the destination directory here is the live stageNativeSources symlink or junction from the preceding generation, but this branch unconditionally removes it and mirrors the plugin placeholder before stageNativeSources recreates the overlay later in generate. Thus even an unchanged generation replaces the native source link (twice when the normal and tests targets both run), defeating the non-destructive guarantee and potentially dropping a long-lived build watch on that staged source directory. The sync needs to preserve a currently required overlay and restore the placeholder only on the no-native-sources path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Known trade-off, taken deliberately in the previous round: correctness first, since the placeholder must come back when native sources go away. Keeping the overlay stable needs the has-sources predicate hoisted out of stageNativeSources. Tracked in #808.
1 participant
Closes #674.
What changes
If nothing in the inputs changed, a second
generatenow writes nothing. Every file under.labelle/<target>/and.labelle/deps/keeps its bytes, inode and mtime, anddeps/is no longer wiped.src/write_if_changed.zig: compares against the file on disk before writing. Used byscanner.writeFile(build.zig, build.zig.zon, main.zig, game.zig,__tests_root.zig, …), the mirrored copies (copyAndScan*,copyDir,copyDirRecursive*for assets), app icon/ico/rc, the staged plugin params/build-hook modules, the constants/i18n/flow modules, the v2 hook import and the csproj.src/deps_sync.zigreplaces thedeleteTree(deps)+hardlinkTreepass. Each staged package is reconciled in place:zig-pkg//.zig-cache/inside a stage are kept. The scripting declare step'szig buildfetches into them, and sweeping them would mean re-fetching on every generate.stageNativeSources) is kept and never descended into.DepsLinkOptions.prune/keep): the exe pass sweeps dropped packages but keeps the tests target's backend link, so the two passes don't fight.rewriteZonPathsnow reads the package's source zon, not the staged copy, and writes only on change. It is idempotent, so the old "additive pass must not re-rewrite" special case is gone and both passes run the same code.minimum_zig_versionin the generated zon comes from the assembler's ownbuild.zig.zon(build option). It was hardcoded"0.15.2".fp >> 32 == Crc32(.name), id not 0 or 0xffffffff) on the emitted zon. The CLI probe can be deleted: Remove the post-generate fingerprint probe (fixFingerprints): the assembler's fingerprint is authoritative labelle-cli#507.Tests (mechanism, not just bytes)
test/generate_stability_tests.zig:generateruns into the same dir leave every file (generated sources plus staged deps) with the same inode and mtime.zig-pkgandkeepentries survive; only dropped top-level deps are swept.minimum_zig_versionand the fingerprint rule.deps_sync.zig(kept/linked/removed counters) andwrite_if_changed.zig. TherewriteZonPathstests now assert that a second call doesn't touch the staged file.Manual acceptance (Windows, Zig 0.16.0)
Generated each example, then generated twice more, snapshotting sha256 + mtime + inode of every entry under
.labellebetween runs:.labelle/root metadata (below)declare-tool/zig-cachetimestampBefore this change, plugin-controllers showed 96 differences,
deps/included.zig build --list-stepson the generated tree exits 0 with nouse this value:.Not covered (issues filed)
.labelle/root metadata still changes on every run: thegenerationtoken andhook_routes.jsonby design (Hooks: expose generated event routes through a human-readable and JSON inspector #724), andgenerated_atinflow_catalog.json/manifest.json. None of these is a build input. generate: .labelle/ root metadata sidecars change on every run (generated_at, #674 follow-up) #801..resources(fantasy-dungeon) still move mtimes of atlas JSON and prefabs, becausepack_resourcesmutates files an earlier phase wrote. generate: pack .resources rewrites still move mtimes every run (copy-then-mutate chain, #674 follow-up) #800.Local test run
zig build teston this branch fails the same 3 tests asmainon this machine:language_policy … REAL generate output is byte-identical(Windows REPARSE_POINT_NOT_RESOLVED) and the twoMANIFEST_V2_GENERATE_CUTOVERtests. Everything else passes (3718/3760, 39 skipped).🤖 Generated with Claude Code
https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit