feat(manifest): pack/feature manifest sidecar (#442) - #446
Conversation
sdl extraction step (window conformed in #437). Opts the sdl DESKTOP build into the manifest-splice path. Loop-style, desktop-only, NO params; EMPTY link beyond the single blank the enum `.link_sdl` emitted (SDL2 is linked by the backend's own build.zig). The backend_dep fragment carries the input core-diamond override (underscore `labelle_core` key, #258). Byte-identical to the enum path (diffed a generated baseline → 0 diff). zig build test green.
Extend the generated catalog into the RFC §7 pack/feature manifest, written to <game>/.labelle/manifest.json alongside flow_catalog.json. Two-tier, realm-structured + sliceable: - index (always loaded): contracts (events/enums) + a realm map (game root + each plugin) with owns / depends_on / exposes (commands vs queries, derived from FlowNode void-ness) / recipes. - per-realm detail: game realm gets full component field schemas + save policy + event payloads (light AST pass over components/ and events/); plugin realms surface event + flow-node names (payloads already live in flow_catalog.json). Everything except recipes is derived from already-scanned data, so it can't drift. Additive: a failure is logged, never fatal (same contract as the flow catalog). Deferred and called out in the schema: recipes (empty, share the scaffold source — cli #271), event emitted_by/subscribed_by cross-refs (empty arrays), and component visibility (needs pack.labelle). Part of #651, implements #442.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new manifest generator for pack/feature sidecar JSON and wires it into ChangesPack/feature manifest sidecar
SDL backend manifest and build fragments
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 a pack/feature manifest emitter (manifest.json) alongside the existing flow catalog, which maps out the game and plugin realms, their components, events, scripts, and exposed flow nodes. The implementation includes AST parsing of game components and events to extract field schemas and save policies. The review feedback highlights critical improvements for resource management and error handling: specifically, deinitializing the parsed AST in parseStructFile to prevent memory leaks, propagating OutOfMemory errors instead of swallowing them in directory parsing, and utilizing std.log.warn instead of std.debug.print for user-facing warnings in the CLI tool.
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.
| fn parseStructFile(aa: std.mem.Allocator, src: []const u8) ![]const StructDecl { | ||
| const src_z = try aa.dupeZ(u8, src); | ||
| var ast = try std.zig.Ast.parse(aa, src_z, .zig); | ||
|
|
||
| var decls: std.ArrayList(StructDecl) = .empty; | ||
| for (ast.rootDecls()) |decl_idx| { | ||
| const vd = ast.fullVarDecl(decl_idx) orelse continue; | ||
| const init_node = vd.ast.init_node.unwrap() orelse continue; | ||
|
|
||
| var buf: [2]std.zig.Ast.Node.Index = undefined; | ||
| const container = ast.fullContainerDecl(&buf, init_node) orelse continue; | ||
|
|
||
| const name_tok = vd.ast.mut_token + 1; | ||
| const name = ast.tokenSlice(name_tok); | ||
|
|
||
| var fields: std.ArrayList(Field) = .empty; | ||
| var save: ?[]const u8 = null; | ||
| for (container.ast.members) |m| { | ||
| if (ast.fullContainerField(m)) |fd| { | ||
| const fname = ast.tokenSlice(fd.ast.main_token); | ||
| const ftype_node = fd.ast.type_expr.unwrap() orelse continue; | ||
| const ftype = ast.getNodeSource(ftype_node); | ||
| try fields.append(aa, .{ | ||
| .name = try aa.dupe(u8, fname), | ||
| .zig_type = try aa.dupe(u8, std.mem.trim(u8, ftype, " \t\r\n")), | ||
| }); | ||
| continue; | ||
| } | ||
| // Not a field — look for the `pub const save = ...Saveable(.x, ...)` | ||
| // decl so the manifest can surface the save policy. | ||
| if (save == null) { | ||
| if (ast.fullVarDecl(m)) |member_vd| { | ||
| const mname = ast.tokenSlice(member_vd.ast.mut_token + 1); | ||
| if (std.mem.eql(u8, mname, "save")) { | ||
| save = extractSavePolicy(aa, ast.getNodeSource(m)) catch null; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| try decls.append(aa, .{ | ||
| .name = try aa.dupe(u8, name), | ||
| .save = save, | ||
| .fields = try fields.toOwnedSlice(aa), | ||
| }); | ||
| } | ||
| return decls.toOwnedSlice(aa); | ||
| } |
There was a problem hiding this comment.
The parsed std.zig.Ast is never deinited in parseStructFile, causing a memory leak of the AST parser's internal structures for every parsed file. Adding defer ast.deinit(aa); ensures the memory is reclaimed. Additionally, extractSavePolicy can be safely unwrapped with try to propagate OutOfMemory errors instead of swallowing them with catch null.
fn parseStructFile(aa: std.mem.Allocator, src: []const u8) ![]const StructDecl {
const src_z = try aa.dupeZ(u8, src);
var ast = try std.zig.Ast.parse(aa, src_z, .zig);
defer ast.deinit(aa);
var decls: std.ArrayList(StructDecl) = .empty;
for (ast.rootDecls()) |decl_idx| {
const vd = ast.fullVarDecl(decl_idx) orelse continue;
const init_node = vd.ast.init_node.unwrap() orelse continue;
var buf: [2]std.zig.Ast.Node.Index = undefined;
const container = ast.fullContainerDecl(&buf, init_node) orelse continue;
const name_tok = vd.ast.mut_token + 1;
const name = ast.tokenSlice(name_tok);
var fields: std.ArrayList(Field) = .empty;
var save: ?[]const u8 = null;
for (container.ast.members) |m| {
if (ast.fullContainerField(m)) |fd| {
const fname = ast.tokenSlice(fd.ast.main_token);
const ftype_node = fd.ast.type_expr.unwrap() orelse continue;
const ftype = ast.getNodeSource(ftype_node);
try fields.append(aa, .{
.name = try aa.dupe(u8, fname),
.zig_type = try aa.dupe(u8, std.mem.trim(u8, ftype, " \t\r\n")),
});
continue;
}
// Not a field — look for the `pub const save = ...Saveable(.x, ...)`
// decl so the manifest can surface the save policy.
if (save == null) {
if (ast.fullVarDecl(m)) |member_vd| {
const mname = ast.tokenSlice(member_vd.ast.mut_token + 1);
if (std.mem.eql(u8, mname, "save")) {
save = try extractSavePolicy(aa, ast.getNodeSource(m));
}
}
}
}
try decls.append(aa, .{
.name = try aa.dupe(u8, name),
.save = save,
.fields = try fields.toOwnedSlice(aa),
});
}
return decls.toOwnedSlice(aa);
}
| fn parseStructDir( | ||
| aa: std.mem.Allocator, | ||
| game_dir: []const u8, | ||
| folder: []const u8, | ||
| names: []const []const u8, | ||
| ) ![]const StructDecl { | ||
| const io = config.globalIo(); | ||
| var list: std.ArrayList(StructDecl) = .empty; | ||
| for (names) |name| { | ||
| const rel = try std.fmt.allocPrint(aa, "{s}.zig", .{name}); | ||
| const path = try std.fs.path.join(aa, &.{ game_dir, folder, rel }); | ||
| const src = std.Io.Dir.cwd().readFileAlloc(io, path, aa, .limited(2 * 1024 * 1024)) catch continue; | ||
| const decls = parseStructFile(aa, src) catch continue; | ||
| for (decls) |d| try list.append(aa, d); | ||
| } | ||
| return list.toOwnedSlice(aa); | ||
| } |
There was a problem hiding this comment.
Silently swallowing OutOfMemory errors with catch continue is dangerous as it can mask critical system issues and lead to hard-to-debug partial generation states. Instead, explicitly catch and propagate OutOfMemory while continuing on other non-fatal errors (such as missing files or syntax errors) to preserve the graceful-degradation contract.
fn parseStructDir(
aa: std.mem.Allocator,
game_dir: []const u8,
folder: []const u8,
names: []const []const u8,
) ![]const StructDecl {
const io = config.globalIo();
var list: std.ArrayList(StructDecl) = .empty;
for (names) |name| {
const rel = try std.fmt.allocPrint(aa, "{s}.zig", .{name});
const path = try std.fs.path.join(aa, &.{ game_dir, folder, rel });
const src = std.Io.Dir.cwd().readFileAlloc(io, path, aa, .limited(2 * 1024 * 1024)) catch |err| switch (err) {
error.OutOfMemory => return error.OutOfMemory,
else => continue,
};
const decls = parseStructFile(aa, src) catch |err| switch (err) {
error.OutOfMemory => return error.OutOfMemory,
else => continue,
};
for (decls) |d| try list.append(aa, d);
}
return list.toOwnedSlice(aa);
}
| ) catch |err| { | ||
| std.debug.print("labelle-assembler: manifest sidecar emission failed: {s}\n", .{@errorName(err)}); | ||
| }; |
There was a problem hiding this comment.
Using std.debug.print for user-facing warnings in a CLI tool violates the general rule to use standard logging facilities (such as std.log.warn) or write directly to stderr. Replacing it with std.log.warn ensures consistent output formatting and integrates correctly with standard logging configurations.
) catch |err| {
std.log.warn("labelle-assembler: manifest sidecar emission failed: {s}", .{@errorName(err)});
};
References
- In CLI tools, use standard logging facilities (e.g. std.log.warn) or write to stderr for user-facing warnings, rather than using debug-specific print functions (e.g. std.debug.print).
![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/manifest.zig`:
- Around line 184-199: parseStructDir currently skips a name entirely when
readFileAlloc or parseStructFile fails, so unreadable or partially unparsable
files disappear from the index. Update parseStructDir to preserve the scanned
name by creating a fallback StructDecl (or equivalent name-only entry) from name
when the file cannot be read or parsed, and still append it to list instead of
using catch continue. Keep the existing flow in parseStructDir, but ensure
downstream consumers of d.components and d.game_events can still see the
original scanned component/event name even when AST detail is missing.
🪄 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: 3826387b-b018-4c85-92cc-d92a80bff73d
📒 Files selected for processing (6)
backends/sdl/backend.manifest.zonbackends/sdl/build_fragments/backend_dep.txtbackends/sdl/build_fragments/link.txtsrc/flow_catalog/json_writer.zigsrc/manifest.zigsrc/root.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: 2ef6d78e70
ℹ️ 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 (d.plugin_events) |e| { | ||
| if (!first) try w.writeAll(", "); | ||
| try jw.writeJsonString(w, e.event_name); |
There was a problem hiding this comment.
When two non-game realms expose the same event name (for example engine.tick and a plugin tick, or two plugins both exporting collision_begin), this flattens every plugin/engine event to the bare event_name. Other event consumers in this repo keep the realm-qualified form to avoid collisions, so contracts.events becomes ambiguous and an agent cannot tell which realm owns the contract entry.
Useful? React with 👍 / 👎.
| var ast = try std.zig.Ast.parse(aa, src_z, .zig); | ||
|
|
||
| var decls: std.ArrayList(StructDecl) = .empty; | ||
| for (ast.rootDecls()) |decl_idx| { |
There was a problem hiding this comment.
For any component or event module that contains an additional top-level container helper such as const Options = struct { ... } or pub const Clip = enum { ... }, this loop appends that helper as another StructDecl because it walks every root declaration instead of the file-stem component/event declaration. The rest of the assembler registers only the file-stem Pascal decl from components/{name}.zig or events/{name}.zig, so the manifest will advertise helper types as real components/events.
Useful? React with 👍 / 👎.
| if (d.cfg.plugins.len > 0) try w.writeAll(","); | ||
| try w.writeAll("\n"); | ||
|
|
||
| for (d.cfg.plugins, 0..) |plugin, pi| { |
There was a problem hiding this comment.
When the engine package exposes lifecycle events, discoverPluginEvents returns them as plugin_events under the engine realm and writeContractEvents includes them, but realm emission here only iterates d.cfg.plugins. A project can therefore get contract entries for engine events without any index.realms or detail realm that owns them, so consumers cannot fetch the referenced engine realm or see its event list.
Useful? React with 👍 / 👎.
| // command == void impl; query == reporter (non-void). | ||
| if (n.is_void != commands) continue; | ||
| if (!first) try w.writeAll(", "); | ||
| try jw.writeJsonString(w, n.node_name); |
There was a problem hiding this comment.
When a game has FlowNodes in more than one script, the script module path is part of the public name used by the registry/catalog, but this writes only the bare declaration name. Two scripts can both expose spawn, and even without a collision the manifest does not tell consumers which script-qualified node to call, so index.realms[game].exposes can become ambiguous or non-resolvable.
Useful? React with 👍 / 👎.
| const decls = parseStructFile(aa, src) catch continue; | ||
| for (decls) |d| try list.append(aa, d); |
There was a problem hiding this comment.
If a valid component/event file does not parse into a literal top-level struct here (for example it aliases a generic component type, uses unsupported newer Zig syntax, or is temporarily unreadable), this continue drops the scanned name entirely. The generated registries still import the file-stem decl from component_names/event_names, so the manifest can incorrectly report that an existing component or event is absent instead of falling back to the scanned name with an empty schema.
Useful? React with 👍 / 👎.
…ar (#442) - parseStructFile: deinit the parsed Ast (per-file leak); propagate OOM from extractSavePolicy instead of swallowing it via `catch null`. - parseStructDir: propagate OutOfMemory; degrade non-fatal read failures and no-match files to a name-only StructDecl (file-stem Pascal name, empty fields) so the manifest still lists what the registries import. - parseStructDir: emit only the file-stem Pascal decl per file, so helper containers (`const Options`/`pub const Clip`) no longer leak as phantom components/events. - contracts.events: realm-qualify entries (`engine.tick`, `box2d.collision_begin`, `game.<Event>`) to match the toolchain's dotted qualified form and avoid cross-realm name collisions. - Emit a dedicated `engine` realm (index + detail) when engine lifecycle events are discovered, so `engine.<event>` contract entries resolve to a realm that actually appears. - Game-realm exposes: script-qualify FlowNode commands/queries (`flows.hit_counter.spawn`) via the scriptModuleLabel dotted form so two scripts exposing the same bare name stay distinct. - root.zig: use std.log.warn (not std.debug.print) for the manifest sidecar emission-failed warning. - Tests: name-only fallback + helper-container exclusion (parseStructDir); engine realm emission + realm-qualified contracts + script-qualified exposes (writeManifestJson). Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Synced with main + review passMerge: Fetched and merged Review comments (9 total): all of them were already resolved by the earlier commit
Nothing skipped — every finding was actionable and is covered, with regression tests ( Verification (Zig 0.16.0):
CI re-triggered by the merge push and is running. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8167ab6c89
ℹ️ 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".
| try w.writeAll("["); | ||
| for (items, 0..) |s, i| { | ||
| if (i > 0) try w.writeAll(", "); | ||
| try jw.writeJsonString(w, s.name); |
There was a problem hiding this comment.
When a project has two valid state-scoped or nested game scripts with the same basename (for example scripts/menu/05_update.zig and scripts/playing/05_update.zig), the scanner keeps them distinct by rel_path and generated code imports them via the path-derived identifier, but this index emits only the stripped s.name. That makes index.realms[game].owns.scripts contain duplicate ambiguous names even though the manifest is meant to be the always-loaded ownership map; use rel_path or a path-qualified name here so consumers can resolve the script without fetching and disambiguating detail entries.
Useful? React with 👍 / 👎.
| var first = true; | ||
| for (d.game_events) |e| { | ||
| if (!first) try w.writeAll(", "); | ||
| try writeQualifiedJsonString(w, GAME_REALM, e.name); |
There was a problem hiding this comment.
For game events whose file stem differs from the Pascal payload type (the normal events/worker_sleep_start.zig → WorkerSleepStart convention), generated GameEvents uses eventVariantName(name) as the actual union tag, but this contract entry advertises game.WorkerSleepStart. An agent following contracts.events will try to subscribe/emit a name that is not the generated event tag, while plugin/engine entries already use their real event tags; keep the game contract name based on the scanned event stem/basename instead of StructDecl.name.
Useful? React with 👍 / 👎.
| .dep_name = "labelle_sdl", | ||
| .loop_style = .loop, | ||
| .main_loop_template = "templates/desktop.txt", | ||
| .build_fragments = .{ .backend_dep = "build_fragments/backend_dep.txt", .link = "build_fragments/link.txt" }, |
There was a problem hiding this comment.
This new SDL manifest is what validateProviderContracts reads before generation, but without a non-empty .capabilities set the validator takes the back-compat path and only warns for missing requirements. For example, selecting SDL for a wasm/android project or requiring a raw GUI adapter won't fail at resolve time and instead falls through to later template/codegen failures; add SDL's actual capability set (as the v2 SDL fixture does) so unsupported requirements are rejected early.
Useful? React with 👍 / 👎.
| // Game scripts only — plugin-shipped scripts belong to their plugin. | ||
| var game_scripts: std.ArrayList(ScriptEntry) = .empty; | ||
| for (script_entries) |e| { | ||
| if (e.plugin_name != null) continue; |
There was a problem hiding this comment.
When a plugin ships its own scripts/ directory, scanPluginDir adds those entries and the generated AllScripts block runs them, but this filter drops every plugin-owned script from the manifest and the plugin realm writer never adds a scripts list. That leaves plugin controller scripts with no owning realm in the sidecar, so a consumer inspecting the manifest before editing a plugin cannot see the existing script or its order/state scope; group these entries under their plugin_name realm instead of discarding them.
Useful? React with 👍 / 👎.
1 participant
What
Extends the generated catalog into the pack/feature manifest from RFC §7 (Packs initiative). Emitted as
<game>/.labelle/manifest.json, next to the existingflow_catalog.json— fully additive, no change to flow-catalog generation.The manifest answers the questions an agent (or human) asks before adding a feature: which realm owns this · what shapes do I touch · what already exists · what may I call cross-pack · what's the recipe. For today's codebase (no packs yet) the game root and each plugin are treated as "realms."
Shape — two-tier, realm-structured + sliceable
index(always loaded):contracts(event + enum vocabulary) plus arealmsmap. Each realm carriesowns(names),depends_on,exposes(commandsvsqueries), andrecipes.realms(per-realm detail, fetch the one in play): the game realm gets full component field schemas + save policy and event payloads; plugin realms surface event + flow-node names (payloads already live inflow_catalog.json).Derived (drift-free)
Everything except recipes is generated from data the assembler already scans:
components/*.zigandevents/*.zig(parseStructFile).exposes(command = void impl / query = reporter) from the already-discoveredPluginFlowNodes; plugin event names fromPluginEvents.depends_on(game realm) = plugins inproject.labelle.Deferred (called out in the schema, not silently dropped)
recipes— empty array on every realm; the one non-derivable field. Shares the scaffold's template source (cli feat(backends): relocate windowless-SDL desktop gamepad source out of core into assembler (core#28) #271) so recipe and scaffold stay one definition.emitted_by/subscribed_byevent cross-refs — empty arrays. AST call-site extraction across every script is a larger pass; the shape is in place so a follow-up fills it with no schema bump.visibility(pack vs public) — comes frompack.labelle, which doesn't exist yet; omitted rather than guessed.Verification
zig build+zig build testpass.src/manifest.zig: component field/save parsing, event payload parsing, save-policy extraction, fullwriteManifestJsonround-trip throughstd.json(index + detail), and an empty-project sidecar emission.generateagainstflying-platform-labelle: produced a 37 KB manifest — 15 realms (game + 14 plugins), 91 components with field schemas, 15 events with payloads, 66 ordered scripts, valid JSON.Part of #651, implements #442.
Summary by CodeRabbit
New Features
manifest.jsonsidecar generated alongside existing build outputs to provide richer project and gameplay metadata.Bug Fixes