feat: labelle plugins listing + license/author metadata (#300) - #306
Conversation
Asset Plugins Phase 2, CLI side. Adds a `labelle plugins [dir]` subcommand that reads each declared plugin's `plugin.labelle` and prints an aligned table of plugin name, declared version, license, and author. - src/cli/plugins.zig: `readPluginMeta` parses name/manifest_version and the optional license/author (ignore_unknown_fields; tolerates a missing or malformed manifest by falling back to `-`), `resolvePluginDir` mirrors the local/remote resolution used by the lockfile writer, and a pure `renderTable` for testable aligned output. - Wired into cli.zig dispatch (standalone, reads project.labelle itself) and surfaced in `labelle help`. - zspec tests: reader parses full metadata, tolerates missing license/author + missing file; renderer emits header + rows with `-` placeholders. Generate-time `depends_on_resources` validation is delivered assembler -side (labelle-assembler#576); the CLI shells out to the assembler for `generate` and never parses the merged resource set, so #300's CLI deliverable is this provenance-surfacing listing. Closes #300 Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request implements the plugins command for the labelle CLI, allowing users to list attached plugins alongside their version, license, and author metadata extracted from plugin.labelle manifests. The feedback focuses on improving memory safety and error handling in Zig by propagating OutOfMemory errors instead of swallowing them in readPluginMeta and resolvePluginDir, using errdefer to clean up partial allocations, and updating the command's call sites and test cases accordingly.
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 readPluginMeta(allocator: std.mem.Allocator, plugin_dir: []const u8) ?PluginMeta { | ||
| const manifest_path = std.fs.path.join(allocator, &.{ plugin_dir, "plugin.labelle" }) catch return null; | ||
| defer allocator.free(manifest_path); | ||
|
|
||
| const raw = std.Io.Dir.cwd().readFileAlloc(config.globalIo(), manifest_path, allocator, .limited(64 * 1024)) catch |err| { | ||
| // FileNotFound is the common, legal "no manifest" case — stay | ||
| // silent. Anything else is worth a diagnostic breadcrumb. | ||
| if (err != error.FileNotFound) { | ||
| std.debug.print("labelle: warning: could not read '{s}': {s}\n", .{ manifest_path, @errorName(err) }); | ||
| } | ||
| return null; | ||
| }; | ||
| defer allocator.free(raw); | ||
|
|
||
| const raw_z = allocator.dupeZ(u8, raw) catch return null; | ||
| defer allocator.free(raw_z); | ||
|
|
||
| const parsed = std.zon.parse.fromSliceAlloc(ZonPluginMeta, allocator, raw_z, null, .{ | ||
| .ignore_unknown_fields = true, | ||
| }) catch |err| { | ||
| std.debug.print("labelle: warning: could not parse '{s}': {s} — listing it without license/author\n", .{ manifest_path, @errorName(err) }); | ||
| return null; | ||
| }; | ||
| // The parser deep-copies strings; free the whole parsed value after | ||
| // re-duping the fields we keep so ownership is a single, simple | ||
| // `PluginMeta.deinit`. | ||
| defer std.zon.parse.free(allocator, parsed); | ||
|
|
||
| const name = allocator.dupe(u8, parsed.name) catch return null; | ||
| const license = if (parsed.license) |l| (allocator.dupe(u8, l) catch null) else null; | ||
| const author = if (parsed.author) |a| (allocator.dupe(u8, a) catch null) else null; | ||
|
|
||
| return PluginMeta{ | ||
| .name = name, | ||
| .manifest_version = parsed.manifest_version, | ||
| .license = license, | ||
| .author = author, | ||
| .allocator = allocator, | ||
| }; | ||
| } |
There was a problem hiding this comment.
In Zig, system-level failures like OutOfMemory should always be propagated rather than swallowed as a generic failure or missing file. Swallowing OutOfMemory makes debugging memory pressure issues extremely difficult.
Additionally, when initializing a resource that will have its ownership transferred later, we should use errdefer to ensure that any successfully allocated fields (like name or license) are properly deallocated if a subsequent allocation fails before the PluginMeta struct is fully constructed and returned.
Let's update readPluginMeta to return !?PluginMeta, propagate OutOfMemory errors, and use errdefer for safe cleanup.
pub fn readPluginMeta(allocator: std.mem.Allocator, plugin_dir: []const u8) !?PluginMeta {
const manifest_path = std.fs.path.join(allocator, &.{ plugin_dir, "plugin.labelle" }) catch |err| {
if (err == error.OutOfMemory) return error.OutOfMemory;
return null;
};
defer allocator.free(manifest_path);
const raw = std.Io.Dir.cwd().readFileAlloc(config.globalIo(), manifest_path, allocator, .limited(64 * 1024)) catch |err| {
if (err == error.OutOfMemory) return error.OutOfMemory;
// FileNotFound is the common, legal "no manifest" case — stay
// silent. Anything else is worth a diagnostic breadcrumb.
if (err != error.FileNotFound) {
std.debug.print("labelle: warning: could not read '{s}': {s}\n", .{ manifest_path, @errorName(err) });
}
return null;
};
defer allocator.free(raw);
const raw_z = allocator.dupeZ(u8, raw) catch |err| {
if (err == error.OutOfMemory) return error.OutOfMemory;
return null;
};
defer allocator.free(raw_z);
const parsed = std.zon.parse.fromSliceAlloc(ZonPluginMeta, allocator, raw_z, null, .{
.ignore_unknown_fields = true,
}) catch |err| {
if (err == error.OutOfMemory) return error.OutOfMemory;
std.debug.print("labelle: warning: could not parse '{s}': {s} — listing it without license/author\n", .{ manifest_path, @errorName(err) });
return null;
};
// The parser deep-copies strings; free the whole parsed value after
// re-duping the fields we keep so ownership is a single, simple
// `PluginMeta.deinit`.
defer std.zon.parse.free(allocator, parsed);
const name = try allocator.dupe(u8, parsed.name);
errdefer allocator.free(name);
const license = if (parsed.license) |l| try allocator.dupe(u8, l) else null;
errdefer if (license) |l| allocator.free(l);
const author = if (parsed.author) |a| try allocator.dupe(u8, a) else null;
return PluginMeta{
.name = name,
.manifest_version = parsed.manifest_version,
.license = license,
.author = author,
.allocator = allocator,
};
}
References
- In Zig, when initializing a resource that will have its ownership transferred later, use errdefer (potentially combined with an ownership flag) to ensure the resource is properly deinitialized if an error occurs before the transfer is complete.
| if (resolvePluginDir(a, project_dir, dep)) |plugin_dir| { | ||
| if (readPluginMeta(a, plugin_dir)) |meta| { | ||
| if (meta.license) |l| license = l; | ||
| if (meta.author) |au| author = au; | ||
| } | ||
| } |
There was a problem hiding this comment.
Since resolvePluginDir and readPluginMeta now propagate OutOfMemory errors, we should use try at their call sites in cmdPlugins to correctly bubble up any allocation failures.
if (try resolvePluginDir(a, project_dir, dep)) |plugin_dir| {
if (try readPluginMeta(a, plugin_dir)) |meta| {
if (meta.license) |l| license = l;
if (meta.author) |au| author = au;
}
}
| var meta = readPluginMeta(std.testing.allocator, dir).?; | ||
| defer meta.deinit(); | ||
|
|
||
| try std.testing.expectEqualStrings("atlas-overlay", meta.name); | ||
| try expect.equal(meta.manifest_version, @as(u8, 1)); | ||
| try std.testing.expectEqualStrings("MIT", meta.license.?); | ||
| try std.testing.expectEqualStrings("Acme Games", meta.author.?); |
There was a problem hiding this comment.
Update the test cases to use try when calling readPluginMeta to match its new error-propagating signature.
var meta = (try readPluginMeta(std.testing.allocator, dir)).?;
defer meta.deinit();
try std.testing.expectEqualStrings("atlas-overlay", meta.name);
try expect.equal(meta.manifest_version, @as(u8, 1));
try std.testing.expectEqualStrings("MIT", meta.license.?);
try std.testing.expectEqualStrings("Acme Games", meta.author.?);
| var meta = readPluginMeta(std.testing.allocator, dir).?; | ||
| defer meta.deinit(); | ||
|
|
||
| try std.testing.expectEqualStrings("bare", meta.name); | ||
| try expect.toBeNull(meta.license); | ||
| try expect.toBeNull(meta.author); |
There was a problem hiding this comment.
Update the test cases to use try when calling readPluginMeta to match its new error-propagating signature.
var meta = (try readPluginMeta(std.testing.allocator, dir)).?;
defer meta.deinit();
try std.testing.expectEqualStrings("bare", meta.name);
try expect.toBeNull(meta.license);
try expect.toBeNull(meta.author);
| try expect.toBeNull(readPluginMeta(std.testing.allocator, dir)); | ||
| } |
| fn resolvePluginDir( | ||
| allocator: std.mem.Allocator, | ||
| project_dir: []const u8, | ||
| dep: project_config.PluginDep, | ||
| ) ?[]const u8 { | ||
| if (dep.isLocal()) | ||
| return std.fs.path.resolve(allocator, &.{ project_dir, dep.localPath() }) catch null; | ||
| return asm_cache.resolveRemotePluginDir(allocator, dep.repo, dep.version) catch null; | ||
| } |
There was a problem hiding this comment.
Similar to readPluginMeta, resolvePluginDir should propagate OutOfMemory errors instead of swallowing them with catch null. Let's update the signature to return !?[]const u8 and explicitly propagate OutOfMemory while falling back to null for other resolution errors.
fn resolvePluginDir(
allocator: std.mem.Allocator,
project_dir: []const u8,
dep: project_config.PluginDep,
) !?[]const u8 {
if (dep.isLocal())
return std.fs.path.resolve(allocator, &.{ project_dir, dep.localPath() }) catch |err| {
if (err == error.OutOfMemory) return error.OutOfMemory;
return null;
};
return asm_cache.resolveRemotePluginDir(allocator, dep.repo, dep.version) catch |err| {
if (err == error.OutOfMemory) return error.OutOfMemory;
return null;
};
}
Review follow-up on #300: `readPluginMeta` and `resolvePluginDir` used `catch → null` on every fallible call, which quietly dropped a plugin from the listing on a real allocation failure. Make both fallible: - `readPluginMeta` now returns `error{OutOfMemory}!?PluginMeta`; OOM from path.join / dupeZ / the ZON parser / the field dupes propagates, while the legitimate "no manifest (FileNotFound)", unreadable, or unparseable cases still degrade to `null` with a warning. Added errdefers so a mid-way OOM doesn't leak the already-duped name/license. - `resolvePluginDir` now returns `![]const u8` (no optional): resolution is pure path construction with no "not found" case, so OOM / NoHomeDirectory surface instead of collapsing to null. - Callers + the three test sites updated to `try`. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3d8ae3b94
ℹ️ 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".
| if (readPluginMeta(a, plugin_dir)) |meta| { | ||
| if (meta.license) |l| license = l; | ||
| if (meta.author) |au| author = au; |
There was a problem hiding this comment.
Validate manifest names before using provenance
When a local path points at the wrong plugin, or a remote cache contains a stale/corrupt plugin.labelle whose .name differs from the .plugins entry, this block still copies that manifest's license/author into the row labelled with dep.name. That misattributes provenance for a different plugin; compare meta.name with dep.name and warn/fall back to placeholders before using these fields.
Useful? React with 👍 / 👎.
Asset Plugins Phase 2 — CLI side (#300)
Adds a
labelle plugins [dir]subcommand that surfaces plugin provenance: for each plugin declared inproject.labelle's.pluginslist it reads that plugin'splugin.labelleand prints an aligned table of name, declared version, license, author.Example
What's in it
src/cli/plugins.zigreadPluginMeta— parsesname/manifest_versionplus the new optionallicense/authorfields viastd.zon.parse.fromSliceAllocwith.ignore_unknown_fields = true. A missingplugin.labelle(legal — many plugins ship none) or a malformed one is tolerated: the row falls back to-rather than aborting the listing.resolvePluginDir— mirrors the local (local:/@) vs remote (~/.labelle/packages/plugins/<repo>/<version>) resolution the lockfile writer already uses.renderTable— a pure, unit-testable aligned-table renderer.cli.zigdispatch as a standalone command (readsproject.labelleitself; clean exit-1 + guidance when run outside a project) and surfaced inlabelle help.-placeholders.zig build testis green.Dependency-ordering note
The generate-time
depends_on_resourcesvalidation (an undeclared game-atlas dependency failinggenerate) is delivered assembler-side in labelle-assembler#576 — the CLI shells out to thelabelle-assemblerbinary forgenerateand never parses the merged resource set, so it cannot meaningfully perform that validation locally. #300's CLI deliverable is this provenance-surfacing listing plus thelicense/authorreader; the two land as a pair.Closes #300
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw