feat(#271): add add pack / add feature scaffold subcommand - #485
Conversation
Scaffolds the two authoring units the Packs RFC (§7) defines:
labelle-assembler add pack <name>
labelle-assembler add feature <kind> <name> (kind: need|role|status)
`add pack` creates packs/<name>/ with the convention subdirs
(components/ events/ scripts/ prefabs/ hooks/, each with a .gitkeep) and a
`pack.labelle` (`.name`, `.manifest_version = 1`, scalar
`.convention_dirs = .copy_and_scan`). Refuses an existing dir.
`add feature <kind> <name>` scaffolds a feature-unit in the game root: a
`components/<name>.zig` (Saveable component) plus a
`scripts/playing/xx_<name>.zig` stub. Per kind:
need — value in [0,1] + a decay script that flags threshold crossings
and shows the standard need_threshold_crossed emit (TODO)
role — role marker + per-frame behavior stub
status — transient status flag + overlay-driver stub
Templates are minimal, AST-check-clean, and double as the recipe source
the pack manifest (#442) will surface. Refuses to overwrite either file.
Follows the `init` split: the CLI parses `add ...` and forwards it to this
subcommand, which owns the templating. Bumps PROTOCOL_VERSION to 4.
Part of #651.
📝 WalkthroughWalkthroughAdds a new ChangesAdd Subcommand Implementation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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 the add subcommand to the labelle-assembler binary, allowing users to scaffold packs and feature-units (needs, roles, and statuses) with pre-defined templates. The protocol version has been bumped to 4, and the new subcommand is integrated into the main entry point. Feedback on the changes suggests improving the robustness of the toTypeName helper by handling digit-prefixed inputs to ensure valid Zig identifiers, and refactoring the writeExclusiveData helper to return errors instead of calling std.process.exit(1) directly to enhance testability.
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.
| pub fn toTypeName(allocator: std.mem.Allocator, name: []const u8) ![]u8 { | ||
| var out: std.ArrayList(u8) = .empty; | ||
| errdefer out.deinit(allocator); | ||
| var at_word_start = true; | ||
| for (name) |c| { | ||
| if (c == '_') { | ||
| at_word_start = true; | ||
| continue; | ||
| } | ||
| if (at_word_start and c >= 'a' and c <= 'z') { | ||
| try out.append(allocator, c - ('a' - 'A')); | ||
| } else { | ||
| try out.append(allocator, c); | ||
| } | ||
| at_word_start = false; | ||
| } | ||
| return out.toOwnedSlice(allocator); | ||
| } |
There was a problem hiding this comment.
To ensure robustness and compliance with the general rules, toTypeName should handle cases where the input string starts with a digit by prefixing the resulting Zig identifier with an underscore. Since Zig identifiers cannot start with a digit, this prevents generating invalid code if the function is called with such inputs in other contexts.
pub fn toTypeName(allocator: std.mem.Allocator, name: []const u8) ![]u8 {
var out: std.ArrayList(u8) = .empty;
errdefer out.deinit(allocator);
if (name.len > 0 and name[0] >= '0' and name[0] <= '9') {
try out.append(allocator, '_');
}
var at_word_start = true;
for (name) |c| {
if (c == '_') {
at_word_start = true;
continue;
}
if (at_word_start and c >= 'a' and c <= 'z') {
try out.append(allocator, c - ('a' - 'A'));
} else {
try out.append(allocator, c);
}
at_word_start = false;
}
return out.toOwnedSlice(allocator);
}
References
- Zig identifiers must not start with a digit. When converting strings (such as file paths) to Zig identifiers, prefix the result with an underscore if the input starts with a digit to ensure the generated code is valid.
| fn writeExclusiveData(io: std.Io, cwd: std.Io.Dir, path: []const u8, data: []const u8) void { | ||
| cwd.writeFile(io, .{ | ||
| .sub_path = path, | ||
| .data = data, | ||
| .flags = .{ .exclusive = true }, | ||
| }) catch |err| switch (err) { | ||
| error.PathAlreadyExists => { | ||
| std.log.err("labelle-assembler add: '{s}' already exists — refusing to overwrite", .{path}); | ||
| std.process.exit(1); | ||
| }, | ||
| else => { | ||
| std.log.err("labelle-assembler add: could not write '{s}': {s}", .{ path, @errorName(err) }); | ||
| std.process.exit(1); | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Calling std.process.exit(1) directly inside helper functions like writeExclusiveData limits their reusability and testability. Returning standard Zig errors (e.g., propagating error.PathAlreadyExists) allows the caller to handle the exit logic, and enables unit tests to directly assert error conditions without aborting the test runner.
fn writeExclusiveData(io: std.Io, cwd: std.Io.Dir, path: []const u8, data: []const u8) !void {
cwd.writeFile(io, .{
.sub_path = path,
.data = data,
.flags = .{ .exclusive = true },
}) catch |err| switch (err) {
error.PathAlreadyExists => {
std.log.err("labelle-assembler add: \'{s}\' already exists — refusing to overwrite", .{path});
return err;
},
else => {
std.log.err("labelle-assembler add: could not write \'{s}\": {s}", .{ path, @errorName(err) });
return err;
},
};
}
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/add_cmd.zig`:
- Around line 240-245: scaffoldFeature is swallowing createDirPath failures for
components_dir and playing_dir, which hides the real cause and lets later writes
fail with misleading errors. Update the directory creation flow in
scaffoldFeature to handle these failures the same way scaffoldPack does:
preserve the error from cwd.createDirPath, emit a clear diagnostic with the
actual path/error, and stop execution instead of continuing into
writeExclusiveData.
- Around line 86-92: The unknown FeatureKind branch in cmdAdd currently logs an
error and exits without showing the add_usage hint like the other parse-error
paths. Update the unknown-kind handling in add_cmd.zig around
std.meta.stringToEnum(FeatureKind, kind_str) to also write the add_usage block
to stderr before exiting, matching the behavior used for missing
target/pack/feature and unexpected argument errors.
🪄 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: 5da428a3-5c55-44a3-963e-acc742f78723
📒 Files selected for processing (2)
src/add_cmd.zigsrc/main.zig
![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: 9df8b1abcd
ℹ️ 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".
| defer allocator.free(file); | ||
| const path = try std.fs.path.join(allocator, &.{ components_dir, file }); | ||
| defer allocator.free(path); | ||
| writeExclusiveData(io, cwd, path, aw.written()); |
There was a problem hiding this comment.
When scripts/playing/xx_<name>.zig already exists but components/<name>.zig does not, this write succeeds and the later exclusive script write exits, so the command reports failure after leaving a half-created scaffold; rerunning then fails on the newly created component as well. Preflight both target paths before writing, or roll back the component on script failure, so the documented refusal to overwrite either file stays atomic.
Useful? React with 👍 / 👎.
| \\ defer view.deinit(); | ||
| \\ | ||
| \\ while (view.next()) |entity| {{ | ||
| \\ const need = game.getComponent(entity, {s}) orelse continue; |
There was a problem hiding this comment.
For any scaffolded feature, this generated script receives the assembled game, whose component API is exposed through game.ecs_backend (the view call just above uses that path, and existing scripts in this repo do the same). game.getComponent is not a Game method, so add feature need/role/status produces a project that fails to compile as soon as the generated script is type-checked; use game.ecs_backend.getComponent(...) here and in the equivalent role/status templates.
Useful? React with 👍 / 👎.
|
|
||
| // ─── pack ────────────────────────────────────────────────────────────── | ||
|
|
||
| const pack_convention_dirs = [_][]const u8{ "components", "events", "scripts", "prefabs", "hooks" }; |
There was a problem hiding this comment.
The scaffold advertises and creates packs/<name>/scripts/, but the generator's scanPack only copies/scans components, events, prefabs, and hooks (src/root.zig lines 652-660). When a user follows the scaffold and adds a pack script there, it is silently ignored by generate, so either this directory should not be created yet or pack script scanning needs to be wired at the same time.
Useful? React with 👍 / 👎.
| const components_dir = try std.fs.path.join(allocator, &.{ root, "components" }); | ||
| defer allocator.free(components_dir); | ||
| cwd.createDirPath(io, components_dir) catch {}; | ||
| const playing_dir = try std.fs.path.join(allocator, &.{ root, "scripts", "playing" }); |
There was a problem hiding this comment.
In projects scaffolded by init, ProjectConfig.states defaults to "running", and ScriptScanner.scanDir ignores first-level script directories that are not in the configured state list. Because add feature always writes the script under scripts/playing/, a default project can successfully scaffold a feature whose script is then silently omitted from generated AllScripts; write it under the default state or update the project states as part of the scaffold.
Useful? React with 👍 / 👎.
| writeExclusiveData(io, cwd, manifest_path, aw.written()); | ||
| } | ||
|
|
||
| std.log.info("labelle-assembler: scaffolded pack '{s}' in {s}/", .{ name, pack_dir }); |
There was a problem hiding this comment.
A light pack only participates in generation when project.labelle declares it in .plugins (for example with repo = "@packs/<name>"), and generate() discovers packs by iterating cfg.plugins. This command only writes packs/<name>/ and reports success, so a default project that runs add pack citizens gets a valid-looking pack that generate completely ignores; update project.labelle or at least emit that required next step.
Useful? React with 👍 / 👎.
…y work
Substantive (codex):
- Auto-register a scaffolded pack in project.labelle .plugins as
.{ .name, .repo = "@packs/<name>" } so generate() (which discovers packs
by iterating cfg.plugins) picks it up; falls back to a printed next-step
when project.labelle is absent/unparseable/already has it. A pack is no
longer left dead.
- Stop scaffolding packs/<name>/scripts/ — scanPack only scans
components/events/prefabs/hooks, so pack scripts are silently ignored
today (follow-up: wire pack-script scanning, then re-add the dir).
- Feature scripts now land under scripts/running/ (the default
ProjectConfig.states) instead of scripts/playing/, which ScriptScanner
silently drops in a default project.
- Feature component lookup goes through game.ecs_backend.getComponent,
consistent with the ecs_backend.view call.
- add feature preflights BOTH target files before writing either, so a
partial pre-existing scaffold no longer leaves a half-written result.
Quality (CodeRabbit + Gemini):
- Unknown feature kind now prints the usage/valid-kinds hint.
- Feature dir-creation errors are reported + abort instead of swallowed.
- toTypeName prefixes '_' when the name starts with a digit.
- writeExclusiveData / preflightAbsent return errors instead of calling
std.process.exit, and emit diagnostics to stderr (repo convention) so
they're unit-testable; callers map the error to the exit code.
Tests: digit-prefix, unknown-kind path, preflight all-or-nothing,
returning write helper, and pack auto-registration (empty + populated
.plugins). Updated existing scaffold tests for scripts/running and the
dropped pack scripts/ dir.
Review findings addressed (pushed 8467601)All in Substantive (codex) — scaffold now produces working output
Quality
TestsAdded: digit-prefix |
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/add_cmd.zig (1)
285-321: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDedup only checks the
@packs/<name>repo string.
registerPackInProjectdecides "already registered" solely by searching for"@packs/<name>". Ifproject.labellealready declares this pack under any other repo form (e.g.local:packs/<name>after a manual edit), the check misses it and a second entry with the same.nameis appended, yielding two plugins with the same name in.plugins. Since the tool always writes@packs/this is an edge case, but consider also short-circuiting when a plugin.name = "<name>"already appears.🤖 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/add_cmd.zig` around lines 285 - 321, The dedup check in registerPackInProject only looks for the `@packs/`<name> repo string, so it can miss an existing plugin entry with the same .name under a different repo form and append a duplicate. Update the registration guard in registerPackInProject to also detect any existing .name = "<name>" entry in src before building the new .plugins item, so the function short-circuits on either match and avoids duplicate plugins.
🤖 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 `@src/add_cmd.zig`:
- Around line 285-321: The dedup check in registerPackInProject only looks for
the `@packs/`<name> repo string, so it can miss an existing plugin entry with the
same .name under a different repo form and append a duplicate. Update the
registration guard in registerPackInProject to also detect any existing .name =
"<name>" entry in src before building the new .plugins item, so the function
short-circuits on either match and avoids duplicate plugins.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84676019a9
ℹ️ 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".
| // aren't a declared state — so a script under `scripts/playing/` would | ||
| // never run in a default project (chatgpt-codex review). Target | ||
| // `scripts/running/` so the scaffolded script is picked up out of the box. | ||
| const script_dir = try std.fs.path.join(allocator, &.{ root, "scripts", "running" }); |
There was a problem hiding this comment.
This still drops scaffolded scripts for projects that customize .states to omit the default running state (for example .states = .{ "playing" }): add feature writes scripts/running/xx_<name>.zig, but ScriptScanner.scanDir skips first-level script directories that are not in ProjectConfig.states, so the generated feature component exists while its behavior script is silently omitted. Read project.labelle and use a configured state (or emit a root-level/global script) instead of hard-coding running.
Useful? React with 👍 / 👎.
| // Already registered? Leave the file untouched. | ||
| if (std.mem.indexOf(u8, src, repo_ref) != null) { | ||
| std.log.info(" pack '{s}' is already registered in project.labelle .plugins", .{name}); |
There was a problem hiding this comment.
When project.labelle already declares a plugin or pack with the same .name but a different repo, this only checks for the "@packs/<name>" repo string and then inserts a second .plugins entry with the same name. That duplicate name can break generation: light-pack filtering is by plugin name, so adding a local pack named like an existing decl-module plugin causes the original plugin to be filtered out as though it were the light pack. Check for an existing .name = "<name>" (preferably by parsing the config) before auto-registering.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| // Find the `.plugins = .{` opener and insert right after the `{`. |
There was a problem hiding this comment.
Because this searches the raw file for the first .plugins substring, add pack can rewrite a comment or string instead of the actual ZON field. For a valid project that omits .plugins but documents it in a comment such as // .plugins = .{} ..., the insertion adds real ZON text into that comment example and leaves project.labelle malformed while reporting success; parse/validate the config edit or at least ignore comments/strings before writing.
Useful? React with 👍 / 👎.
| if (name.len == 0) return false; | ||
| if (name[0] < 'a' or name[0] > 'z') return false; | ||
| for (name) |c| { | ||
| const ok = (c >= 'a' and c <= 'z') or (c >= '0' and c <= '9') or c == '_'; |
There was a problem hiding this comment.
This accepts arbitrarily long feature names, but the generator derives component registry declarations with pathToPascal into a fixed 128-byte buffer. For a name whose PascalCase type exceeds that limit, add feature writes a component exporting the full type name while generated main.zig later imports the truncated type, so the scaffolded project fails to build. Reject names whose derived type would exceed the generator limit, or derive the template name through the same bounded path.
Useful? React with 👍 / 👎.
…atch (#487) (#496) 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
1 participant
Part of the Packs initiative (umbrella labelle-toolkit/labelle-engine#651, RFC-packs §7). This is the assembler half of labelle-cli#271 — it owns the file templating; the CLI PR (labelle-toolkit/labelle-cli#272) is the thin forwarder.
What
A new
addsubcommand mirroring theinitsplit:add pack <name>Creates
packs/<name>/with the convention subdirs (components/ events/ scripts/ prefabs/ hooks/, each with a.gitkeep) and apack.labelle:.{ .name = "<name>", .manifest_version = 1, .convention_dirs = .copy_and_scan, }Refuses if
packs/<name>/already exists. The manifest parses cleanly throughplugin_manifest.loadPackFromDir(asserted in a test).add feature <kind> <name>Scaffolds a feature-unit in the game root — a
components/<name>.zig(Saveable component) + ascripts/playing/xx_<name>.zigstub — per the RFC "feature = events + component + script" shape:valuein[0,1](.saveable) + a decay script that flags yellow/red threshold crossings and shows the exactneed_threshold_crossedemit as a TODO (the event lives once in thecitizenspack, RFC §6)..saveable) + a per-frame behavior stub..transient) + an overlay-driver stub.<kind>is a hardcoded set behind an explicit arg; unknown kinds are rejected with the valid set.<name>must be a lowercase identifier (derived to a PascalCase type). Both files use exclusive writes — refuses to overwrite.Templates are deliberately minimal and correct: per RFC §7 they double as the "recipe source" the pack manifest (#442) will surface, and every generated file passes
zig ast-check.Bumps
PROTOCOL_VERSION3 → 4.Verified
zig build+zig build testgreen (new tests: pack tree + parseable manifest, feature need component+script, status transient policy, name validation, PascalCase derivation, overwrite refusal).add pack,add feature {need,role,status}, and all error paths (existing dir → 1, unknown kind → 2, bad name → 2, existing file → 1). Every generated.zigpasseszig ast-check.Part of labelle-toolkit/labelle-engine#651.
Summary by CodeRabbit
New Features
addCLI command to scaffold packs and feature components.Documentation
--helpoutput to includeadd pack <name>andadd feature <kind> <name>.Bug Fixes
Tests