packs: light pack builds end-to-end — don't emit @import/module dep for module-less dir-scan packs - #482
Conversation
…odule wiring (#481) A light pack (module-less, carrying pack.labelle) is declared in project.labelle .plugins just like a real plugin, but ships no Zig module — only convention dirs scanned + <pack>__-namespaced into the unified registries (#439/#440). The plugin-list/build wiring still treated every .plugins entry as a decl-module plugin, emitting @import("<pack>") in main.zig and a b.dependency("labelle_<pack>", …) in build.zig/.zon that a module-less pack cannot resolve, so labelle build failed. Add root.declModulePlugins: splits the declared .plugins list into the decl-module subset (kept) vs light packs (dropped, name in pack_entries). generate() now runs the whole module-emitting phase against a cfg copy (cfg_modules) whose .plugins excludes light packs, so no pack import or build dep is emitted; the pack's contribution stays the already-scanned pack_scans registry entries. Audited + switched every module-emitting site: build.zig.zon deps, build.zig module graph, main.zig ComponentRegistryWithPlugins / SystemRegistry / plugin controllers, plugin events + flow-decl discovery. Project-description sidecars (flow catalog, feature manifest) keep the full cfg. Closes #481, Part of #651
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds light-pack filtering to module wiring, so module-less packs skip ChangesLight Pack Module Exclusion
Pack and plugin manifest conflict guard
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant generate as generate()
participant declModulePlugins as declModulePlugins
participant build_files as build_files
participant main_zig as main_zig
generate->>declModulePlugins: cfg.plugins, pack_names
declModulePlugins-->>generate: filtered module_plugins
generate->>build_files: cfg_modules
generate->>main_zig: cfg_modules
main_zig-->>generate: main.zig without light-pack imports
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/gemini review |
![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 a mechanism to filter out "light packs" (module-less, directory-scanned convention bundles) from the list of plugins that require Zig module imports or build dependencies. It adds the declModulePlugins helper function to perform this filtering and updates the generation process to use this filtered list (cfg_modules) when generating main.zig, build.zig, and build.zig.zon. Unit tests have also been added to verify this behavior. There are no review comments, so I have no feedback to provide.
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.
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to filter out module-less 'light packs' from the list of plugins used during code generation and build dependency setup. By adding the declModulePlugins helper in src/root.zig, the assembler now separates genuine decl-module plugins from directory-scanned light packs, ensuring that the latter do not generate invalid @import statements or build dependencies in build.zig and build.zig.zon. Comprehensive unit and integration tests have been added to verify this filtering behavior and ensure light packs build successfully end-to-end. I have no feedback to provide.
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.
![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/root.zig`:
- Around line 1339-1349: The decl-module plugin selection is currently using
pack entries that may include light-pack-only plugins, so any plugin with a
pack.labelle gets dropped from declModulePlugins and loses import/dependency
wiring. Add an explicit rejection or validation in root.zig around
declModulePlugins or in the manifest-loading path so decl-module plugins cannot
carry pack.labelle, and ensure cfg_modules only receives the mutually exclusive
decl-module subset.
🪄 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: d51c7efe-fc1c-43e6-a558-9124e7607626
📒 Files selected for processing (2)
src/root.zigtest/pack_scan_tests.zig
| var pack_names_for_filter: std.ArrayList([]const u8) = .empty; | ||
| defer pack_names_for_filter.deinit(allocator); | ||
| try pack_names_for_filter.ensureTotalCapacity(allocator, pack_entries.items.len); | ||
| for (pack_entries.items) |e| pack_names_for_filter.appendAssumeCapacity(e.plugin.name); | ||
|
|
||
| const module_plugins = try declModulePlugins(allocator, cfg.plugins, pack_names_for_filter.items); | ||
| defer allocator.free(module_plugins); | ||
| // A `cfg` view whose `.plugins` is the decl-module subset. Passed to every | ||
| // site that emits a Zig-module import or a build-file module dependency. | ||
| var cfg_modules = cfg; | ||
| cfg_modules.plugins = module_plugins; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for how pack.labelle vs plugin.labelle / module presence are distinguished.
fd -t f 'plugin_manifest.zig' -x cat -n {}
rg -nP -C3 'loadPackOptional|pack\.labelle|plugin\.labelle' src/Repository: labelle-toolkit/labelle-assembler
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the module-wiring logic around declModulePlugins and its callers.
ast-grep outline src/root.zig --view expanded | sed -n '1,260p'
printf '\n--- declModulePlugins references ---\n'
rg -n -C 4 'declModulePlugins|module_plugins|pack_entries|loadPackOptional|loadOptional|b\.dependency|`@import`' src/root.zigRepository: labelle-toolkit/labelle-assembler
Length of output: 23742
Reject pack.labelle on decl-module plugins
pack_entries is the light-pack set, so any plugin with a pack.labelle is filtered out of declModulePlugins and loses its @import/b.dependency wiring. Add a guard here or in the manifest loader if both manifests are meant to be mutually exclusive.
🤖 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 `@src/root.zig` around lines 1339 - 1349, The decl-module plugin selection is
currently using pack entries that may include light-pack-only plugins, so any
plugin with a pack.labelle gets dropped from declModulePlugins and loses
import/dependency wiring. Add an explicit rejection or validation in root.zig
around declModulePlugins or in the manifest-loading path so decl-module plugins
cannot carry pack.labelle, and ensure cfg_modules only receives the mutually
exclusive decl-module subset.
A light pack (pack.labelle) and a decl-module plugin (plugin.labelle + build.zig / src/root.zig) are mutually exclusive by design: a pack is module-less and contributes only its scanned convention dirs, while a decl-module plugin is wired in as an importable module. declModulePlugins() in root.zig treats "carries pack.labelle" as the sole light-pack predicate and DROPS every such plugin from the module wiring (@import / b.dependency). A plugin that erroneously carried BOTH a pack.labelle AND decl-module content would be silently stripped of its module wiring — a confusing failure the CodeRabbit review flagged. Add a generate-time guard in loadPackFromDir(): when a dir carries a pack.labelle, reject it with error.PackAndPluginManifestConflict if it ALSO ships any decl-module signal (plugin.labelle, build.zig, or src/root.zig). This keeps declModulePlugins fed a clean either/or split. Tests: pack + each of the three decl-module signals -> rejected; a clean light pack and a clean decl-module plugin both still load.
|
Addressed the CodeRabbit (Major) finding — reject Guard location: Detection signal: a new helper
Any one present ⇒ generation fails with Tests added ( Verify: |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a mutual-exclusivity guard and filtering mechanism to distinguish between light packs (module-less, directory-scanned bundles) and decl-module plugins (real Zig packages). It ensures that light packs are not incorrectly imported or treated as build dependencies in generated files, while still registering their scanned components and events. The feedback suggests optimizing memory allocation in src/root.zig by replacing a dynamic std.ArrayList with a fixed-size slice allocation, as the number of pack entries is known beforehand.
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 pack_names_for_filter: std.ArrayList([]const u8) = .empty; | ||
| defer pack_names_for_filter.deinit(allocator); | ||
| try pack_names_for_filter.ensureTotalCapacity(allocator, pack_entries.items.len); | ||
| for (pack_entries.items) |e| pack_names_for_filter.appendAssumeCapacity(e.plugin.name); | ||
|
|
||
| const module_plugins = try declModulePlugins(allocator, cfg.plugins, pack_names_for_filter.items); |
There was a problem hiding this comment.
Since the exact number of pack entries (pack_entries.items.len) is known beforehand, we can allocate a slice directly using allocator.alloc instead of using a dynamic std.ArrayList. This simplifies the code, improves readability, and avoids the overhead of ArrayList capacity tracking.
const pack_names_for_filter = try allocator.alloc([]const u8, pack_entries.items.len);
defer allocator.free(pack_names_for_filter);
for (pack_entries.items, 0..) |e, i| pack_names_for_filter[i] = e.plugin.name;
const module_plugins = try declModulePlugins(allocator, cfg.plugins, pack_names_for_filter);
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes labelle build failures when a project declares a light pack (a pack.labelle directory-scan-only pack with no Zig module) in .plugins, by ensuring codegen/build wiring only emits @import(...) and b.dependency(...) for decl-module plugins.
Changes:
- Add
declModulePlugins(...)and thread a filteredcfg_modulesthrough all module-emitting sites so light packs don’t produce@import("<pack>")or module build deps. - Add mutual-exclusivity validation so a
pack.labelledirectory can’t also ship decl-module signals (plugin.labelle,build.zig,src/root.zig). - Add tests covering the filter behavior and verifying emitted
main.zig/build.zigcontent excludes light-pack module wiring while still registering scanned pack items.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/root.zig |
Introduces and applies the decl-module plugin filter (cfg_modules) to prevent emitting imports/deps for module-less light packs. |
src/plugin_manifest.zig |
Adds a guard rejecting pack/plugin manifest (or decl-module signal) coexistence and tests for that behavior. |
test/pack_scan_tests.zig |
Adds tests asserting light packs are excluded from module wiring while their scanned items remain emitted/registered. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The mutual-exclusivity guard lives on the pack path; a decl-module | ||
| // plugin with a build.zig + src/root.zig but NO pack.labelle loads fine | ||
| // through the plugin.labelle path. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
Closes #481, Part of #651
Problem
Pack items now register (components/events/prefabs/hooks scanned,
<pack>__-namespaced, validated — #439/#440/#441), but a light pack did not build end-to-end. A light pack is declared inproject.labelle.pluginslike any plugin, yet ships no Zig module (a bare dir of convention files, nobuild.zig). The plugin-list/build wiring still treated every.pluginsentry as a decl-module plugin, emitting:@import("<pack>")in the generatedmain.zig(ComponentRegistryWithPlugins / SystemRegistry args, plugin controllers), andb.dependency("labelle_<pack>", …)+ module dep inbuild.zig/build.zig.zon,which a module-less pack can't resolve — so
labelle buildon a game with a light pack failed to compile.Change
Split the declared
.pluginslist into decl-module plugins (get the module import + build dep) vs light packs (dir-scan-only; their entire contribution is the already-scannedpack_scansregistry entries).root.declModulePlugins(allocator, plugins, pack_names)— returns the subset ofpluginsNOT carrying apack.labelle(i.e. not inpack_entries), order preserved, caller-owned.generate()builds acfg_modulesview whose.pluginsis that decl-module subset, and runs the whole module-emitting phase against it. The scan/copy + pack-scan loops that need the FULL declared list already ran earlier and are untouched.Audited generated sites (all now fed
cfg_modules)build.zig.zondeps —generateBuildZigZon(.labelle_<name> = .{ .path })build.zigmodule graph —generateBuildZig(b.dependency/plugin_<name>_mod/overrideImport/ sibling wiring /@libstest chaining)main.zig:ComponentRegistryWithPlugins/SystemRegistryplugin args (@import("<name>"))_plugin_modstuplediscoverPluginEventsdiscoverPluginFlowDeclsgame.zigshim'sPluginEvents/PluginFlowNodes(consume the filtered discovery results)Kept on the full
cfg(intentional): the project-description sidecars — flow catalog (emitFlowCatalogSidecar) and feature manifest (emitManifestSidecar) — so they still describe every declared plugin/pack. Both are best-effort/non-fatal and tolerate a pack's missingsrc/root.zig.Verification
zig build+zig build testpass (46/46 steps, 1040/1044 tests, 4 pre-existing skips).Added tests in
test/pack_scan_tests.zig:LIGHT_PACK_MODULE_FILTER— unit tests fordeclModulePlugins(drops light packs, keeps decl-module plugins, order preserved; no-op with no packs; drops a game whose only plugin is a light pack).LIGHT_PACK_BUILDS_END_TO_END— feeds a declared.pluginslist (physicsdecl-module +citizenslight pack) through the productiondeclModulePluginssplit, then the real emitters:@import("physics")IS present,@import("citizens")is NOT, yet the pack's component (.citizens__Worker = @import("packs/citizens/components/Worker.zig").Worker), event, and prefab embed ARE emitted (viapack_scans).b.dependency("labelle_physics"/plugin_physics_modpresent,labelle_citizens/plugin_citizens_modabsent.This chains the actual #481 filter into the actual codegen/build emitters. A full
generate()compile-the-binary harness isn't wired in this repo (existing pack/build tests all drive the sub-emitters directly, since fullgenerate()needs on-disk framework-package resolution), so the end-to-end proof is at the emitted-content layer: no pack import/dep, pack items present.Deferred
Nothing in scope. The engine-side isolation half (PackView registry partition) remains #652-remainder per the ticket.
Summary by CodeRabbit
New Features
Bug Fixes
Tests