Repository navigation
feat(#439): pack dir-scan — register pack components/events/prefabs into unified registries - #478
Conversation
…nto unified registries
Finish the pack directory-scan (Packs RFC §4, convention_dirs = copy_and_scan)
so a light pack's components/ events/ prefabs/ are scanned like the game root
and wired into the generated registries, instead of the scanned names being
computed and discarded.
- New pack.labelle manifest (plugin_manifest.zig): scalar
.convention_dirs = .copy_and_scan shorthand; exposes/depends_on accepted
but ignored for now.
- root.scanPack copies <pack>/{components,events,prefabs}/ into
<target>/packs/<name>/ and records stems as a PackScan.
- Codegen block-writers register pack items into the SAME registries the
game root feeds (the unified set): Components (ComponentRegistryWithPlugins
field list), GameEvents union + event imports, embedded prefabs.
- Threaded via main_template.pack_scans module-level var (mirrors
loop_style_override) to avoid churning the 100+ call sites of the ~19-arg
generateMainZigFromTemplate.
Deferred with clean seams: hooks/ scanning; the invisible <pack>__ prefix +
.jsonc ref rewrite (#440); the per-pack global++own registry partition /
PackView (#652-remainder); exposes/depends_on DAG. Registered names are BARE
today.
Part of #651, implements #439.
Tests: pack_scan_tests.zig (scanPack copy/scan + emission assertions) and
pack.labelle parser tests in plugin_manifest.zig.
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds pack support end to end: it parses ChangesPack Support Implementation
Estimated code review effort: 4 (Complex) | ~45 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 implements pack directory scanning (Packs RFC §4, #439), allowing light, directory-scanned plugins (packs) to contribute components, events, and prefabs directly into the unified game-root registries. It introduces the PackScan and PackManifest structures, updates the codegen templates and block-writers to integrate pack items, and adds comprehensive tests in test/pack_scan_tests.zig. The review feedback recommends replacing debug-specific print functions (std.debug.print) with standard logging facilities (such as std.log.warn) for user-facing warnings in src/plugin_manifest.zig.
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.
| std.debug.print( | ||
| "labelle: failed to parse pack.labelle for pack '{s}' at {s}\n parser error: {any}\n see docs/RFC-packs.md for the pack manifest schema\n", | ||
| .{ expected_name, manifest_path, err }, | ||
| ); |
There was a problem hiding this comment.
In CLI tools, use standard logging facilities (such as std.log.warn) or write directly to stderr for user-facing warnings/errors, rather than using debug-specific print functions like std.debug.print.
std.log.warn(
"labelle: failed to parse pack.labelle for pack '{s}' at {s}\n parser error: {any}\n see docs/RFC-packs.md for the pack manifest schema\n",
.{ expected_name, manifest_path, err },
);
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
| std.debug.print( | ||
| "labelle: pack.labelle name mismatch\n project.labelle declares '{s}'\n but its pack.labelle has name = '{s}'\n at {s}\n", | ||
| .{ expected_name, parsed.name, manifest_path }, | ||
| ); |
There was a problem hiding this comment.
In CLI tools, use standard logging facilities (such as std.log.warn) or write directly to stderr for user-facing warnings/errors, rather than using debug-specific print functions like std.debug.print.
std.log.warn(
"labelle: pack.labelle name mismatch\n project.labelle declares '{s}'\n but its pack.labelle has name = '{s}'\n at {s}\n",
.{ expected_name, parsed.name, manifest_path },
);
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
| std.debug.print( | ||
| "labelle: pack '{s}' has manifest_version {d}\n but this labelle-cli release supports manifest_version 1..{d}\n", | ||
| .{ expected_name, parsed.manifest_version, SUPPORTED_MANIFEST_VERSION }, | ||
| ); |
There was a problem hiding this comment.
In CLI tools, use standard logging facilities (such as std.log.warn) or write directly to stderr for user-facing warnings/errors, rather than using debug-specific print functions like std.debug.print.
std.log.warn(
"labelle: pack '{s}' has manifest_version {d}\n but this labelle-cli release supports manifest_version 1..{d}\n",
.{ expected_name, parsed.manifest_version, SUPPORTED_MANIFEST_VERSION },
);
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
… warnings Address Gemini: the 3 new pack.labelle warning sites (parse failure, name mismatch, unsupported manifest_version) are user-facing CLI warnings — route them through std.log.warn. Left pre-existing debug.print calls untouched (out of scope for this PR).
|
Addressed @gemini-code-assist — the 3 new pack.labelle warning sites now use |
|
Thanks for the update, @apotema. Using |
![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: 12665e08df
ℹ️ 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".
| for (self.pack_scans) |pack| { | ||
| for (pack.prefab_names) |name| { | ||
| const display = std.fs.path.basename(name); | ||
| try w.print(" try JsoncBridge.addEmbeddedPrefab(&g, \"{s}\", @embedFile(\"{s}/prefabs/{s}.jsonc\"), \"prefabs\");\n", .{ display, pack.import_prefix, name }); |
There was a problem hiding this comment.
When a pack prefab contains a JSONC include or other source-relative lookup, this emits the source from packs/<name>/prefabs/... but still registers the prefab with the root path literal "prefabs", so the bridge resolves relative paths against the game's prefab directory instead of the copied pack directory; the callback lifecycle mirrors the same literal. Pass the pack's prefixed prefab root (for example {pack.import_prefix}/prefabs) here so scanned pack prefabs can resolve their own local files.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codegen/main_template.zig (1)
146-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExtend basename-collision checks to pack prefabs
checkBasenameCollisionsonly seesprefab_names, but pack prefabs are also registered by bare basename viaJsoncBridge.addEmbeddedPrefab(...). A pack prefab that matches a root prefab—or another pack prefab—will overwrite the earlier entry at runtime. The same bare-name collision risk also exists for pack component/event registries, which currently fail later with a duplicate-field error.🤖 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/codegen/main_template.zig` around lines 146 - 162, The basename-collision check in the main template only covers root prefab names, so pack prefabs and pack component/event registries can still collide and overwrite or fail later. Extend the collision detection around `checkBasenameCollisions` / `prefab_names` to include the bare basenames used by `JsoncBridge.addEmbeddedPrefab(...)` and the pack registries, and surface the same early diagnostic when duplicates are found. Update the generation path in `main_template.zig` so all registered bare names are validated together before emission.
🧹 Nitpick comments (4)
src/plugin_manifest.zig (3)
202-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReusing
SUPPORTED_MANIFEST_VERSIONcouples pack and plugin schema versioning.The file's own comment (Lines 104-108) states
pack.labelleis deliberately kept as a "SEPARATE file + schema fromplugin.labelle... so neither parser has to disambiguate". However,loadPackFromDirvalidates against the sameSUPPORTED_MANIFEST_VERSIONconstant used byloadFromDirforplugin.labelle(Line 13, Line 331). Since the two schemas are independent, a future bump to this constant driven purely by aplugin.labellechange would silently widen the accepted version range forpack.labelletoo, even if the pack schema hasn't actually changed.Consider introducing a distinct
SUPPORTED_PACK_MANIFEST_VERSIONconstant so the two schemas can version independently.🤖 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/plugin_manifest.zig` around lines 202 - 208, `loadPackFromDir` is reusing the plugin schema version constant, which incorrectly ties `pack.labelle` validation to `plugin.labelle` schema changes. Add a separate `SUPPORTED_PACK_MANIFEST_VERSION` constant in `src/plugin_manifest.zig` and update `loadPackFromDir` to validate against it instead of `SUPPORTED_MANIFEST_VERSION`; keep `loadFromDir` using the existing plugin constant so each parser can version independently.
158-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared file-read/parse boilerplate.
loadPackFromDirduplicates the read-file →dupeZ→zon.parse.fromSliceAlloc→ error-log boilerplate already present inloadFromDir(Lines 272-310), differing only in the target type and error names. Consider extracting a small generic helper (e.g.,readManifestFile(allocator, manifest_path) ![:0]u8or a comptime-generic parse wrapper) to avoid maintaining two near-identical ~20-line blocks in sync.♻️ Sketch of a shared helper
+fn readManifestBytes(allocator: std.mem.Allocator, manifest_path: []const u8) !?[]u8 { + const cwd = std.Io.Dir.cwd(); + return cwd.readFileAlloc(config.globalIo(), manifest_path, allocator, .limited(64 * 1024)) catch |err| { + if (err == error.FileNotFound) return null; + return err; + }; +}Both
loadFromDirandloadPackFromDircould then call this and only diverge on the ZON type / validation.🤖 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/plugin_manifest.zig` around lines 158 - 216, `loadPackFromDir` repeats the same read-file, `dupeZ`, `zon.parse.fromSliceAlloc`, and error logging flow already implemented in `loadFromDir`, so extract that shared boilerplate into a small reusable helper. Add a generic or file-read helper that returns the parsed ZON result, then have both `loadFromDir` and `loadPackFromDir` call it and keep only their type-specific validation and error mapping (`PackManifestParseError`, `PackManifestNameMismatch`, `PackManifestUnknownVersion`) in the individual functions.
932-1048: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood test coverage; one parity gap with malformed ZON.
The pack manifest tests mirror the plugin ones well (missing file, shorthand parse, default, ignored fields, name mismatch, version mismatch). One case present for
loadFromDirbut missing here: a malformed-ZON test assertingerror.PackManifestParseError(parity with Line 757's"loadFromDir: errors on malformed ZON").🤖 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/plugin_manifest.zig` around lines 932 - 1048, Add a malformed-ZON parity test to loadPackFromDir alongside the existing pack manifest cases, similar to the loadFromDir malformed ZON test. Use loadPackFromDir and writePackManifestFile to feed invalid ZON, then assert it returns error.PackManifestParseError so manifest parsing behavior matches the plugin loader tests.test/pack_scan_tests.zig (1)
139-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate long positional call — extend
genWithPackinstead of re-inlining it.The "no packs" test (Lines 223-243) re-inlines the entire
generateMainZigFromTemplatepositional argument list thatgenWithPackalready wraps, rather than callinggenWithPackwith an empty pack list. Since the PR explicitly notes this is "the existing large function signature" left unchanged, two independent call sites make it easy for them to silently diverge on a future signature change.♻️ Suggested consolidation
-fn genWithPack(tmpl: []const u8, cfg: generate.ProjectConfig, pack: generate.PackScan) ![]const u8 { - generate.main_template.pack_scans = &.{pack}; +fn genWithPacks(tmpl: []const u8, cfg: generate.ProjectConfig, packs: []const generate.PackScan) ![]const u8 { + generate.main_template.pack_scans = packs; defer generate.main_template.pack_scans = &.{}; return generate.generateMainZigFromTemplate( std.testing.allocator, tmpl, cfg, raylib_lifecycle, ... ); } + +fn genWithPack(tmpl: []const u8, cfg: generate.ProjectConfig, pack: generate.PackScan) ![]const u8 { + return genWithPacks(tmpl, cfg, &.{pack}); +}Then the "no packs" test becomes:
- const main_zig = try generate.generateMainZigFromTemplate( - std.testing.allocator, - component_tmpl, - .{ .y_axis = .up, .name = "test-game", .backend = .raylib, .ecs = .mock }, - raylib_lifecycle, - ... - ); + const main_zig = try genWithPacks( + component_tmpl, + .{ .y_axis = .up, .name = "test-game", .backend = .raylib, .ecs = .mock }, + &.{}, + );Also applies to: 218-250
🤖 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 139 - 164, The no-packs test is duplicating the long generateMainZigFromTemplate positional argument list instead of reusing genWithPack, which risks the two call sites drifting apart. Update genWithPack to support an empty pack case (or a pack list parameter) and have the no-packs test call that helper with no packs, using the existing generate.main_template.pack_scans setup to keep all generateMainZigFromTemplate arguments centralized.
🤖 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 980-1025: The pack scan logic now writes plugin content under a
top-level packs layout, so “packs” must be treated as a reserved convention-dir
name. Update RESERVED_DIR_NAMES in plugin_manifest.zig to include "packs" so
loadPackOptional rejects any plugin manifest that tries to declare
convention_dirs.name = "packs". Keep the check aligned with existing
reserved-name validation so scanPack and the new pack_scans flow can’t collide
with target_dir/packs/.
---
Outside diff comments:
In `@src/codegen/main_template.zig`:
- Around line 146-162: The basename-collision check in the main template only
covers root prefab names, so pack prefabs and pack component/event registries
can still collide and overwrite or fail later. Extend the collision detection
around `checkBasenameCollisions` / `prefab_names` to include the bare basenames
used by `JsoncBridge.addEmbeddedPrefab(...)` and the pack registries, and
surface the same early diagnostic when duplicates are found. Update the
generation path in `main_template.zig` so all registered bare names are
validated together before emission.
---
Nitpick comments:
In `@src/plugin_manifest.zig`:
- Around line 202-208: `loadPackFromDir` is reusing the plugin schema version
constant, which incorrectly ties `pack.labelle` validation to `plugin.labelle`
schema changes. Add a separate `SUPPORTED_PACK_MANIFEST_VERSION` constant in
`src/plugin_manifest.zig` and update `loadPackFromDir` to validate against it
instead of `SUPPORTED_MANIFEST_VERSION`; keep `loadFromDir` using the existing
plugin constant so each parser can version independently.
- Around line 158-216: `loadPackFromDir` repeats the same read-file, `dupeZ`,
`zon.parse.fromSliceAlloc`, and error logging flow already implemented in
`loadFromDir`, so extract that shared boilerplate into a small reusable helper.
Add a generic or file-read helper that returns the parsed ZON result, then have
both `loadFromDir` and `loadPackFromDir` call it and keep only their
type-specific validation and error mapping (`PackManifestParseError`,
`PackManifestNameMismatch`, `PackManifestUnknownVersion`) in the individual
functions.
- Around line 932-1048: Add a malformed-ZON parity test to loadPackFromDir
alongside the existing pack manifest cases, similar to the loadFromDir malformed
ZON test. Use loadPackFromDir and writePackManifestFile to feed invalid ZON,
then assert it returns error.PackManifestParseError so manifest parsing behavior
matches the plugin loader tests.
In `@test/pack_scan_tests.zig`:
- Around line 139-164: The no-packs test is duplicating the long
generateMainZigFromTemplate positional argument list instead of reusing
genWithPack, which risks the two call sites drifting apart. Update genWithPack
to support an empty pack case (or a pack list parameter) and have the no-packs
test call that helper with no packs, using the existing
generate.main_template.pack_scans setup to keep all generateMainZigFromTemplate
arguments centralized.
🪄 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: 6b87a5f2-24d2-478b-872f-bb014b565055
📒 Files selected for processing (14)
build.zigsrc/codegen/blocks/events.zigsrc/codegen/blocks/hooks.zigsrc/codegen/blocks/imports.zigsrc/codegen/blocks/registries.zigsrc/codegen/context.zigsrc/codegen/lifecycle/callback.zigsrc/codegen/lifecycle/loop.zigsrc/codegen/main_template.zigsrc/codegen/scan.zigsrc/main_zig.zigsrc/plugin_manifest.zigsrc/root.zigtest/pack_scan_tests.zig
…view) Finding 1 (chatgpt-codex, functional): pack-scanned prefabs were registered with the bare game-root prefab root `"prefabs"`, so a pack prefab's JSONC `"include"` / source-relative lookups resolved against the game's prefabs/ dir instead of the copied pack dir. Register them with the pack's own `<import_prefix>/prefabs` root at both the loop- lifecycle (loop.zig) and callback-lifecycle (callback.zig) emit sites. Game-root prefabs keep using `"prefabs"`. Finding 2 (CodeRabbit, minor): reserve `packs` as a convention-dir name in RESERVED_DIR_NAMES so a plugin can't declare convention_dirs.name = "packs" and write into target_dir/packs/, colliding with the pack-scan layout. Tests: extend pack_scan prefab-emission test to assert the pack prefab root is `packs/citizens/prefabs` (not `"prefabs"`); add a plugin_manifest reserved-name rejection test for `packs`; add `packs` to the isReservedDirName inline list.
|
Addressed both review findings (commit e6644ea): Finding 1 (chatgpt-codex, functional) — pack prefab includes resolved against the wrong dir. Confirmed real:
Game-root prefabs are unchanged (still Finding 2 (CodeRabbit, minor) — reserve Tests:
|
1 participant
What
Finishes the pack directory-scan (
convention_dirs = copy_and_scan), the phase-4 crux of the Packs initiative (RFCFlying-Platform/flying-platform-labelledocs/RFC-packs.md§4 + §6).A local pack is the light, directory-scanned form of a plugin: instead of contributing components/events/prefabs through decl-modules (
pub const Components), it drops game-convention files intocomponents/ events/ prefabs/and ships a thinpack.labelle. Before this PR, a pack's non-script convention dirs were scanned and then discarded — the names never reached the generated registries (root.zigcopy_and_scan loop:scanner.freeNames(...)with a "computed but not exposed" comment). This wires them into the same registries the game root feeds (the unified set, RFC §6-1b), so pack components are stored/serialized identically to game-root ones.Part of #651, implements #439.
Implemented
pack.labellemanifest (plugin_manifest.zig): the scalar.convention_dirs = .copy_and_scanshorthand (RFC §10 Q3), kept as a separate typed schema fromplugin.labelle's per-dir array so neither parser has to disambiguate scalar-vs-array in one field.exposes/depends_onare accepted but ignored (ignore_unknown_fields) — the DAG/isolation work is later.root.scanPack: copies<pack>/{components,events,prefabs}/into<target>/packs/<name>/…and records the scanned stems as aPackScan(import_prefix = packs/<name>). Missing subdirs tolerated. Runs for every plugin that carries apack.labelle; decl-module plugins (nopack.labelle) are untouched → fully back-compat.ComponentRegistryWithPlugins(.{ … })field list (registries.zig)GameEventsunion +AllHookPayloadsmerge + event imports (events.zig,hooks.zig,imports.zig)@embedFile+JsoncBridge.addEmbeddedPrefabin both loop and callback lifecycles (loop.zig,callback.zig;JsoncBridgegate widened so a pack can ship prefabs even when the game root has no scenes)main_template.pack_scansmodule-level var (same pattern as the existingloop_style_override), set byroot.generateand cleared after — so the ~19-arggenerateMainZigFromTemplatesignature (100+ test call sites) stays untouched.Deferred (clean seams left, not silently dropped)
hooks/scanning — the game-root hook pipeline is a three-block coordination (import +GameHookstype + per-instance init) whose ident/instance names must be unique across game + packs; it wants the<pack>__prefix first. Folded in once packs: invisible <pack>__ namespacing + local→prefixed .jsonc rewrite (save-stable name) #440 lands. (The RFC listshooks/; this slice does components/events/prefabs, matching the ticket's test spec.)<pack>__<Name>prefix +.jsonclocal→prefixed ref rewrite (packs: invisible <pack>__ namespacing + local→prefixed .jsonc rewrite (save-stable name) #440) — registered identifiers are bare today (.Worker, not.citizens__Worker). Physical paths are already pack-namespaced (files never collide on disk), but registry field / event-variant names are bare, so a pack and the game root that both defineWorkerwould collide at the registry. The block-writers already looppack_scansseparately, so packs: invisible <pack>__ namespacing + local→prefixed .jsonc rewrite (save-stable name) #440 just changes the identifier derivation.global ++ ownregistry partition /PackView(Parity: engine now accepts flat-form pack-namespaced component keys — assembler classifiers don't #652-remainder) — everything lands in one flat registry for now (no visibility partition).exposes/depends_onDAG + isolation (§6) — parsed-away for now..pluginsstill emits@import("<pack>")in the plugin list + build wiring, which a module-less light pack can't resolve yet. Converging the two registry/build APIs (dir-scan vs decl-module) is the engine/build-graph part of Parity: engine now accepts flat-form pack-namespaced component keys — assembler classifiers don't #652-remainder. This PR's tests assert on the emittedmain.zig(the discarded-names fix), not a downstream compile — as the ticket suggests ("assertion over the emitted registry block").Tests
test/pack_scan_tests.zig:scanPackcopy/scan (files land underpacks/<name>/…; stems returned; partial-dir tolerance;hooks/ignored)ComponentRegistryWithPlugins, pack event widensGameEvents+ imported through the prefix, pack prefab embedded from the prefix, and emptypack_scans= byte-unchanged no-oppack.labelleparser tests (scalar shorthand, default, ignoresexposes/depends_on, name/version validation) inplugin_manifest.zigzig build+zig build testgreen (22 test steps, exit 0).Summary by CodeRabbit
packsas reserved).main.zigemission, including correct behavior when pack data is absent and validation for invalid pack manifests.