feat(packs): scan a light pack's scripts/ into the per-state dispatch (#487) - #496
Conversation
…atch (#487) A light pack scanned components/events/prefabs/hooks but NOT scripts/, so a pack's per-frame SYSTEM had to leak into the game root (the #1 gap the pack-colony-demo eval found). This copies a pack's scripts/<state>/*.zig into `packs/<name>/scripts/` and registers them into the SAME per-state script dispatch the game root + plugins use, so a pack script's `pub fn tick(game, dt)` runs in its declared state. Key decisions: - Placement UNDER the pack dir (packs/<name>/scripts/…), NOT scripts/.plugin_<name>/. A pack has no importable module, so its script reaches its own components by relative import (../../components/foo.zig) exactly as a game-root script does. Copying the subtree under the pack — beside the components/ scanPack already copies — preserves that relative offset so the import resolves unchanged. - ScriptEntry.import_base: "scripts/" for game+plugin scripts (unchanged), "" for pack scripts whose rel_path is already a full packs/<name>/scripts/… target-relative path. AllScripts emits @import(import_base ++ rel_path). - Pack scripts carry plugin_name = pack name, so they form their own numeric-prefix scope, sort into the plugin block (after game scripts) by declaration order, and are skipped by the game-script FlowNode walk. Pack scripts are therefore lifecycle-only (tick/setup/State/drawGui); FlowNode discovery inside pack scripts is a deliberate follow-up. - Packs are skipped in the plugin-scripts loop to avoid double-scanning. - Re-add scripts/ to the `add pack` scaffold's convention dirs (#485). Existing game-root-only projects are unaffected (regression test guards the "scripts/" prefix). Adds 4 tests. Unblocks assembler#491. Claude-Session: https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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 (5)
📝 WalkthroughWalkthroughThis PR extends pack support to scan and register a light pack's ChangesPack scripts scanning support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
![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: e7b9038220
ℹ️ 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".
| } | ||
| break :blk false; | ||
| }; | ||
| if (is_pack) continue; |
There was a problem hiding this comment.
When a pack appears before a regular plugin in project.labelle and both ship scripts, this continue removes the pack from the declaration-order scan, while the later pack_entries loop registers all pack scripts only after every non-pack plugin script. Since ScriptScanner assigns plugin_index in scan-call order and sorts plugin/pack entries by that index, the later regular plugin will run before the earlier pack, breaking the existing per-state script ordering contract tied to .plugins order.
Useful? React with 👍 / 👎.
| // `PackScan.import_prefix` (`packs/<name>`) plus `/scripts`. | ||
| const pack_scripts_import_prefix = try std.fmt.allocPrint(allocator, "packs/{s}/scripts", .{e.plugin.name}); | ||
| defer allocator.free(pack_scripts_import_prefix); | ||
| try script_scan.scanPackScriptsDir(pack_scripts_dst, pack_scripts_import_prefix, e.plugin.name); |
There was a problem hiding this comment.
If a pack used to have scripts/ and a later version removes that directory entirely, copyAndScanAbs returns without touching the existing destination, but this still scans <target>/packs/<name>/scripts. Because generate only createDirPaths the target rather than recreating it, leftover copied scripts from the previous run are registered and compiled even though the source pack no longer ships them; skip the scan when the source directory is absent or prune the destination in that case.
Useful? React with 👍 / 👎.
| errdefer self.allocator.free(name_copy); | ||
| const rel_path = try std.fmt.allocPrint(self.allocator, "{s}/{s}", .{ import_prefix, entry.name }); | ||
| errdefer self.allocator.free(rel_path); | ||
| try self.addEntryWithPath(name_copy, null, &.{}, rel_path); |
There was a problem hiding this comment.
For a pack that ships a root scripts/context.zig, adding it as a normal script entry makes the existing hasContextEntry predicate see a context script even when the game has no scripts/context.zig; the template then emits the hard-coded @import("scripts/context.zig") while AllScripts skips the pack entry by name. That makes such a pack either break builds without a game context file or silently drop the pack script, so pack entries named context need to be excluded from the game-context sentinel path.
Useful? React with 👍 / 👎.
…ins, pack-script order/staleness/context) (#500) * fix(packs): address codex findings from #494/#496 — scene-lint builtins exemption, pack-script order/staleness/context Four codex-review findings merged with the packs batch (#494, #496): 1. scene_name_lint.zig — exempt engine/gfx built-in component names (e.g. `VideoComponent`, always registered by `writeComponentRegistryBlock`) from the bare-pack-component lint. Previously a bare built-in was falsely flagged and mis-suggested to `pack__VideoComponent` when a pack shipped a same-named component. Adds `builtin_component_names` + exemption in `lintSource`. 2. root.zig — scan each light pack's `scripts/` at its `.plugins` declaration-order position (interleaved with plugins) so `ScriptScanner`'s `plugin_index` reflects `.plugins` order. The old two-phase scan (all plugins, then all packs) pushed a pack declared before a plugin behind it, breaking the per-state script ordering contract. 3. root.zig — when a pack ships no `scripts/` source, prune any stale dest a prior `generate` copied and register nothing. `copyAndScanAbs` no-ops on a missing source without touching the dest, so leftover copied scripts were otherwise scanned + compiled. Extracted `scanPackScriptsAt` (guards source existence) for both #2 and #3. 4. validate.zig / registries.zig — exclude pack/plugin `context` scripts (`plugin_name != null`) from the GameContext sentinel. A pack's `scripts/context.zig` no longer flips `hasContextEntry`, and `AllScripts` keeps importing it (instead of silently dropping it). Tests: builtin-exemption lint test; pack-before-plugin ordering test; stale-dest prune + present-script tests; pack-context emission test; hasContextEntry game-vs-pack tests. Also wired `codegen/validate.zig` into root.zig's test aggregator so its tests run under `zig build test`. `zig build` + `zig build test` green (1134 tests pass). Claude-Session: https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog * fix(packs): propagate non-FileNotFound errors when probing pack scripts/ (codex P2 on #500) `scanPackScriptsAt`'s source-`scripts/` existence probe used a catch-all that treated ANY openDir failure — AccessDenied (permissions / broken mount), NotDir (`scripts` is a file), etc. — the same as a missing directory: it pruned the generated copy and silently dropped the pack's scripts, producing an incomplete build with no error. Mirror `copyAndScanAbs`'s source-root open (scanner.copyAndScanRecursive) EXACTLY: tolerate ONLY `error.FileNotFound` (prune stale dest + skip) and PROPAGATE every other error. Test: `scanPackScriptsAt propagates a non-FileNotFound probe error and does NOT prune (#500 codex)` — a pack whose `scripts` path is a file yields error.NotDir, which must propagate while the pre-existing generated copy stays intact. `zig build` + `zig build test` green (1135 tests pass). Claude-Session: https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
1 participant
What
Extends light-pack scanning so a pack's per-frame system can live INSIDE the pack. Before this,
scanPackscanned a pack'scomponents/ events/ prefabs/ hooks/but notscripts/, so a pack's per-frame logic had to leak into the game root — the #1 gap the pack-colony-demo eval found.Now a pack's
scripts/<state>/*.zigis copied intopacks/<name>/scripts/and registered into the same per-state script dispatch as game-root + plugin scripts. A pack script'spub fn tick(game, dt)runs in its declared state alongside the game's own scripts.How / key design decisions
packs/<name>/scripts/…), NOTscripts/.plugin_<name>/(the decl-module-plugin layout). A light pack has no importable Zig module, so its script reaches its own components by relative import (@import("../../components/foo.zig")) — exactly how a game-root script reachescomponents/. Copying the scripts subtree under the pack, beside thecomponents/thatscanPackalready copies, preserves that relative offset so the import resolves unchanged. Placing scripts underscripts/.plugin_<name>/would break those imports.ScriptEntry.import_base—"scripts/"for game + plugin scripts (rel_path relative to the generatedscripts/dir, behavior unchanged);""for pack scripts, whoserel_pathis already a fullpacks/<name>/scripts/…target-relative path. TheAllScriptsemitter now emits@import(import_base ++ rel_path).plugin_name = <pack>, so each pack forms its own numeric-prefix scope (per-pack duplicate-order validation), sorts into the plugin block after game scripts by declaration order (plugin_index), and is skipped by the game-script FlowNode walk..pluginsentry) so a pack that shipsscripts/isn't scanned twice.add pack/add featurescaffold subcommand #485) — re-addsscriptstoadd pack'spack_convention_dirs.Limitation
Pack scripts are lifecycle-only (
tick/setup/State/drawGui). FlowNode/PinStyle discovery inside a pack script is intentionally out of scope here (the game-script FlowNode walk skips plugin/pack-scoped entries) and can be a follow-up.Tests
Adds 4 tests in
test/pack_scan_tests.zig:scanPackScriptsDirregisters a pack script into the per-state dispatch, and the../../componentsimport target lands beside it (layout invariant).scripts/dir.AllScriptsimports a pack script verbatim frompacks/<name>/scripts/…, state-scoped.scripts/….zig buildandzig build testboth pass (1098 tests, +4).Closes #487. Unblocks assembler#491.
https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
Summary by CodeRabbit
New Features
scripts/subdirectory by default.Bug Fixes
scripts/directory.