Repository navigation
feat(packs): per-pack module graph — pack files behind a real module boundary (#498 PR 2) - #538
Conversation
… module boundary (#498 PR 2) Each light pack becomes a build-system module (pack__<prefix>) rooted at a generated packs/<name>/__pack_root.zig that re-exports every scanned component/event/hook/script. The generated main.zig reaches pack contents EXCLUSIVELY through @import("pack__<prefix>") — all four emission sites converted atomically (component registry fields, event aliases, hook aliases, AllScripts), plus both flow-handler sites, which also fixes the pre-existing broken @import("scripts/packs/…") a pack flow-handler would have emitted. The module's import table is the wall: engine/core/gfx, the backend modules, ecs/gui when wired, and every decl-module plugin (plugins are the sanctioned inter-domain surface — FP's packs route worker access through worker_controller's citizens surface by design) — and nothing else. No `game` shim, no sibling packs. An implicit `contracts` pack is wired into every other pack module (one direction). Proven on a real two-pack project (null backend): builds green with a scene resolving citizens__Counter through the module path; a sibling relative import fails with "file exists in modules 'pack__citizens' and 'pack__production'"; @import("game") fails with "no module named 'game' available within module 'pack__production'". Prefab .jsonc embeds stay path-based (data has no module membership; embed path + <pack>__ key are the save contract). Registry FIELD names are unchanged (serde/save keys). Pack-less projects emit byte-identical output — every new writer gates on pack presence (goldens untouched). PR 3 (root/Registry bridge + @import("pack") self-import), PR 4 (exposes surfaces + depends_on), PR 5 (lint demotion + doc truth-up), PR 6 (example + e2e fixture) follow. Part of #498 Claude-Session: https://claude.ai/code/session_01P7YLw4hXFCCaY2LAUt4G1j
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR adds generated per-pack ChangesPack module generation and wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces per-pack module root generation (__pack_root.zig) and build-system wiring to enforce pack isolation boundaries ("wire the wall"). Instead of direct path imports, pack components, events, hooks, and scripts are now re-exported by their respective pack modules and accessed via @import("pack__<prefix>"). This prevents dual-module membership errors and restricts import tables to exclude the game shim and sibling packs. Feedback on the changes highlights a potential memory leak in src/codegen/pack_root.zig where an errdefer is needed to deallocate the ArrayList if toOwnedSlice fails after calling toArrayList().
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var arr_list = alloc_writer.toArrayList(); | ||
| return arr_list.toOwnedSlice(allocator); |
There was a problem hiding this comment.
When moving an allocating writer's buffer to an ArrayList via toArrayList(), the writer is reset to empty. To prevent memory leaks if a subsequent toOwnedSlice fails with OutOfMemory, an errdefer should be added to deinit the ArrayList. This is safe from double-free because the writer's deinit becomes a no-op on an empty buffer.
var arr_list = alloc_writer.toArrayList();
errdefer arr_list.deinit(allocator);
return arr_list.toOwnedSlice(allocator);
References
- In Zig, when moving an allocating writer's buffer to an ArrayList via
toArrayList(), the writer is reset to empty. To prevent memory leaks if a subsequenttoOwnedSlicefails withOutOfMemory, add anerrdeferto deinit the ArrayList. This is safe from double-free because the writer'sdeinitbecomes a no-op on an empty buffer. - In Zig 0.16, the
deinitAPI for an unmanaged ArrayList requires passing the allocator. When adding anerrdeferto free an unmanaged ArrayList on an allocation failure (such as beforetoOwnedSlice(allocator)), useerrdefer list.deinit(allocator).
There was a problem hiding this comment.
Applied in ce3a5c5 — errdefer arr_list.deinit(allocator) between toArrayList and toOwnedSlice, with a comment pinning the reset-writer rationale. (The identical pre-existing pattern in root.zig's generateTestsRoot is left for the PR 5 cleanup sweep.)
There was a problem hiding this comment.
Already applied in this very commit (ce3a5c5) — the errdefer sits at pack_root.zig:139; this looks like a re-post against the fix itself.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codegen/blocks/hooks.zig`:
- Around line 37-48: The import logic in printFlowHandlerImport should account
for FlowNodes-promoted scripts instead of always falling back to the path-based
scripts/{s} import, which can recreate the duplicate-module issue. Thread the
promotion state into this helper and, when the entry is promoted, switch to the
script__<ident> import path using the existing rel_path/identifier plumbing
rather than the plain file import. Also replace the misleading error.NameTooLong
return for a missing plugin_name with the appropriate missing-plugin sentinel or
early return used elsewhere in this codepath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b44e7fa5-fd69-4b22-94e8-b9dd85b467b1
📒 Files selected for processing (7)
src/build_files.zigsrc/codegen/blocks/hooks.zigsrc/codegen/blocks/imports.zigsrc/codegen/blocks/registries.zigsrc/codegen/pack_root.zigsrc/root.zigtest/pack_scan_tests.zig
…he hook sites; errdefer the pack-root buffer CodeRabbit: printFlowHandlerImport now mirrors AllScripts' THREE shapes — a handler that also exports FlowNodes routes through @import("script__<ident>") at both hook sites (this hole predates the PR: the old inline path-imports had it too; the shared helper is where it becomes fixable). Misleading error.NameTooLong sentinel replaced with error.PackScriptMissingOwner at both pack-marker sites. Two routing tests pin promoted + plain shapes. Gemini: errdefer the toArrayList buffer in renderPackRoot (toArrayList resets the writer, so an OOM inside toOwnedSlice leaked it). Claude-Session: https://claude.ai/code/session_01P7YLw4hXFCCaY2LAUt4G1j
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/pack_scan_tests.zig (1)
1219-1225: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnpromoted-handler test only checks one of the two hook sites.
The promoted-handler test (Line 1212-1213) verifies both the
GameHooksreceiver-type tuple and thehooks_initinstantiation route throughscript__counter. The unpromoted counterpart only asserts the receiver-type shape (Line 1223) — it never checks that thehooks_initsite also keeps the plainscripts/counter.zigpath import (and doesn't leakscript__counter) for this case. Given the PR explicitly routes "at both hook sites," the negative/unpromoted path deserves the same symmetric coverage to catch a regression at thehooks_initsite specifically.Suggested addition
test "a plain (unpromoted) flow handler keeps the scripts/ path import" { const main_zig = try gen(empty_plugin_flow_nodes); defer std.testing.allocator.free(main_zig); try std.testing.expect(contains(main_zig, "*`@import`(\"scripts/counter.zig\").FlowEventHandler,")); + try std.testing.expect(contains(main_zig, "`@import`(\"scripts/counter.zig\").FlowEventHandler = .{};")); try std.testing.expect(!contains(main_zig, "script__counter")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pack_scan_tests.zig` around lines 1219 - 1225, Add symmetric coverage in the unpromoted-handler test so it checks both hook sites, not just the `GameHooks` receiver tuple. In `test "a plain (unpromoted) flow handler keeps the scripts/ path import"`, extend the assertions around `gen(empty_plugin_flow_nodes)` to also verify the `hooks_init` instantiation still uses the plain `scripts/counter.zig` import and does not reference `script__counter`, matching the promoted-handler test’s two-site coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/pack_scan_tests.zig`:
- Around line 1219-1225: Add symmetric coverage in the unpromoted-handler test
so it checks both hook sites, not just the `GameHooks` receiver tuple. In `test
"a plain (unpromoted) flow handler keeps the scripts/ path import"`, extend the
assertions around `gen(empty_plugin_flow_nodes)` to also verify the `hooks_init`
instantiation still uses the plain `scripts/counter.zig` import and does not
reference `script__counter`, matching the promoted-handler test’s two-site
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43cdfab1-3352-42fe-b0b5-677b4c5ef634
📒 Files selected for processing (4)
src/codegen/blocks/hooks.zigsrc/codegen/blocks/registries.zigsrc/codegen/pack_root.zigtest/pack_scan_tests.zig
🚧 Files skipped from review as they are similar to previous changes (3)
- src/codegen/blocks/hooks.zig
- src/codegen/blocks/registries.zig
- src/codegen/pack_root.zig
1 participant
PR 2 of the #498 train ("wire the wall") — the atomic one: the per-pack Zig module graph.
What lands
packs/<name>/__pack_root.zig(newcodegen/pack_root.zigrenderer): generated module root re-exporting every scanned component/event/hook/script underpathToIdentaccessors. Prefab.jsoncstays@embedFiled by path — data has no module membership, and embed path +<pack>__key are the save contract.pack__<prefix>_modin the generated build.zig (emitPackModules/emitPackImports, mirroring the Game-script CustomNodes don't compile: shim missing PluginFlowNodes + named-module promotion needed (follows #238) #240 promoted-script wiring):createModuleper pack +addImporton every artifact (desktop exe, wasm, android lib, ios exe, test_root).AllScripts— plus both flow-handler sites (GameHooksreceiver types +hooks_init), which also handles the pre-existing seam where a pack script withhas_event_handleremitted a broken@import("scripts/packs/…"). One file, one module: any remaining path import would be the dual-module compile error, so partial conversion was never an option.The import table IS the wall
In:
labelle-engine/labelle-core/labelle-gfx, the four backend modules,ecs_backend/gui_backendwhen wired, every decl-module plugin, and the implicitcontractspack (one-directionaloverrideImport). Out: thegameshim and every sibling pack.Deviation from the issue sketch, by design decision: the guide said "NOT other plugins", but FP's packs import plugins 50+ times as their sanctioned domain surface (the citizens pack's own boundary doc routes all worker access through
worker_controller's surface), so plugins stay importable — the wall targets pack↔pack isolation and game-root reach, not the shared substrate.Proven on a real build
Scratch two-pack project (null backend, zig_ecs, scene resolving
citizens__Counterthrough the module path):error: file exists in modules 'pack__citizens' and 'pack__production'@import("game")→error: no module named 'game' available within module 'pack__production'Invariants held
<pack>__<Pascal>— the serde/save keys);PackViewallow-lists untouched.PACK_ROOT_RENDER+PACK_MODULE_BUILDstructs (renderer shape, restricted-table slice asserts, contracts direction check, no-pack zero-wiring) and the PACK_EMISSION/PACK_SCRIPTS asserts flipped to the module forms with negative old-form guards.Consumer note
FP adopts this at its next assembler re-pin: plugin imports keep working (see above); its 5 known game-root escapes (
../../../components/game_time.zig) become compile errors — that's the already-tracked.global-facets contracts migration, not a regression.Next in train: PR 3 (
@import("root")Registry bridge +@import("pack")self-import), PR 4 (exposessurface modules +depends_on), PR 5 (lint demotion + stale-comment truth-up), PR 6 (examples/packs-demo + CI e2e fixture). Also flagged:root.zig(~2050) andbuild_files.zig(~1160) exceed the 1000-line rule — split proposed alongside PR 5 rather than inside this atomic change.Part of #498
https://claude.ai/code/session_01P7YLw4hXFCCaY2LAUt4G1j
Summary by CodeRabbit
New Features
Bug Fixes
Tests